drivers/shtc1: Initial support for the SHTC1 temperature and humidity sensor #7866

shr70 · 2017-10-26T08:53:38Z

This commit adds an implementation for the shtc1 temperature and humidity sensor

Furthermore a test is supplied for the driver

PeterKietzmann

@shr70 thanks for your contribution and the time you already spent! There are a couple of style and conceptual comments in line. Please also consider implementation SAUL for your driver.

drivers/include/shtc1.h

PeterKietzmann · 2017-11-16T09:27:31Z

drivers/include/shtc1.h

+
+
+/**
+ * @brief initializes the sensor and i2c


Start with capital letter here and be consistent with i2c or I2C here and all following occurences

PeterKietzmann · 2017-11-16T09:28:07Z

drivers/include/shtc1.h

+/**
+ * @brief reads out id
+ *
+ * When working correctly id should equal xxxx'xxxx'xx00'0111


Please prefix with @details. This concerns every occasion where you document some details to the function.
Maybe use capital letters fir ID here and following?

PeterKietzmann · 2017-11-16T09:30:11Z

drivers/include/shtc1.h

+ * Where x is unspecified
+ *
+ * @param[in] dev		The I2C Device
+ * @param[out] id		contains the id read from i2c


The ID is specific to the device, not to the I2 bus I assume?

PeterKietzmann · 2017-11-16T09:31:24Z

drivers/include/shtc1.h

+ * @param[out] id		contains the id read from i2c
+ *
+ * @return			0 on everything done
+ * @return			-1 on error occured


on error is enough

PeterKietzmann · 2017-11-16T09:38:04Z

drivers/shtc1/shtc1.c

+
+int8_t shtc1_measure(i2c_t dev, crc_type_t crc, shtc1_values_t* received_values)
+{
+	uint8_t data[] = { 0x7C, 0xA2 };


Please check out the peripheral functions i2c_read/write_reg(s), define register addresses in a separate header file and document what you intend to do here. Same applies for following occurrences.

i2c_read/write_reg() is not possible, as the shtc1 expects a stop condition after the command is send. Thats why I choose to use a i2c_write_bytes(command) and i2c_read_bytes() solution.

PeterKietzmann · 2017-11-16T09:38:55Z

drivers/shtc1/shtc1.c

+int8_t shtc1_measure(i2c_t dev, crc_type_t crc, shtc1_values_t* received_values)
+{
+	uint8_t data[] = { 0x7C, 0xA2 };
+	i2c_write_bytes(dev, 0x70, data, 2);


The device address will be part of the params definition later (compare device driver guide linked above))

PeterKietzmann · 2017-11-16T09:40:51Z

drivers/include/shtc1.h

+ * @brief reads temperature and humidity values
+ *
+ * @param[in] dev		The I2C Device
+ * @param[out] received_values	the received values are going to be saved here


Please document all parameters.

Please document the unit of the returned values

PeterKietzmann · 2017-11-16T09:43:09Z

drivers/shtc1/shtc1.c

+	}
+	received_values->temp = (175.0*temp_f/65536)-45;
+	received_values->rel_humidity = 100*(abs_humidity/65536.0);
+


Usually we abstain from extra divisions and normalization operations to represent a value in persent or something. Do you think we can simplify here?

I believe that a conversion of the temperature to °C is neccessary. The raw values does not hold a lot of information to a user.
The relative humidity can be changed back to the absolute one as this should be clear.

Let's see at the driver guide

On top, devices should work with physical values where ever possible. So e.g. sensors should return already converted values in some physical unit instead of RAW data, so that users can work directly with data from different devices directly without having to care for device specific conversion.

PeterKietzmann · 2017-11-16T09:54:50Z

tests/driver_shtc1/main.c

+    puts("Error: END of main().");
+    /* should be never reached */
+    return 0;
+}


I think we don't need the extra shell command. Just put the actual code directly in main() and periodically read out sensor values.

shr70 · 2017-12-06T16:29:46Z

Remodeled the driver according to driver guide. Now I will add in the requested changes

PeterKietzmann · 2017-12-11T15:43:26Z

@shr70 I've seen activity in this PR. And thanks for your feedback! I will come back t you the next days

PeterKietzmann

@shr70 the code looks way better than before, thanks for your efforts! Please address above comment and rebase. Now the driver needmys to be tested, does anyone own this device?
Furthermore: It is not necessary but how about adding a SAUL adaption?

PeterKietzmann · 2017-12-18T10:54:37Z

@shr70 can you recommend a breakout board or is this device mounted on a board supported by RIOT?

shr70 · 2017-12-18T14:08:09Z

@PeterKietzmann We tested this driver with a predecessor of the Jiminy-Board. The SHTC1 was part of a sensor shield. If you can not get your hands on the SHTC1 we might be able to send you one of our old shields.

PeterKietzmann · 2017-12-19T08:12:25Z

@shr70 that would be pretty helpful! Can you send me an e-mail please?

PeterKietzmann · 2018-07-04T08:06:51Z

@shr70 would you adapt this PR according to #9162? After the I2C remodeling has been done I'd give it an other review

