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.
Edit: Original response was through email and the formatting was very off and difficult to read.
|
|
Hi @sirhcel. I have not been able to work on this further. Life has gotten pretty busy. :-( Do you have any example libraries I could look at which use the techniques you are discussing? I do not particularly like the duplication either. My main rationale for doing it the way I did is that it:
I figured keeping them near each other would make it less likely someone would only update the synchronous or asynchronous code. Not having two sets of code; however, would make it impossible to forget. It could also lead to less lines of code. I think you are on to something. I just don't really know where to begin and have been pressed for time. I think the downside to #7 is that it might be easy to forget to update methods in one or the other. The flip side is that it is probably unlikely the logic for the sensor would change much over time. It is likely going to be more about tracking changes to esp-hal and esp-hal-async. It would be easier to sort those changes with the implementations being separated as they are in #7. I still would like to get some version of async into your code as I would prefer to abandon my fork and put a note in it pointing back to this original one. At the same time, the way I coded it seems sub optimal, and perhaps a bit hacky. It makes sense not to accept this pull request in light of that. #7 might be a good option instead, if you feel it is a better direction to go in. Thanks and Happy Holidays / Happy New Year! |
You name it. It was the same on my side too.
I played with maybe-async-cfg in 7017fef but I this did not allow to define different variants of the driver struct and I abandoned it.
Thank you for explaining your rationale and your proposal at all! I see your point and I am with you. What made me finally go for #7 was having async support as and additive feature.
I landed #7 today and I will cut a new release the coming days. I would not call it hacky. Async support for an embedded driver still feels a bit like we are in the phase of experimenting. So it was definitely worth the effort. Thank you very much for that!
Thank you! I hope you had a good start as well. |
|
Hi Christian,
That is great news there is now async in the official driver. This Monday I aim to update the Readme on my fork to recommend going back to your official version. I do not think having this forked is good for the community. Thanks for bringing async to the official driver. :)
Brian
Edit: Removed all the email detritus.
|
Release 0.2.0 is out and I published the examples I used for testing as sht4x-examples. A small migration guide for switching back to sht4x might come in handy. Please feel free to use the examples if you want. |
|
Thanks again. I have deprecated sht4x-ng on crates.io. Hopefully these changes make it clear enough to someone they should use sht4x. |
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.