zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.93k stars 6.65k forks source link

Timing Bug in the SHT4x Sensor Driver #80652

Open Nightshadow1258 opened 3 weeks ago

Nightshadow1258 commented 3 weeks ago

I created a custom application for my custom board that used a NRF52840 at its core and is basically just the code of the samples/sensor/sgp40_sht4x sample to read the Sensor data. First I had some issue where the initialization "Failed to reset the device". I was able to fix this by enabling the internal pull-ups for the I2C Interface. Not sure why this was necessary since there are 4.7k Pull-up on the board, but if it works I am not complaining. After that I had troubles reading from the device as can be seen in the image below Pasted image 20241030211751

I went to debug this and found that after setting a break point after the sht4x_write_command function call and before sht4x_read_sample in the sht4x.c file that I get correct readings on my Uart-Output. After that I did some digging in the Datasheet (https://sensirion.com/resource/datasheet/sht4x) that there is a delay of 10ms between writing and reading via the I2C Interface. In the current implementation the delay is defined by the array measure_wait_us inside of the sht4.h file static const uint16_t measure_wait_us[3] = { 1700, 4500, 8200 }; This corresponds to the "Measurement Duration" in the datasheet Pasted image 20241030220816, although some values are slightly different (probably Datasheet changes). For testing I just set them all to 10000 (which will be a 10ms delay) and voila after that I can read the sensor without any errors.

Since I also verified before that my Board and the Sensor were working by using Arduino and PlatformIO. I check the implementation (https://github.com/Sensirion/arduino-i2c-sht4x/blob/master/src/SensirionI2cSht4x.cpp) there and found that they use slightly longer delays of 2ms,5ms,10ms. I tested them aswell by changing the values from above to the following: static const uint16_t measure_wait_us[3] = { 2000,5000, 10000 }; After testing all three repeatability settings, I can say that they all worked for me. Pasted image 20241030221411 I suspect that the "Measurement Duration" form the Datasheet is the time needed for the measurement without the time needed to write them to the registers. At least that would explain why adding some additional time is working.

Maybe someone else can confirm this behaviour on there end as well? I can also do a PR if this is wanted.

Kind Regards

github-actions[bot] commented 3 weeks ago

Hi @Nightshadow1258! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

JarmouniA commented 3 weeks ago

Hi @Nightshadow1258 I have a theory, that I haven't yet verified, that the problem is not actually with the timings but rather with how k_sleep is used in the sensor driver. From a quick look at the driver, I noticed it doesn't verify that the specified time has actually passed after calling k_sleep, by checking its return value. https://github.com/zephyrproject-rtos/zephyr/blob/743761d7d1d7be7ef7cf0614e37ead5eb748b79e/drivers/sensor/sensirion/sht4x/sht4x.c#L112 https://github.com/zephyrproject-rtos/zephyr/blob/743761d7d1d7be7ef7cf0614e37ead5eb748b79e/include/zephyr/kernel.h#L549 Also I noticed that Arduino wait values are rounded up milliseconds, so it could be a truncation issue https://github.com/zephyrproject-rtos/zephyr/blob/743761d7d1d7be7ef7cf0614e37ead5eb748b79e/include/zephyr/sys_clock.h#L128

Nightshadow1258 commented 3 weeks ago

Hey @JarmouniA, thank you for your response. Today I tried a different board with the default values https://github.com/zephyrproject-rtos/zephyr/blob/743761d7d1d7be7ef7cf0614e37ead5eb748b79e/drivers/sensor/sensirion/sht4x/sht4x.h#L43C1-L45C3 which worked with out the errors mentioned in my first post. For testing I tried again with the "not working" Board and sure enough I got the same errors again which are resolved by increasing the time. This makes pinpointing the issue difficult. For testing I printed the return value of the k_sleep to my UART output, but I always read 0 here, regardless if the error occurs or not. Maybe I just got a "bad" Sensor patch on my Boards. I will have to make a new Revision anyway soon. Kind regards

teburd commented 3 weeks ago

does zephyr's timer_behavior test pass on the board that's failing? It could be that timers on this board aren't quite working correctly. Have you inspected the timing with external devices like a logic analyzer or scope to verify that the timing is in fact what you think it should be?

str4t0m commented 2 weeks ago

static const uint16_t measure_wait_us[3] = { 1700, 4500, 8200 };

Sensirion seems to have have made slight adjustments to these timing values with the release of the 3rd revision of the datasheet. The values used in the driver match exactly the older timing values. I have verified that the driver still works on my boards, so this really seems to be related to your board and I wouldn't increase the wait time unnecessarily.

While it won't fix your issue, I have opened #81094 to update the wait times to be aligned with the datasheet.