shr70 · 2018-10-09T18:33:18Z

@PeterKietzmann sorry for the late reply, I was busy with my exams. Can you check the driver again? It should now be properly adjusted to the new I2C interface.

PeterKietzmann · 2018-10-19T09:35:37Z

drivers/include/shtc1.h

+typedef struct {
+    i2c_t bus;  /**< I2C bus descriptor */
+    uint8_t addr;   /**< I2C address of the sensor */
+    shtc1_crc_type_t crc;   /**< crc check enabled or disabled (CRC_ENABLED/CRC_DISABLED) */


Align comments

PeterKietzmann · 2018-10-19T09:44:08Z

drivers/include/shtc1.h

+ * @{
+ */
+typedef struct {
+    shtc1_values_t values;  /**< Values struct, where all read data will be stored */


Just for my understanding: Why did you integrate the values struct here? For caching purposes?

PeterKietzmann · 2018-10-19T09:45:42Z

drivers/include/shtc1.h

+ *
+ * @return              SHTC1_OK if a measurement completed. The values will be stored in the values struct. Temperature in °C and humidity in %
+ * @return              SHTC1_ERROR on checksum error
+ */


Please reduce line width of the doc

PeterKietzmann · 2018-10-19T09:46:28Z

drivers/include/shtc1.h

+
+/**
+ * @brief Reads temperature and humidity values
+ * @details The values wil be saved in the device descriptor (values struct). The temperature is in °C and the humidity in %


PeterKietzmann · 2018-10-19T09:53:21Z

drivers/include/shtc1.h

+ * @return              SHTC1_OK if a measurement completed. The values will be stored in the values struct. Temperature in °C and humidity in %
+ * @return              SHTC1_ERROR on checksum error
+ */
+int8_t shtc1_measure(shtc1_t* const dev);


Wouldn't it be nice to have a function that directly returns the data instead of fiddling around with the device struct? Furthermore, I expected some intentions to cache sensor data when you introduced shtc1_values_t. If this was the case, wouldn't you need a distinction between triggering a new read and getting cached data?

shtc1_values_t are used in the device struckt shtc1_t.

Just passing the struckt instead of dev, temp, hum pointer is much more convinient. IMHO.
We could discuss to add raw and processed data to the shtc1_values_t. What do you think.

PeterKietzmann · 2018-10-19T09:54:35Z

drivers/include/shtc1.h

+typedef struct {
+    float temp;         /**< Temperature after an according call to the measurement function */
+    float rel_humidity; /**< Relative humidity after an according call to the measuerment funtion*/
+    unsigned int id;    /**< ID read from the sensor, only available after shtc1_id() was called */


id has 16 bit right? So you better take uint16_t here.

PeterKietzmann · 2018-10-19T09:57:36Z

drivers/shtc1/include/shtc1_regs.h

+#define SHTC1_COMMAND_RESET_LOW                     0x5D
+#define SHTC1_COMMAND_ID_HIGH                       0xEF
+#define SHTC1_COMMAND_ID_LOW                        0xC8
+#define SHTC1_ID                                    0x07    /* ID Mask */


I don't know if we have a convention for that but I expected parentheses around the values. Would you mind to add?

PeterKietzmann · 2018-10-19T10:16:05Z

drivers/shtc1/shtc1.c

+    }
+    /* Receive the measurement */
+    uint8_t received[6];
+    if(i2c_read_bytes(dev->params.bus, dev->params.addr, received, 6, 0)) {


Maybe spend a comment about the format (upper 8bit, lower 8bit, crc)

PeterKietzmann

@shr70 find some comments and questions inline. Generally the code looks good to me. The only open question is about the sensor data in the device struct (aka caching capabilities?). Maybe @aabadie can shed some light into the current "best practices" for drivers.
I just learned that with my Jiminy I need #7943 in order to test the driver. So let's try to get this it done before.

PeterKietzmann · 2018-12-18T19:21:13Z

Still waiting for #7943. And I sill would really much like to return the measurement values via return or pointers instead of via device struct.

Josar · 2018-12-18T22:58:21Z

@PeterKietzmann the current implementation saves the measured values in the dev.values.temp and dev.values.rel_humidity so they can be used multiple times and are stored and valid until a new measurement is done.

I just don't see how this is more fiddling around then passing 3 parameter for the device, temperature and humidity as it is done in sht1x or sht3x drivers.

PeterKietzmann · 2019-01-07T14:18:19Z

@Josar what means "fiddedling around" is a matter of taste, that's for sure. I'm with you that having access to cached sensor data without triggering a measurement is a reasonable feature. However, my primary concern was and still is a similar interface to other sensors drivers.
What do you think about a driver that implements (i) init (ii) trigger measurement (iii) get cached data (iv) convenience function that combines (ii) and (iii)? (iii) and (iv) with parameters for both temperature and humidity.
We came to a similar consensus in #7822.

PeterKietzmann · 2019-01-10T07:47:09Z

@Josar #7822 was merged which allows me to test this one one clarified about the API and rebased.

PeterKietzmann · 2019-01-29T20:17:24Z

@Josar still around?

Josar · 2019-01-30T17:09:30Z

Desk full of work. And waiting for your comments on the Api.

@Josar #7822 was merged which allows me to test this one one clarified about the API and rebased.

PeterKietzmann · 2019-01-31T09:22:43Z

I sill would really much like to return the measurement values via return or pointers instead of via device struct

What do you think about a driver that implements (i) init (ii) trigger measurement (iii) get cached data (iv) convenience function that combines (ii) and (iii)? (iii) and (iv) with parameters for both temperature and humidity.
We came to a similar consensus in #7822.

... is what I proposed. @haukepetersen, @aabadie please state your opinion on the shtc1_measure(shtc1_t* const dev) function and how measured values should be returned so we can finally converge here

keestux · 2019-01-31T20:34:26Z

BTW Notice that this prototype is most likely a programming mistake.

int8_t shtc1_measure(shtc1_t* const dev)

The dev object is not const, only the pointer is. And inside the function that has hardly any value. Except maybe for a compiler to know that you are not going to change the pointer. You can still write struct members.

I'm not sure if that paradigm comes from @Josar or @shr70 but I really think we should change it. I've seen this in sht2x as well.

stale · 2019-08-10T02:07:56Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

benpicco · 2019-12-16T17:22:12Z

superseded by #12924

miri64 requested review from OlegHahm, LudwigKnuepfer and PeterKietzmann October 28, 2017 15:04

miri64 assigned PeterKietzmann Oct 28, 2017

miri64 added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Oct 28, 2017

PeterKietzmann requested changes Nov 16, 2017

View reviewed changes

shr70 mentioned this pull request Nov 28, 2017

drivers/sht21: initial support for the SHT21 sensor #7803

Closed

shr70 force-pushed the shtc1 branch from 551816b to bc18d6b Compare December 6, 2017 16:38

PeterKietzmann requested changes Dec 18, 2017

View reviewed changes

shr70 force-pushed the shtc1 branch 2 times, most recently from cf63cee to f700b22 Compare December 18, 2017 14:01

shr70 force-pushed the shtc1 branch 4 times, most recently from f6700f6 to 5695c13 Compare December 18, 2017 15:12

shr70 force-pushed the shtc1 branch 2 times, most recently from 8addc91 to 443cffe Compare January 9, 2018 14:43

shr70 force-pushed the shtc1 branch from 443cffe to 437694c Compare April 5, 2018 14:48

shr70 added 5 commits October 9, 2018 20:30

drivers/shtc1: Initial support for the shtc1 temp+humidity sensor

3111bbe

tests/driver_shtc1: Initial test case for the SHTC1 sensor

448e2a0

started remodelling of driver interface

8aa22ba

drivers/shtc1: finished remoddeling

aef067f

drivers/shtc1: Adopted requested changes

6a7303e

shr70 force-pushed the shtc1 branch from 437694c to 60f548e Compare October 9, 2018 18:31

PeterKietzmann added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Oct 15, 2018

PeterKietzmann reviewed Oct 19, 2018

View reviewed changes

fixup! drivers/shtc1: Adopted driver to new I2C interface

f04d8f9

Use unsigned int instead of float

090d720

Josar force-pushed the shtc1 branch from cfe0156 to 090d720 Compare December 18, 2018 23:01

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

PeterKietzmann added the State: don't stale State: Tell state-bot to ignore this issue label Aug 19, 2019

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 19, 2019

daexel mentioned this pull request Dec 11, 2019

drivers/shtc1: Initial support for the SHTC1 temperature and humidity sensor #12924

Merged

benpicco closed this Dec 16, 2019

drivers/shtc1: Initial support for the SHTC1 temperature and humidity sensor #7866

drivers/shtc1: Initial support for the SHTC1 temperature and humidity sensor #7866

Conversation

shr70 commented Oct 26, 2017

PeterKietzmann 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

shr70 commented Dec 6, 2017

PeterKietzmann commented Dec 11, 2017

PeterKietzmann left a comment

Choose a reason for hiding this comment

PeterKietzmann commented Dec 18, 2017

shr70 commented Dec 18, 2017

PeterKietzmann commented Dec 19, 2017

PeterKietzmann commented Jul 4, 2018

shr70 commented Oct 9, 2018

PeterKietzmann Oct 19, 2018 • edited Loading

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

Josar Nov 8, 2018 • edited Loading

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

PeterKietzmann left a comment

Choose a reason for hiding this comment

PeterKietzmann commented Dec 18, 2018

Josar commented Dec 18, 2018 • edited Loading

PeterKietzmann commented Jan 7, 2019 • edited Loading

PeterKietzmann commented Jan 10, 2019

PeterKietzmann commented Jan 29, 2019

Josar commented Jan 30, 2019

PeterKietzmann commented Jan 31, 2019

keestux commented Jan 31, 2019

stale bot commented Aug 10, 2019

benpicco commented Dec 16, 2019

PeterKietzmann Oct 19, 2018 •

edited

Loading

Josar Nov 8, 2018 •

edited

Loading

Josar commented Dec 18, 2018 •

edited

Loading

PeterKietzmann commented Jan 7, 2019 •

edited

Loading