Add option in DHT driver to do the measure without blocking interrupts #6946

EddieParis · 2021-02-21T09:41:54Z

This is done because ESP8266 has a PWM relying on interrupts.
Doing the DHT measure with interruption blocking, sometimes makes
glitches on the PWM.

fixes #6945

This is done because ESP8266 has a PWM relying on interrupts. Doing the DHT measure with interruption blocking, sometimes makes glitches on the PWM.

EddieParis · 2021-02-21T10:24:09Z

To reproduce issue, assuming dht is on gpio 0 and led on gpio14

import dht
from machine import Pin, PWM
p=PWM(Pin(14))
p.freq(150)
p.duty(100)
d=dht.DHT22(Pin(0))
while True:
d.measure()

You should see glitches on the leds

change to d=dht.DHT22(Pin(0), False), no more glitches

kadamski · 2021-02-24T14:09:43Z

drivers/dht/dht.c

@@ -37,7 +37,9 @@
 #define mp_hal_pin_od_high_dht mp_hal_pin_od_high
 #endif

-STATIC mp_obj_t dht_readinto(mp_obj_t pin_in, mp_obj_t buf_in) {
+STATIC mp_obj_t dht_readinto(mp_obj_t pin_in, mp_obj_t buf_in, mp_obj_t irq_lock) {
+    mp_uint_t irq_state=0;


This does not follow the code style in the file. Put spaces around '='. Also there seems to be no need for the 0 initialization.

unfortunately you have to :-( That is why I added it.

+1: ../../drivers/dht/dht.c: In function 'dht_readinto':
./xtirq.h:48:5: error: 'irq_state' may be used uninitialized in this function [-Werror=maybe-uninitialized]
asm volatile ("wsr %0, ps; rsync" : : "a" (ps));
^
../../drivers/dht/dht.c:41:15: note: 'irq_state' was declared here
mp_uint_t irq_state;
^
cc1: all warnings being treated as errors

Well, either I'm missing something or compiler is wrong about that but well, in any way, we want to make it happy.

kadamski · 2021-02-24T14:09:48Z

docs/esp8266/tutorial/dht.rst

+    >>> while not measure_ok:
+    >>>     try:
+    >>>         d.measure()
+    >>>         measure_ok = True


A minor comment, I think the more common way of handling it would be to get rid of this variable and use brake like so:

while True: try: d.measure() break except [...]

yes ! good point.

kadamski · 2021-02-24T14:15:10Z

drivers/dht/dht.c

@@ -54,7 +56,9 @@ STATIC mp_obj_t dht_readinto(mp_obj_t pin_in, mp_obj_t buf_in) {
    mp_hal_pin_od_low(pin);
    mp_hal_delay_ms(18);

-    mp_uint_t irq_state = mp_hal_quiet_timing_enter();
+    if (mp_obj_is_true(irq_lock)) {


mp_obj_is_true() is a little involved function so it would be worth calling it only once instead if tree times. You should consider creating a bool that will "cache" this computation once, something like:
bool irq_lock_true = mp_obj_is_true(irq_lock);

well, I considered the call to mp_obj_is_true() as almost free. I can do that if you prefer.

Check its implementation here. For most cases it is, indeed, but if someone passes some object there, it does require quite some checks.
Ultimately it's not my decision, I'm just a random guy doing a review, but I do think its worth saving those few CPU cycles.

kadamski · 2021-02-24T14:21:43Z

drivers/dht/dht.py


    def measure(self):
        buf = self.buf
-        dht_readinto(self.pin, buf)
+        dht_readinto(self.pin, buf, self.irq_block)


We could consider putting a retry loop here in case self.irq_block is false to free the users from doing that. I would avoid the infinite loop, though.. so maybe some fixed amount of retries should be introduced?

I prefer to let the user decide to manage that as he prefers. In my case, I have other things to do so I can't lock main loop for too long, so I retry later. DHT is already really slow, not worth adding more delays.

Seems valid argument, indeed. I was just afraid that anybody has to implement this loop while using this argument anyways.

With a DHT sensor you have to implement a try:except: retry.. loop anyway because they are not very stable.

kadamski · 2021-02-24T14:22:40Z

As a general comment, the same issue exist with onewire module (as DHT protocol is very similiar to onewire bus) so if you are already putting a warning to the documentation, maybe it would make sense to mention onewire case as well?

EddieParis · 2021-02-24T19:21:45Z

Yeah, why not. But in the onewire, bits are read one by one and the protocol is waaaaaay faster (locking is shorter), so I'm not sure it can produce visible effects.

kadamski · 2021-02-25T10:38:34Z

Yeah, why not. But in the onewire, bits are read one by one and the protocol is waaaaaay faster (locking is shorter), so I'm not sure it can produce visible effects.

True. But if this (locking for ~70us) is not a problem then maybe for DHD it would also be ok to do a lock/unlock in every iteration of the loop (which takes 80-120us) and only for a machine_time_pulse_us(). This way we still can get a wrong result if entering into the critical region is delayed much more than 50us (the time of a low pulse before each high pulse we are measuring) but we might get a little better results than without any locking while the overall latency would be still be much smaller.

DHT will send 26-28us pulse for "0" and 70us for "1" but dht.c is considering a "1" everything that is >48us so we already have a safety margin of around 22us in case our machine_time_pulse_us() starts late (when the 50us low singal is already over).

To sum up, my understanding is this:

If we do not do any locking:
- we might get an interrupt before machine_time_pulse_us() starts counting "high" time - this may give us shorter measurement so we might end up interpreting "1" as "0"
- we might also get an interrupt during machine_time_pulse_us() counting "high" time - this may give us longer measurement so we may interpret "0" as "1"
If we do machine_time_pulse_us inside of a lock that is released and reackquired each iteration:
- we might get an interrupt before machine_time_pulse_us starts so we can also interpret "1" as "0"
- but the interrupt during machine_time_pulse_us() will be delayed so we can't interpret "0" as "1"
- the downside is that we can introduce up to around 100us of latency but that is still nothing compared to the ~4ms of latency we have with full locking.

What do you think ?

EddieParis · 2021-02-25T18:02:19Z

Theoretically you are right. But I think this is over complicated for a very little benefit. My software is running for almost a year with this patch and 3 channel PWM in parallel, I observed very few checksum errors, and no weird measures that could have gone through checksum verification.
Any third party opinion ?

projectgus · 2024-03-07T23:52:58Z

This is an automated heads-up that we've just merged a Pull Request
that removes the STATIC macro from MicroPython's C API.

See #13763

A search suggests this PR might apply the STATIC macro to some C code. If it
does, then next time you rebase the PR (or merge from master) then you should
please replace all the STATIC keywords with static.

Although this is an automated message, feel free to @-reply to me directly if
you have any questions about this.

EddieParis force-pushed the master branch 2 times, most recently from 4fdf251 to 2021e34 Compare February 21, 2021 09:52

drivers/dht: Add option to do the measure without blocking interrupts.

cca2f91

This is done because ESP8266 has a PWM relying on interrupts. Doing the DHT measure with interruption blocking, sometimes makes glitches on the PWM.

EddieParis force-pushed the master branch from f5c1ced to cca2f91 Compare February 21, 2021 10:01

kadamski suggested changes Feb 24, 2021

View reviewed changes

kadamski reviewed Feb 24, 2021

View reviewed changes

drivers/dht/dht.c: Modifications after pull requests comments.

e6dbe2a

dpgeorge added the drivers label Nov 30, 2021

Gadgetoid mentioned this pull request Feb 29, 2024

global: Remove the STATIC macro. #13763

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option in DHT driver to do the measure without blocking interrupts #6946

Add option in DHT driver to do the measure without blocking interrupts #6946

EddieParis commented Feb 21, 2021

EddieParis commented Feb 21, 2021

kadamski Feb 24, 2021

EddieParis Feb 24, 2021

kadamski Feb 24, 2021

kadamski Feb 24, 2021

EddieParis Feb 24, 2021

kadamski Feb 24, 2021

EddieParis Feb 24, 2021

kadamski Feb 24, 2021

kadamski Feb 24, 2021

EddieParis Feb 24, 2021 •

edited

Loading

kadamski Feb 24, 2021

kevinkk525 Feb 24, 2021

kadamski commented Feb 24, 2021

EddieParis commented Feb 24, 2021 •

edited

Loading

kadamski commented Feb 25, 2021 •

edited

Loading

EddieParis commented Feb 25, 2021

projectgus commented Mar 7, 2024

Add option in DHT driver to do the measure without blocking interrupts #6946

Are you sure you want to change the base?

Add option in DHT driver to do the measure without blocking interrupts #6946

Conversation

EddieParis commented Feb 21, 2021

EddieParis commented Feb 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EddieParis Feb 24, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadamski commented Feb 24, 2021

EddieParis commented Feb 24, 2021 • edited Loading

kadamski commented Feb 25, 2021 • edited Loading

EddieParis commented Feb 25, 2021

projectgus commented Mar 7, 2024

EddieParis Feb 24, 2021 •

edited

Loading

EddieParis commented Feb 24, 2021 •

edited

Loading

kadamski commented Feb 25, 2021 •

edited

Loading