SHT4X sensor driver about nuttx

[acassis]

Nice work @linguini1 !!!

Suggestion: run the checkpatch to catch the coding style issues:

$ ./tools/checkpatch.sh -f drivers/sensors/sht4x.c

You can find about our Coding style here: https://nuttx.apache.org/docs/latest/contributing/coding_style.html

If you decide to create a blog post talking about your first experience creating a driver for NuttX, please share it on our Reddit: https://www.reddit.com/r/nuttx/

[linguini1]

Oh yes, I'm sure my work so far violates all the style rules. Saving my formatting until the end. I will be sure to make a post!

[davidgnx]

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 linguini1@5a367df 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]

Thank you for the kind words and great suggestions @davidgnx!

1), 2), 3) You're correct, I didn't consider the integer precisions. I was porting this from another system where I had previously used floats, but wanted integers to be easier for less powerful hardware. Lots of good considerations that I will take into account, thank you!

5. You're very correct, that was not intended. Thanks for the catch!

6. It seems I did forget to rename the register function... never develop late at night I guess!

I will keep this issue open just a little longer to see if there is any information about dynamically registering drivers, if that's alright. Thanks for your help!