davidgnx
commented
4 months ago Hi @linguini1!
First of all: the code looks beautiful, understandable and nicely commented, thanks for this. Also thank you for your motivation and welcome to the NuttX community!
I know that your fork is work-in-progress, but I tried to figure out what is wrong to help you. During this "review" I have noticed a few things that you are probably not aware of... These are just suggestions, but they may improve the usability of this driver. :)
1.) Temperature range: The sensor is capable of measuring temperatures between -40 C and +125 C with a resolution of 0.01 C and an accuracy of 0.1 C (SHT45). Your choice to store the results in an int16_t represented as mC or mF limits the theoretical measurement range to: -32.768 C ... +32.767 C in Celsius mode and to -35.98 C ... 0.43 C in Fahrenheit mode. This means that in Fahrenheit mode this driver will barely be able to measure any temperature above the freezing point of water (which is fairly suboptimal for many users).
2.) sht4x_calc_temp(): There are massive integer overflows/underflows for huge ranges of valid input values (input values: raw measurement data points). This is related to 1.), see above.
3.) Humidity range: The sensor is capable of measuring relative humidity between 0 and 100 with a resolution of 0.01 %RH and an accuracy of 1 %RH (SHT45). You store the result of the measurement with a resolution of 1% RH, therefore you lose two digits precision. Hint: The accuracy of the sensor is 1%, so losing two digits from the resolution is not necessarily a problem, it depends on the use case.
4.) sht4x_calc_hum(): hum is an unsigned integer (uint16_t), therefore it can never be < 0. Those values, that are supposed to be < 0 will be mapped to 100 instead of 0 because of an integer overflow/underflow. So your clamped output will look like [ ..., 100, 100, 100, 0, 0, 1, 1, ..., 99, 99, 100, 100, 100, 100 ...] instead of [ ..., 0, 0, 0, 0, 1, 1, ..., 99, 99, 100, 100, 100, 100 ...] (note the wrongly mapped values below zero).
5.) sht4x_ioctl(): The ioctl command SNIOC_READ_CONVERT_DATA converts both temperature and humidity using sht4x_calc_temp(), whereby sht4x_read() is using sht4x_calc_temp() and sht4x_calc_hum() respectively. Not sure if this is intended, probably not.
6.) sht4x_register(): This may (or may not) answer your original question, I didn't verify, just looked at your code briefly. You declare the function in include/nuttx/sensors/sht4x.h, but you do not define it anywhere, or at least I haven't found it in the fork you've linked. There is a function named sht21_register() though (defined in drivers/sensors/sht4x.c), I guess it's a typo or copy-paste error.
If this was the problem, feel free to close this issue.
As this is an issue and not a pull request, sooner or later my comment and your code will drift apart. I did not want to comment on the commits inside your fork, so: the last commit on your fork (branch: sht41-sensor) was https://github.com/linguini1/nuttx/commit/5a367dfa68413a8e311c79a3d6b1b67ada7a11d3 when this comment was written).
Greetings, David
p.s.: The second question (dynamically loading drivers) is not answered by this post, so feel free to comment, thank you!
linguini1
acassis
Hello,
I am relatively new contributor, and I am looking to add support for the SHT4X temperature and humidity sensor. I have a branch that is a work in progress here: https://github.com/linguini1/nuttx/tree/sht41-sensor
I have been able to accomplish most of the driver by looking at the SHT21 and SHT3X drivers made before, which are similar. Most of the i2c logic I have already tested previously on another platform, but I would like to now test on a NuttX build to make sure that all parts of the driver work correctly.
I am struggling to register the driver properly. I have found where the SHT3X was registered on an ESP32 board on the code base, as well as this article: https://www.embeddedrelated.com/showarticle/1668.php
I have tried adding the registration logic to the
rp2040_common_bringup()but receive a linker error that mysht4x_registerfunction cannot be found, despite the inclusion of the correct headers.This also leads me to another question; is there a way to dynamically load these drivers, or must the drivers always be started in the board initialization logic by pre-configuring which sensor drivers to include?