Support for embedded-hal 1.0.0 and async #5
Conversation
…n the work by @tazz4843. The reason I did not use his branch is that I wanted this to be based on embedded-hal-1.0.0. Since the signatures changed, this allowed for puttign all the methods in the same impl block and then feature gating them.
- information about async feature - use of embedded-hal 1.0.0 - updated the example
|
@sirhcell, is there anything you'd like changed? I'd love to get this pull request merged in, if you think it is ok. |
|
@sirhcel It would be great if you could approve this PR and get this merged in. Thank you for your consideration! |
|
I have temporarily forked this project and published crate sht4x-ng. That means the cargo.toml in this pull request would need to be edited to put back the original name. I would still love to see these changes upstreamed. |
fix clippy warnings
|
Thank you very much for your PR! I finally managed to also look into the async part of it. It took me a while because adding async support to an existing driver still feels a bit alienating. The support for it got better with maybe-async and maybe-async-cfg since I looked at #1 and #2 last time. But neither of the two buys us out for an embedded-hal driver yet as far as I can tell. I really don't like the code duplication associated with it. But massaging maybe-async-cfg into working with the type parameters of the drivers struct is beyond my current schedule and "pay grade" - and so it looks like duplicating code is the way to go in the short term.
To my understanding, Cargo features should be additive. A feature switching between sync and async will break in a dependency tree where both variants are used. So I would prefer always providing sync support and enabling async support with the feature What are your points for the toggling feature flag? I definitely see the benefit of having sync and async methods spatially next to each other. But from my experience with dependency trees, this would be payed with a lot of potential hassle in dependency trees. And the latter will be "payed" by the users and not the authors of this crate. |
|
Hi,I will need to look at all your points more closely soon. Thank you for getting back to me. I really appreciate it.I have not yet completely examined each of your points, but I aim to soon to make sure I properly understand them when I get a chance. This is the first embedded driver I have contributed to, and as such the “mental model” I used to assess the pros and cons of this approach is most definitely not very complete. I was also not aware of the maybe-async crates and will need to learn more about them.The duplication of methods also bothers me, but I thought gating them behind a feature toggle would at least keep them near each other. There is also the other PR you mentioned which I have not examined yet examined that proposes using a different type for async. To be honest, that idea sounds more Rusty on the surface than using a toggling feature. Thanks again. I look forward to learning more and finding a good way forward. It’s not important to me whether my PR or another PR gets async into the driver. It was just a need I had for a project that I thought would be helpful for others. I really did not want to fork the driver (it just makes finding the right crate to use even harder for everyone) and would be happy, once we figure everything out, to publish a new version with a README pointing back to your crate and then yanking all the versions of the forked as it will have outlived its usefulness.Sent from my phoneOn Oct 13, 2024, at 2:18 PM, Christian Meusel ***@***.***> wrote:
Thank you very much for your PR! I finally managed to also look into the async part of it.
It took me a while because adding async support to an existing driver still feels a bit alienating. The support for it got better with maybe-async and maybe-async-cfg since I looked at #1 and #2 last time. But neither of the two buys us out for an embedded-hal driver yet as far as I can tell.
I really don't like the code duplication associated with it. But massaging maybe-async-cfg into working with the type parameters of the drivers struct is beyond my current schedule and "pay grade" - and so it looks like duplicating code is the way to go in the short term.
@sirhcell, is there anything you'd like changed? I'd love to get this pull request merged in, if you think it is ok.
To my understanding, Cargo features should be additive. A feature switching between sync and async will break in a dependency tree where both variants are used. So I would prefer always providing sync support and enabling async support with the feature async and providing the async variant of the driver as Sht4xAsync. Which at a first glance boils the changes down to #7.
What are your points for the toggling feature flag?
I definitely see the benefit of having sync and async methods spatially next to each other. But from my experience with dependency trees, this would be payed with a lot of potential hassle in dependency trees. And the latter will be "payed" by the users and not the authors of this crate.
—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you authored the thread.Message ID: ***@***.***>
|
This pull request starts with the changes made by @madmo in his pull request (#3).
It then takes ideas from the pull request @tazz4843 has for adding async support (#2). It differs in two main ways:
asyncfeature is used as a toggle instead of having ablockingfeature and anasyncfeatureI hope this is helpful. I initially created a pull request into @madmo's branch, but I have not heard back and it kind of muddies the intent of his pull request as its scope was only embedded-hal 1.0.0 and not async.