drivers/sht11: Major refactoring #9317

maribu · 2018-06-08T22:10:40Z

Renamed driver to sht1x to reflect that fact SHT10, SHT11 and SHT15 are supported
Removed sensor specific shell commands in favor of SAUL
Major refactoring and new features:
- Use RIOT's GPIO interface to access the sensor to increase portability
- Changed API to allow more than one sensor per board
- Added sht1x_params.h that specifies how the sensors is connected - each board can overwrite default settings by #defining SHT1X_PARAM_CLK and SHT1X_PARAM_DATA
- Changed arithmetic to use integer calculations only instead of floating point arithmetic
- Added support for checking the CRC sum
- Allow optional skipping of the CRC check to speed up measuring
- Added support for advanced features like reducing the resolution and skipping calibration to speed up measuring
- Allow specifying the supply voltage of sensor which heavily influences the temperature result (and use that information to calculate the correct temperature)
- Reset sensor on initialization to bring it in a well known state
- Support for the obscure heater feature. (Can be useful to check the temperature sensor?)
Confirmed new integer only arithmetic by unit tests
Added SAUL integration

Hint: Before testing on the MSBA2 or other lpc2387 based boards, please apply this PR containing various bugfixes for the gpio driver.

aabadie

Thanks for refactoring this sensor @maribu ! It's already much better than before.

I think the shell commands could be kept and I have some comments to improve the module organisation (using pseudomodules) and also regarding the way the sensor is initialized/configured.

aabadie · 2018-06-09T16:51:38Z

drivers/Makefile.dep

@@ -302,7 +302,7 @@ ifneq (,$(filter servo,$(USEMODULE)))
  FEATURES_REQUIRED += periph_pwm
 endif

-ifneq (,$(filter sht11,$(USEMODULE)))
+ifneq (,$(filter sht1x,$(USEMODULE)))


it's better to use pseudomodule for specific version of the sensor (sht10, sht11, sht15) and to load the generic module when the specific one is loaded.
You can declare the pseudomodules in RIOTBASE/makefiles/pseudomodules.inc.mk (see sx1272, sx1276 as example) and here use the following makefile rule:

ifneq (,$(filter sht1%,$(USEMODULE))) USEMODULE += sht1x USEMODULE += xtimer endif

aabadie · 2018-06-09T16:52:33Z

doc/doxygen/src/creating-an-application.md

 your Makefile needs to contain these lines:

 ~~~~~~~~~~~~~~~~~~~~~~~~~~~ {.mk}
-USEMODULE += sht11
+USEMODULE += sht1x


use the pseudomodule related to the concrete version of the sensor (sht10,11,15)

aabadie · 2018-06-09T16:53:25Z

drivers/include/sht1x.h

