drivers/dht: use IRQs for decoding #18591
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This changed the DHT implementation to use an GPIO IRQ for decoding the received data instead of busy polling. This should improve real time behavior of the system and improve robustness of the reception under load.
maribu
requested review from
aabadie,
fjmolinas,
DipSwitch,
vincent-d and
haukepetersen
as code owners
github-actions
bot
added
Area: cpu
|
Output on the Nucleo F767ZI (with Same on the nRF52840-DK: |
maribu
changed the title
|
The issue has been traced down to an identical bug in STM32 and nRF5x Since the bug is fixed, this should now actually work. |
| unsigned bit = (now - arg->time) > PULSE_WIDTH_THRESHOLD; | ||
|
|
||
| if (bit) { | ||
| arg->data[pos >> 3] |= (0x80U >> (pos & 7)); |
maribu
added
the
State: waiting for other PR
|
To avoid confusion: The bug fix in #19674 should go in first. I'll rebase this one on top. Likely it also makes sense to have the IRQ based implementation as an alternative to the polling based one, e.g. when the sensor is connected to a pin that has no IRQ support. |
hugueslarrive
added a commit
to hugueslarrive/RIOT
that referenced
this pull request
Jun 8, 2023
- many backports from @maribu's IRQ based implementation (RIOT-OS#18591) - reintroduction of DHT11/DHT22 differentiation - use of ztimer - use of errno.h - separation of dht_read() steps into functions for better readability - simplification of bit identification - sensor presence checking in dht_init() - automatic adjustment to a minimum data hold time - default input mode changed to open drain - cleaner AVR support
hugueslarrive
added a commit
to hugueslarrive/RIOT
that referenced
this pull request
Jun 10, 2023
- many backports from @maribu's IRQ based implementation (RIOT-OS#18591) - use of ztimer and errno.h - separation of dht_read() steps into functions for better readability - reintroduction of DHT11/DHT22 differentiation - sensor presence checking in dht_init() - automatic adjustment to a minimum data hold time - default input mode changed to open drain - AVR support without platform-specific handling by avoiding ztimer_spin() and using the overflow of an 8-bit variable as a pre-timeout to minimize time-consuming ztimer_now() calls
hugueslarrive
added a commit
to hugueslarrive/RIOT
that referenced
this pull request
Jun 10, 2023
- many backports from @maribu's IRQ based implementation (RIOT-OS#18591) - use of ztimer and errno.h - separation of dht_read() steps into functions for better readability - reintroduction of DHT11/DHT22 differentiation - sensor presence checking in dht_init() - automatic adjustment to a minimum data hold time - default input mode changed to open drain - AVR support without platform-specific handling by avoiding ztimer_spin() and using the overflow of an 8-bit variable as a pre-timeout to minimize time-consuming ztimer_now() calls
hugueslarrive
added a commit
to hugueslarrive/RIOT
that referenced
this pull request
Jun 12, 2023
- many backports from @maribu's IRQ based implementation (RIOT-OS#18591) - use of ztimer and errno.h - separation of dht_read() steps into functions for better readability - reintroduction of DHT11/DHT22 differentiation - sensor presence checking in dht_init() - automatic adjustment to a minimum data hold time - default input mode changed to open drain - AVR support without platform-specific handling by avoiding ztimer_spin() and using the overflow of an 8-bit variable as a pre-timeout to minimize time-consuming ztimer_now() calls
hugueslarrive
added a commit
to hugueslarrive/RIOT
that referenced
this pull request
Jun 20, 2023
- many backports from @maribu's IRQ based implementation (RIOT-OS#18591) - use of ztimer and errno.h - separation of dht_read() steps into functions for better readability - reintroduction of DHT11/DHT22 differentiation - sensor presence checking in dht_init() - default input mode changed to open drain - AVR support without platform-specific handling by avoiding ztimer_spin() and using the overflow of an 8-bit variable as a pre-timeout to minimize time-consuming ztimer_now() calls - add a new DHT11_2022 type for 0.01 °C resolution devices - data caching removed
hugueslarrive
added a commit
to hugueslarrive/RIOT
that referenced
this pull request
Jun 20, 2023
- many backports from @maribu's IRQ based implementation (RIOT-OS#18591) - use of ztimer and errno.h - separation of dht_read() steps into functions for better readability - reintroduction of DHT11/DHT22 differentiation - sensor presence checking in dht_init() - default input mode changed to open drain - AVR support without platform-specific handling by avoiding ztimer_spin() and using the overflow of an 8-bit variable as a pre-timeout to minimize time-consuming ztimer_now() calls - add a new DHT11_2022 type for 0.01 °C resolution devices - data caching removed
bors bot
added a commit
that referenced
this pull request
Jun 20, 2023
19718: drivers/dht: busy wait reimplementation r=maribu a=hugueslarrive ### Contribution description In PR #19674, I also provided quick and dirty fixes to restore functionality on esp8266 and enable operation on AVR. While reviewing PR #18591, it became apparent to me that this driver needed a refresh, particularly its migration to ztimer. The cause of the malfunction on esp8266 was that since the default switch to ztimer as the backend for xtimer, XTIMER_BACKOFF was no longer taken into account. Therefore, the correction I provided in PR #19674 simply made explicit what was previously done implicitly with xtimer and now needs to be done explicitly with ztimer (spinning instead of sleeping). Moreover, it was unnecessarily complex to measure the pulse duration in a busy-wait implementation, which required 2 calls to ztimer_now() and 32-bit operations expensive on 8-bit architecture. Instead, it is sufficient to read the state of the bus at the threshold moment. Finally, in practice, it is possible to reduce the read interval (down to less than 0.5s for DHT22) by "harassing" the DHT with start signals until it responds. This re-implementation brings the following improvements: - Many backports from `@maribu's` IRQ based implementation (#18591): - Use of ztimer - Use of errno.h - Use of a dht_data structure to pass arguments, to facilitate integration - Adaptation of the bit parsing technique to parse bits into the data array - Reintroduction of DHT11/DHT22 differentiation. - Separation of `dht_read()` steps into functions for better readability and the ability to share certain functions among different implementations - Sensor presence check in `dht_init()` - ~~Automatic adjustment to a minimum data hold time~~ - Default input mode changed to open drain (a pull-up resistor should be placed close to the output if necessary but not close to the input) - AVR support without platform-specific handling by avoiding ztimer_spin() and using the overflow of an 8-bit variable as a pre-timeout to minimize time-consuming ztimer_now() calls Regarding the changes in the start signal sequence and the removal of the `_reset()` function:  ~~In the previous implementation, there was an unnecessary spike at the beginning of the signal sequence, corresponding to START_HIGH_TIME. This spike has been removed in the re-implementation, as it is unnecessary. Instead, the MCU now simply pulls the signal low for START_LOW_TIME and then releases the bus, which is sufficient for initiating communication with the DHT sensor.~~ Actually, it is necessary to raise the bus level; otherwise, the _wait_for_level() called immediately after to check the response of the DHT may read the port before the signal level is raised, resulting in a false positive. Additionally, the previous implementation had an issue where the MCU switched back to output mode and went high immediately after reading the 40 bits of data. However, the DHT sensor was still transmitting 2 or 3 additional bytes of '0' at that point, causing a conflict. This issue has been resolved in the re-implementation:  ~~Regarding the optimization for AVR, I have performed measurements of `_wait_for_level()` until timeout (85 loops):~~ ~~- on esp8266-esp-12x: 264 µs, which is 3.11 µs per loop~~ ~~- on nucleo-f303k8: 319 µs, which is 3.75 µs per loop~~ ~~- on arduino-nano: 3608 µs, which is 42.45 µs per loop~~ ~~Duration measurements on the Arduino Nano:~~ Co-authored-by: Hugues Larrive <hlarrive@pm.me>
bors bot
added a commit
that referenced
this pull request
Jun 20, 2023
19718: drivers/dht: busy wait reimplementation r=benpicco a=hugueslarrive ### Contribution description In PR #19674, I also provided quick and dirty fixes to restore functionality on esp8266 and enable operation on AVR. While reviewing PR #18591, it became apparent to me that this driver needed a refresh, particularly its migration to ztimer. The cause of the malfunction on esp8266 was that since the default switch to ztimer as the backend for xtimer, XTIMER_BACKOFF was no longer taken into account. Therefore, the correction I provided in PR #19674 simply made explicit what was previously done implicitly with xtimer and now needs to be done explicitly with ztimer (spinning instead of sleeping). Moreover, it was unnecessarily complex to measure the pulse duration in a busy-wait implementation, which required 2 calls to ztimer_now() and 32-bit operations expensive on 8-bit architecture. Instead, it is sufficient to read the state of the bus at the threshold moment. Finally, in practice, it is possible to reduce the read interval (down to less than 0.5s for DHT22) by "harassing" the DHT with start signals until it responds. This re-implementation brings the following improvements: - Many backports from `@maribu's` IRQ based implementation (#18591): - Use of ztimer - Use of errno.h - Use of a dht_data structure to pass arguments, to facilitate integration - Adaptation of the bit parsing technique to parse bits into the data array - Reintroduction of DHT11/DHT22 differentiation. - Separation of `dht_read()` steps into functions for better readability and the ability to share certain functions among different implementations - Sensor presence check in `dht_init()` - ~~Automatic adjustment to a minimum data hold time~~ - Default input mode changed to open drain (a pull-up resistor should be placed close to the output if necessary but not close to the input) - AVR support without platform-specific handling by avoiding ztimer_spin() and using the overflow of an 8-bit variable as a pre-timeout to minimize time-consuming ztimer_now() calls Regarding the changes in the start signal sequence and the removal of the `_reset()` function:  ~~In the previous implementation, there was an unnecessary spike at the beginning of the signal sequence, corresponding to START_HIGH_TIME. This spike has been removed in the re-implementation, as it is unnecessary. Instead, the MCU now simply pulls the signal low for START_LOW_TIME and then releases the bus, which is sufficient for initiating communication with the DHT sensor.~~ Actually, it is necessary to raise the bus level; otherwise, the _wait_for_level() called immediately after to check the response of the DHT may read the port before the signal level is raised, resulting in a false positive. Additionally, the previous implementation had an issue where the MCU switched back to output mode and went high immediately after reading the 40 bits of data. However, the DHT sensor was still transmitting 2 or 3 additional bytes of '0' at that point, causing a conflict. This issue has been resolved in the re-implementation:  ~~Regarding the optimization for AVR, I have performed measurements of `_wait_for_level()` until timeout (85 loops):~~ ~~- on esp8266-esp-12x: 264 µs, which is 3.11 µs per loop~~ ~~- on nucleo-f303k8: 319 µs, which is 3.75 µs per loop~~ ~~- on arduino-nano: 3608 µs, which is 42.45 µs per loop~~ ~~Duration measurements on the Arduino Nano:~~ 19737: dist/tools/openocd: start debug-server in background and wait r=benpicco a=fabian18 19746: buildsystem: Always expose CPU_RAM_BASE & SIZE flags r=benpicco a=Teufelchen1 ### Contribution description Hello 🐧 This moves the definition of `CPU_RAM_BASE/SIZE` from being only available in certain situation to be always available. Reason for change is to simplify common code in the cpu folder. In cooperation with `@benpicco` ### Testing procedure Passing CI ### Issues/PRs references First usage will be in the PMP driver. Although there is more code in RIOT that could be refactored to use these defines instead of hacks / hardcoded values. Co-authored-by: Hugues Larrive <hlarrive@pm.me> Co-authored-by: Fabian Hüßler <fabian.huessler@ml-pa.com> Co-authored-by: Teufelchen1 <bennet.blischke@outlook.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Contribution description
This changed the DHT implementation to use an GPIO IRQ for decoding the received data instead of busy polling. This should improve real time behavior of the system and improve robustness of the reception under load.
Testing procedure
I used an Nucleo-F767ZI with the DHT data pin connected to the bottom left pin on the bottom left female pin header and the following patch:
For some reason, the reading fails every second time because theztimer_sleep(ZTIMER_USEC, START_LOW_TIME);(withSTART_LOW_TIME == (20U * US_PER_MS)) returns every second time after 11 µs instead of after 20 ms. I could reproduce the issue also on the nRF52840 DK.This indicates either a bug in this driver or in ztimer.Update: The issue was #18976 (so
periph_timer, notztimer).Issues/PRs references
Will need a rebase on top of #19674 once this is in.