bogde / HX711

An Arduino library to interface the Avia Semiconductor HX711 24-Bit Analog-to-Digital Converter (ADC) for Weight Scales.
MIT License
894 stars 537 forks source link

Waiting for the HX711 using a spinlock? #125

Open amotl opened 5 years ago

amotl commented 5 years ago

Hi @bogde,

along the lines of #123, I am also thinking about giving the current implementation for waiting for the HX711 to become ready

https://github.com/bogde/HX711/blob/899dc7c72415078b054c2b07cb3568e51be038b8/HX711.cpp#L55-L60

a minor facelift. As the outcome might also slightly amend the behavior of the library with respect to current userspace code in the wild, I would propose adding this on top of #123 to at least ship both aspects at once in order to make transitioning easier.

I didn't want to change the semantics related to this together with the infrastructural improvements carried out with #123. Also, I felt changes to that would be worth a discussion if you would have any strong opinions about that, so I ask for your understanding to spawn yet another issue apart from the main "spring-cleaning" thread.

In general, I propose a non-blocking interface like the one @thomasfredericks outlined in #96 with HX711_timeout_example.ino.

This might also help with things like #76 or #111. In general, I would try to design the interface that it would not be possible to deadlock so easily by default without knowing about the implementation details of the read() method. So, it might also become more of a topic for improving the documentation accordingly besides actual code refactoring.

What do you think about that?

With kind regards, Andreas.

amotl commented 5 years ago

By 87727e03, we added some more variants how to wait for the HX711 chip:

// Wait for the HX711 to become ready
void wait_ready(unsigned long delay_ms = 0);
bool wait_ready_retry(int retries = 3, unsigned long delay_ms = 0);
bool wait_ready_timeout(unsigned long timeout = 1000, unsigned long delay_ms = 0);

This is coming from #76 and #96, thanks to @Petorrr and @thomasfredericks again.

amotl commented 5 years ago

Also, out of curiosity we have been wondering if the ready event could even be interrupt-triggered like seen in this example? This could make yet another variant for wait_ready and would probably yield a fully asynchronous implementation.

amotl commented 5 years ago

I think #31, #109 and #111 might also be related. @LwServices already said in #31:

i think it need to be rewritten with attachinterupt or timer?

amotl commented 5 years ago

Thoughts

As this issue just hit @HamidSaffari again (see #135) and the improvements made by #123 obviously haven't been sufficient to solve this, we should think about how to

a) finally get rid of the spin lock altogether by completely switching to an interrupt-based implementation, or

b) improve the way how to either prevent getting into the spin lock altogether, or

c) find a way to limit the spinning time of the spin lock i.e. to also apply the timeout here correctly.

Plan

Regarding a): Recently, I found such an interrupt-based implementation by @stickbreaker at https://github.com/espressif/arduino-esp32/issues/1289#issuecomment-379041436. However, this is ESP32-only and would have to be ported to other architectures. Altogether, while this would be a major rewrite of this library, we should not abandon this in the long run.

Regarding b) and c): In the short term, I would propose to implement a way to limit the spinning time of the spin lock through the timeout value by growing it from an argument to HX711::wait_ready_timeout(unsigned long timeout, unsigned long delay_ms) into an instance attribute of HX711 itself in order to be able to apply it at the right places.