+ */
+
+/**
+ * @defgroup    drivers_sht1x SHT10/SHT1X/SHT15 Humidity and Temperature Sensors


Should be SHT10/SHT11/SHT15

aabadie · 2018-06-09T16:53:53Z

drivers/include/sht1x.h

+/**
+ * @defgroup    drivers_sht1x SHT10/SHT1X/SHT15 Humidity and Temperature Sensors
+ * @ingroup     drivers_sensors
+ * @brief       Driver for Sensirion SHT10/SHT1/SHT15 Humidity and Temperature


typo on SHT11

aabadie · 2018-06-09T16:54:22Z

drivers/include/sht1x.h

+ * @file
+ * @brief       SHT1X Device Driver
+ *
+ * @author      Freie Universität Berlin, Computer Systems & Telematics


The author is invalid, maybe it was already like this ?

Yes, I could remove the author tag and keep them in the copyright declaration. The author tag does not provide additional information compared to the copyright declaration.

aabadie · 2018-06-09T17:05:25Z

sys/auto_init/saul/auto_init_sht1x.c

+        saul_entries[(i * 3) + 2].dev = &(sht1x_devs[i]);
+        saul_entries[(i * 3)    ].name = "SHT1X temperature";
+        saul_entries[(i * 3) + 1].name = "SHT1X humidity";
+        saul_entries[(i * 3) + 2].name = "SHT1X config (Low resolution | "


I think the calibration should be passed in the driver params.

This was not actually calibrating the sensor - the calibration data is stored inside the sensor. But the sensor can be configured to not use them, which speeds up measurements about 10 ms. So this should be a runtime option, as this trade-off between speed and precision depends on the situation.

But the configuration was removed from SAUL anyway (as suggested), so that comment no longer applies.

aabadie · 2018-06-09T17:06:01Z

sys/shell/commands/Makefile

@@ -8,9 +8,6 @@ endif
 ifneq (,$(filter ps,$(USEMODULE)))
  SRC += sc_ps.c
 endif
-ifneq (,$(filter sht11,$(USEMODULE)))


I would keep the shell commands

aabadie · 2018-06-09T17:07:35Z

tests/driver_sht1x/Makefile

+BOARD ?= native
+include ../Makefile.tests_common
+
+USEMODULE += sht1x


Same comment about the version of sensor, better use the pseudomodule which will implicitly load the common concrete module.

Sure the device specific pseudo modules should be used for unit tests that are not device specific? I would prefer to keep sht1x here, otherwise it is not obvious that is applies to all variants.

Sure the device specific pseudo modules should be used for unit tests that are not device specific?

Ok, I initially missed that subtlety. If you want to test conversion functions in a unit test way then I would recommend that you add this in a real unittest under the unittests application.
The driver_sht1x should be a helper application for testing real hardware. Instead of native, the default board could msba2 or msp430 since they embed one of this sensor. This application could either call the driver initialization, use a default configuration and then periodically read the sensor values or, just use the shell commands to give full control on the device.

aabadie · 2018-06-09T17:09:29Z

tests/driver_sht1x/main.c

+                    continue;
+                }
+                int16_t got_hum = sht1x_humidity(&dev, raw_hum, temp);
+                if (


This file needs to be uncrustified

aabadie · 2018-06-09T17:12:17Z

tests/driver_sht1x/main.c

+    const char *res_temp[] = {"12", "14"};
+    const char *res_hum[] = {"8", "12"};
+    const char *voltages[] = {"5.0", "4.0", "3.5", "3.0", "2.5"};
+    sht1x_dev_t dev = {.conf = 0};


Why not use the sht1x_init with the default params ? This is the common usage for other sensors.

Because this would require an actual sensor to be present, but the unit tests are intended to run on "native" without any hardware. The unit tests verify that the integer arithmetic still gives the same results as the floating point arithmetic given in the data sheet. Because these tests are computationally expensive, it makes no sense to run them on embedded hardware.

Also, using default parameters would dramatically decrease test coverage. The unit tests iterate over all possible configuration states in the for loops and verify that the calculations are correct for all of them - including the configuration that was chosen to be the default one.

maribu · 2018-06-10T19:58:40Z

@aabadie: Thanks for your review. Unless I have overlooked anything, I have addressed all issues you pointed out with the exception of the issues for the unit tests I commented on.

Please review again and give feedback if the two points with the unit tests can remain as they are for the reasons I stated in the comments above, or whether I should address them somehow.

Kind regards,
Marian

maribu · 2018-06-10T20:21:59Z

Hmm, coccinelle wants me to cast implicitly from int to int16_t, even though this cast will loose precision on pretty much all >= 32 bit systems. Implicit casts that loose precision are easily overlooked. As casts that loose precision can easily result in bugs, it is widely considered as good coding style to at least make the cast explicit to spotlight the loose in precision. GCC and clang even provide a warning (-Wconversion) to motivate developers to do the conversion explicitly when loosing precision. Maybe coccinelle can be configured similarly?

aabadie

Thanks for addressing the initial comments.
I found other small issues with typos and doxygen. See below.

I also have comments related to the unittests: if you want to test the conversion functions under different parameters and independently of the platform, I think it's better to create a new unittest under the unittests application.
The driver_sht1x application should be testable on real hardware.

The last big comment is about the way you split the auto_init from the saul registry initialization. I think the implementation files could be merged, using conditional macros.

Maybe I missed some subtlety, so forgive me it's the case.

aabadie · 2018-06-11T06:26:59Z

drivers/Makefile.dep

@@ -302,7 +302,8 @@ ifneq (,$(filter servo,$(USEMODULE)))
  FEATURES_REQUIRED += periph_pwm
 endif

-ifneq (,$(filter sht11,$(USEMODULE)))
+ifneq (,$(filter sht1%,$(USEMODULE)))
+  USEMODULE += sht1x


This module also depends on periph_gpio so you should also add here FEATURES_REQUIRED += periph_gpio

aabadie · 2018-06-11T06:28:15Z

drivers/include/sht1x.h

+} sht1x_dev_t;
+
+/**
+ * @brief Parameters requried to set up the SHT1X device driver


typo requried

aabadie · 2018-06-11T06:28:28Z

drivers/include/sht1x.h

+ * @brief Parameters requried to set up the SHT1X device driver
+ */
+typedef struct {
+    gpio_t      clk;  /**< GPIO conntected to the clock pin of the SHT1X */


typo conntected

aabadie · 2018-06-11T06:28:43Z

drivers/include/sht1x.h

+
+/**
+ * @brief             Calculate the temperature from the raw input
+ * @note              This internal funciton is exposed for unit tests


typo funciton

aabadie · 2018-06-11T06:29:12Z

drivers/include/sht1x.h

+ *
+ * @param dev         SHT1X sensor to read
+ * @param temp        Store the measured temperature in E-02 °C here
+ * @param hum         Store the measured relative humidit in E-02 % here


typo humidit

aabadie · 2018-06-11T06:44:15Z

sys/auto_init/auto_init.c

@@ -307,8 +304,8 @@ void auto_init(void)
    auto_init_lis3dh();
 #endif
 #ifdef MODULE_LIS3MDL
-extern void auto_init_lis3mdl(void);
-auto_init_lis3mdl();
+    extern void auto_init_lis3mdl(void);


aabadie · 2018-06-11T06:54:47Z

tests/driver_sht1x/Makefile

+BOARD ?= native
+include ../Makefile.tests_common
+
+USEMODULE += sht1x


Sure the device specific pseudo modules should be used for unit tests that are not device specific?

Ok, I initially missed that subtlety. If you want to test conversion functions in a unit test way then I would recommend that you add this in a real unittest under the unittests application.
The driver_sht1x should be a helper application for testing real hardware. Instead of native, the default board could msba2 or msp430 since they embed one of this sensor. This application could either call the driver initialization, use a default configuration and then periodically read the sensor values or, just use the shell commands to give full control on the device.

aabadie · 2018-06-11T06:57:41Z

sys/shell/commands/sc_sht1x.c

+        case 1:
+            return &sht1x_devs[0];
+        case 2:
+            if ((argv[1][0] >= '0') && (argv[1][0] <= '9') && (!argv[1][1])) {


Just use int dev_idx = atoi(argv[1]);, easier to read.

aabadie · 2018-06-11T06:58:53Z

sys/shell/commands/sc_sht1x.c

+    const sht1x_dev_t *dev = get_dev(argc, argv);
+
+    if (!dev) {
+        return -1;


Maybe add an error message.

The error message is printed by get_dev() :-)

aabadie · 2018-06-11T07:03:02Z

sys/auto_init/saul/auto_init_sht1x_saul.c

+extern const saul_driver_t sht1x_saul_hum_driver;
+/** @} */
+
+void auto_init_sht1x_saul(void)


No need for a saul specific function, you could add it to auto_init_sht1x but guard it with an #ifdef if the saul module is loaded. If you do this, auto_init_sht1x_saul.c can be removed. And the auto_init_sht1x.c could be with the other saul devices auto init files.

maribu · 2018-06-11T20:49:51Z

@aabadie: Thanks for the review and for pointing me to the unit tests app. This is much better now :-)

