search

jonas-hagen / hx711

A platform agnostic driver to interface with the HX711 (load cell amplifier and ADC)
Apache License 2.0
14 stars 10 forks source link

Don't take ownership of `Delay` #7

Closed fuchsnj closed 1 year ago

fuchsnj commented 3 years ago

Some embedded-hal implementations (such as stmf32f1xx) don't allow you to clone Delay. Since hx711 takes ownership of Delay, it's impossible to use it with other devices that also need to delay. Delay should be passed in as a reference where it's needed.

jonas-hagen commented 3 years ago

Thanks for pointing this out. There are some drivers, that take a reference to Delay for every API call that needs one, indeed. This driver would need one for basically every API call. I remember that I tried to find a way to avoid this and also avoid taking ownership of the Delay. But I do not remember why I decided to implement it the way it is now. Maybe it was just not a priority because I prefer faster Delay implementations (see the README). Maybe I did not find a satisfying solution at the time (lifetimes?).

Besides, it does not seem to be very unreasonable to assume that there is a trait implementation that can be owned. See here:

Would it be true to say that device driver implementations should assume that there will be an implementation of the HAL trait that can be owned when making decisions as to their API?

Yep, basically all drivers make this assumption and to not do so, as you note, creates a nightmarish API.

Is passing a Delay to every API call the only practical alternative to taking ownership?

fuchsnj commented 3 years ago

I'm working on making this change and will open a PR once I've tested it. We can discuss the smaller details on that if you want.

As far as approaches, I don't really see a viable alternative. You can't just store a reference since it needs to be mutable and couldn't be shared anyway.

On Thu, Dec 24, 2020, 5:26 PM Jonas notifications@github.com wrote:

Thanks for pointing this out. Do you have an example ready? This would be very helpful. There are some drivers, that take a reference to Delay for every API call that needs one. I remember that I tried to find a way to avoid this and also avoid taking ownership of the Delay. But I do not remember why I decided to implement it the way it is now. Maybe it was just not a priority because I prefer faster Delay implementations (see the README). Maybe I did not find a satisfying solution at the time (lifetimes?).

Besides, it does not seem to be very unreasonable to assume that there is a trait implementation that can be owned. See here https://github.com/rust-embedded/book/issues/246#issuecomment-645050343:

Would it be true to say that device driver implementations should assume that there will be an implementation of the HAL trait that can be owned when making decisions as to their API?

Yep, basically all drivers make this assumption and to not do so, as you note, creates a nightmarish API.

A specific suggestion would help to discuss the trade-offs, if any.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jonas-hagen/hx711/issues/7#issuecomment-751125249, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABTFOAA5LMDNOFGT4GDOIYLSWO5Y7ANCNFSM4VIMLOOA .

jonas-hagen commented 3 years ago

I am not yet convinced, that adding an extra parameter to each API call is worth it.

In my opinion, it is reasonably easy to implement a Delay that can be owned and cloned. Or just use the implementation in the asm-delay crate. If you want to use the systick delay of the stm32f1xx-hal, then yes unfortunately this cannot be cloned.