aabadie

SAUL integration and unittests looks good now. Thanks!

Please address the remaining minor comments (doxygen, typos). It would also be nice to have a test application in tests/driver_sht1x: it could a simple application that load the auto_init, sht1x and shell_commands and start a shell. This way one can play on real hardware using shell commands. What do you think ?

aabadie · 2018-06-12T06:45:17Z

drivers/include/sht1x.h

+ *
+ * @param dev         Device from which the raw value was received
+ * @param raw         The raw (unprocessed) temperature value
+ * @param temp        The temperate at which the humidity was measure in E-02 °C


typo 'temperature'

aabadie · 2018-06-12T06:51:30Z

sys/auto_init/auto_init.c

-#ifdef MODULE_SHT11
-#include "sht11.h"
+#ifdef MODULE_SHT1X
+void auto_init_sht1x(void);


No need to declare this function here. Just use extern before the call to the function.

aabadie · 2018-06-12T06:52:03Z

sys/auto_init/auto_init.c

-#ifdef MODULE_SHT11
-    DEBUG("Auto init SHT11 module.\n");
-    sht11_init();
+#ifdef MODULE_SHT1X


I would move this just before the #ifdef MODULE_AUTO_INIT_SAUL.

maribu · 2018-06-12T13:03:40Z

Unless I have overlooked anything, I have address all open issues and also added an on-device test for the driver. This test will read both temperature and humidity value three times for each configuration and then drop to the shell for manual testing. I added the MSBA2 as default board, because I tested on that. Please note that the driver does only work on the MSBA2 after the gpio driver bugs addressed by this PR are fixed.

aabadie

Thanks for updating again @maribu. It's getting closer.

Found another typo in the new test application.
By the way, some of my previous comments are not addressed.

aabadie · 2018-06-12T16:24:30Z

tests/driver_sht1x/main.c

@@ -0,0 +1,172 @@
+/*
+ * Copyright (C) 2018 2018 Otto-von-Guericke-Universität Magdeburg


2018 appears twice

maribu · 2018-06-12T19:06:31Z

In case you wonder why I put thumbs up on every comment: I in fact missed some and would like to tick them with a check mark. As there is no check mark, the thumbs up is what comes nearest to it. So it is just for me to check that I actually applied every comment

maribu · 2018-06-12T20:08:00Z

Done :-)

aabadie

I like. I wanted to ACK but found 2 minor things remaining. See below.

aabadie · 2018-06-12T20:28:31Z

makefiles/pseudomodules.inc.mk

@@ -105,6 +106,10 @@ PSEUDOMODULES += si7013
 PSEUDOMODULES += si7020
 PSEUDOMODULES += si7021

+# include variants of SX127X drivers as pseudo modules


Ok, I understand. This block should be moved to its original place (before the si1145 pseudo modules). Normally only changes related to sht11/10/15 should appear.

aabadie · 2018-06-12T20:33:09Z

drivers/sht1x/include/sht1x_defines.h

+/** @} */
+
+/**
+ * @name Commands that can be send to the SHT1X driver


s/send/sent/

aabadie · 2018-06-12T20:41:33Z

Just triggered Murdock, maybe some other things will raise.

maribu · 2018-06-13T06:31:41Z

Done. I believe I still have to add some boards to BOARD_INSUFFICIENT_MEMORY for the test, but lets wait for Murdock to complete

aabadie · 2018-06-13T06:50:08Z

Murdock is still failing, I think you have to remove the DIRS = drivers in boards/common/msb-430/Makefile

maribu · 2018-06-13T07:35:05Z

The default pin configuration is invalid on telosb, wsn430-v1_3b, wsn430-v1_4 and z1. How should I handle that?

maribu · 2018-06-13T11:24:27Z

I just changed the way the SAUL info texts are added. The new way is a bit more clever and matches the strategy other drivers use

aabadie · 2018-06-13T11:35:13Z

The default pin configuration is invalid on telosb, wsn430-v1_3b, wsn430-v1_4 and z1. How should I handle that?

No idea, this is related to 16bit platform but the error is weird. Maybe @cladmi has an idea ?

maribu · 2018-06-13T16:58:12Z

Hi,

cpu/msp430fxyz/include/periph_cpu.h contains:

typedef uint16_t gpio_t;
[...]
#define GPIO_PIN(x, y)      ((gpio_t)(((x & 0xff) << 8) | (1 << (y & 0xff))))

so for y > 15 the result of (1 << (y & 0xff)) is (as far as I know) not strictly defined, as a << b is only defined for 0 <= b < BITWIDTH(a). Changing that line to

#define GPIO_PIN(x, y)      ((gpio_t)(((x & 0xff) << 8) | (1 << (y & 0x0f))))

would make the result valid again. But maybe it is desired that the compilation fails for invalid pin specifications?

PS: I'm checking that right now with a dummy commit added on top of the PR. So please don't merge until I remove that again.
PPS: Test succeeded. Should the GPIO_PIN macro be changed (in a separate PR)?

maribu · 2018-06-14T11:25:23Z

@cladmi: Would this be a viable solution? If so, should I create an PR for that, or do you want to handle it?

The sensor family SHT10, SHT11 and SHT15 only differ in their accuracy (as in calibration, not as in resolution). Thus, the same driver can be used for all. The new driver name better reflects this fact.

- Use RIOT's GPIO interface to access the sensor to increase portability - Changed API to allow more than one sensor per board - Added `sht1x_params.h` that specifies how the sensors is connected - each board can overwrite default settings by #defining SHT1X_PARAM_CLK and SHT1X_PARAM_DATA - Changed arithmetic to use integer calculations only instead of floating point arithmetic - Added support for checking the CRC sum - Allow optional skipping of the CRC check to speed up measuring - Added support for advanced features like reducing the resolution and skipping calibration to speed up measuring - Allow specifying the supply voltage of sensor which heavily influences the temperature result (and use that information to calculate the correct temperature) - Reset sensor on initialization to bring it in a well known state - Support for the obscure heater feature. (Can be useful to check the temperature sensor?) - Updated old SHT11 shell commands to the new driver interface, thus allowing more than one SHT10/11/15 sensor to be used - Added new shell command to allow full configuration of all attached SHT1x sensors - Removed old command for setting the SHT11 temperature offset, as this feature is implemented in the new configuration command

- Added unit tests for new integer only arithmetic - Temperature conversion differs at most 0.01°C from double arithmetic - Humidity conversions differs at most 0.1% from double arithmetic

maribu · 2018-06-27T07:08:32Z

@aabadie: I just rebased the PR against current master (and removed the testing commit). The issue with compilation on msp430fxyz boards is now fixed :-)

aabadie

Looks good now.

Untested-ACK

maribu mentioned this pull request Jun 8, 2018

drivers/sht11: Added glue for SAUL #9239

Closed

maribu force-pushed the sht1x branch from 598ecf5 to f6e08bb Compare June 8, 2018 22:26

aabadie requested changes Jun 9, 2018

View reviewed changes

maribu force-pushed the sht1x branch 2 times, most recently from 8ba8ff3 to 7685085 Compare June 10, 2018 19:54

maribu force-pushed the sht1x branch from 7685085 to 6bd78d7 Compare June 10, 2018 20:03

aabadie requested changes Jun 11, 2018

View reviewed changes

maribu force-pushed the sht1x branch 3 times, most recently from 166d36b to 22485ee Compare June 11, 2018 20:47

aabadie requested changes Jun 12, 2018

View reviewed changes

maribu force-pushed the sht1x branch 5 times, most recently from 82446a1 to d2289b7 Compare June 12, 2018 12:54

aabadie requested changes Jun 12, 2018

View reviewed changes

maribu force-pushed the sht1x branch from d2289b7 to 7e940e0 Compare June 12, 2018 18:59

maribu force-pushed the sht1x branch from 7e940e0 to f6e5aeb Compare June 12, 2018 19:54

aabadie added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers labels Jun 12, 2018

aabadie reviewed Jun 12, 2018

View reviewed changes

aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 12, 2018

maribu force-pushed the sht1x branch from f6e5aeb to 02a6191 Compare June 13, 2018 06:29

maribu force-pushed the sht1x branch from 02a6191 to 73955c7 Compare June 13, 2018 07:18

maribu force-pushed the sht1x branch from 73955c7 to fd8af8b Compare June 13, 2018 11:14

maribu mentioned this pull request Jun 26, 2018

cpu/msp430fxyz: Fixed input sanitizing in GPIO_PIN #9367

Merged

maribu added 5 commits June 27, 2018 08:57

drivers: Renamed module sht11 to sht1x

d208c22

The sensor family SHT10, SHT11 and SHT15 only differ in their accuracy (as in calibration, not as in resolution). Thus, the same driver can be used for all. The new driver name better reflects this fact.

tests/unittests: Added unit tests for sht1x

2bff79e

- Added unit tests for new integer only arithmetic - Temperature conversion differs at most 0.01°C from double arithmetic - Humidity conversions differs at most 0.1% from double arithmetic

drivers/sht1x: Added SAUL integration

b91359b

tests/driver_sht1x: Added tests for sht1x

8e20067

maribu force-pushed the sht1x branch from 0ab9361 to 8e20067 Compare June 27, 2018 06:58

aabadie approved these changes Jun 27, 2018

View reviewed changes

aabadie merged commit 6e6716a into RIOT-OS:master Jun 27, 2018

maribu deleted the sht1x branch June 27, 2018 08:28

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

aabadie mentioned this pull request Sep 13, 2018

examples/default: remove unnecessary dependency to sht11 #9928

Merged

maribu mentioned this pull request Oct 15, 2018

drivers: sht3x temperature and humidity sensor #10012

Merged

		@@ -0,0 +1,172 @@
		/*
		* Copyright (C) 2018 2018 Otto-von-Guericke-Universität Magdeburg

drivers/sht11: Major refactoring #9317

drivers/sht11: Major refactoring #9317

Conversation

maribu commented Jun 8, 2018 • edited Loading

aabadie left a 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

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

maribu commented Jun 10, 2018

maribu commented Jun 10, 2018

aabadie left a 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

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

maribu commented Jun 11, 2018

aabadie left a 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

maribu commented Jun 12, 2018

aabadie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maribu commented Jun 12, 2018

maribu commented Jun 12, 2018

aabadie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabadie commented Jun 12, 2018

maribu commented Jun 13, 2018

aabadie commented Jun 13, 2018

maribu commented Jun 13, 2018

maribu commented Jun 13, 2018

aabadie commented Jun 13, 2018

maribu commented Jun 13, 2018 • edited Loading

maribu commented Jun 14, 2018 • edited Loading

maribu commented Jun 27, 2018

aabadie left a comment

Choose a reason for hiding this comment

maribu commented Jun 8, 2018 •

edited

Loading

maribu commented Jun 13, 2018 •

edited

Loading

maribu commented Jun 14, 2018 •

edited

Loading