[riot-notifications] [RIOT-OS/RIOT] drivers/lm75: driver for the lm75 sensor and derivatives (#16678)

benpicco notifications at github.com
Fri Jul 23 17:18:54 CEST 2021


@benpicco commented on this pull request.



> @@ -72,6 +72,10 @@ ifneq (,$(filter llcc68,$(USEMODULE)))
   USEMODULE += sx126x
 endif
 
+ifneq (,$(filter tmp1075 lm75%,$(USEMODULES)))
+    USEMODULE += lm75

```suggestion
  USEMODULE += lm75
```

> + * @param[out] dev        device structure to initialize
+ * @param[in] params      initialization parameters
+ *
+ * @return LM75_SUCCESS, on success
+ * @return LM75_ERROR_I2C, on I2C related error
+ * @return LM75_ERROR, on initialization related error
+ */
+int lm75_init(lm75_t *dev, const lm75_params_t *params);
+
+/**
+ * @brief Temperature values of LM75 sensor
+ *
+ * Reads the sensor temperature values from TEMP_REG.
+ *
+ * @param[out] dev                device structure
+ * @param[out] temperature        buffer where temperature value will be written

What unit is the temperature in?

> + * @brief Sets the values for Overtemperature shutdown(OS) and Hysteresis temperature(HYST).
+ * OS gives the temperature's higher bound and HYST the lower bound
+ *

What happens in case of an alarm?

> + * @param[out] temperature        buffer where HYST temperature value will be written
+ *
+ * @return LM75_SUCCESS, on success
+ * @return LM75_ERROR_I2C, on I2C related error
+ */
+int lm75_get_hyst_temp(lm75_t *dev, int *temperature);
+
+/**
+ * @brief Activate the LM75 sensor shutdown mode
+ *
+ * @param[in] dev      device structure to set into shutdown mode
+ *
+ * @return LM75_SUCCESS, on success
+ * @return LM75_ERROR, on mode switching related error
+ */
+int lm75_shutdown(lm75_t *dev);

```suggestion
int lm75_poweroff(lm75_t *dev);
```

for consistency with other drivers

> + * @param[in] dev      device structure to set into shutdown mode
+ *
+ * @return LM75_SUCCESS, on success
+ * @return LM75_ERROR, on mode switching related error
+ */
+int lm75_shutdown(lm75_t *dev);
+
+/**
+ * @brief Dectivate the LM75 sensor shutdown mode
+ *
+ * @param[in] dev      device structure to wake up from shutdown mode
+ *
+ * @return LM75_SUCCESS, on success
+ * @return LM75_ERROR, on mode switching related error
+ */
+int lm75_wake(lm75_t *dev);

```suggestion
int lm75_poweron(lm75_t *dev);
```

> + * @return LM75_SUCCESS, on success
+ * @return LM75_ERROR, on mode switching related error
+ */
+int lm75_wake(lm75_t *dev);
+
+/**
+ * @brief Activates one shot conversion mode
+ *
+ * Wakes from shutdown mode, does a single temperature conversion and goes back into shutdown
+ *
+ * @param[in] dev       device structure
+ *
+ * @return M75_ERROR if unsuccessful
+ * @return M75_SUCCESS if successful
+ */
+int tmp1075_one_shot(lm75_t *dev);

>From the description I would have assumed this returns the temperature as well.
Is there a reason for not using one-shot mode in `lm75_get_temperature()` if no alarm is set? 

> +#define OS_ACTIVE_LOW		0
+#define OS_ACTIVE_HIGH		1

```suggestion
#define OS_ACTIVE_LOW         0
#define OS_ACTIVE_HIGH        1
```

Coding style mandates spaces, not tabs

> +#define I2C_BUS             dev->lm75_params.i2c_bus
+#define I2C_ADDR            dev->lm75_params.i2c_addr
+
+
+int lm75_init(lm75_t *dev, const lm75_params_t *params) {
+
+    dev->lm75_params = *params;
+    uint8_t data;
+    uint8_t config = (params->shutdown_mode) | (params->tm_mode << 1) | (params->polarity << 2) | (params->fault_q << 3);
+
+    if (i2c_acquire(I2C_BUS) != 0) {
+        return LM75_ERROR_I2C;
+    }
+
+    /* read the device ID register of the TMP1075 sensor to see if the sensor truly is a TMP1075 */
+    #if MODULE_TMP1075

```suggestion
    if (IS_USED(MODULE_TMP1075)) {
```

> +         if(i2c_read_regs(I2C_BUS, I2C_ADDR, TMP1075_DEVICE_ID_REG, &deid, 2, 0) != 0) {
+            LOG_INFO("Error reading device ID\n");
+         }
+
+        deid = (uint16_t)htons(deid);
+
+        /* checks if the device ID corresponds to the TMP1075 sensor and extends param confi if so*/
+        if (deid == 0x7500) {
+            puts("Device is a TMP1075");
+     		config |= (params->conv_rate << 5) | (params->one_shot_mode << 7);
+   	    	dev->lm75_params.lm75_res.os_res = 625;           /* resolution in 0.0625ºC */
+   	    	dev->lm75_params.lm75_res.os_mult = 10000;        /* Must multiply by 10000 to get temp in ºC */
+   		    dev->lm75_params.lm75_res.os_bits = 4;            /* Only the 12 most significant bits are needed */
+   		    dev->lm75_params.lm75_res.temp_res = 625;         /* resolution in 0.0625ºC */
+   		    dev->lm75_params.lm75_res.temp_mult = 10000;      /* Must multiply by 10000 to get temp in ºC */
+   		    dev->lm75_params.lm75_res.temp_bits = 4;          /* Only the 12 most significant bits are needed */

We can save some RAM by just keeping a pointer to the struct in ROM

```C
static const lm75_params_t tmp1075_params = {
    .os_res = 625;           /* resolution in 0.0625ºC */
    .os_mult = 10000;        /* Must multiply by 10000 to get temp in ºC */
    .os_bits = 4;            /* Only the 12 most significant bits are needed */
    .temp_res = 625;         /* resolution in 0.0625ºC */
    .temp_mult = 10000;      /* Must multiply by 10000 to get temp in ºC */
    .temp_bits = 4;          /* Only the 12 most significant bits are needed */
};

…

dev->lm75_params = &tmp1075_params;
```

> +            puts("Device ID Register doesnt match");
+            return LM75_ERROR;

Why not allowing mixing lm75a and tmp1075 devices? We can identify them at run-time after all.

> +    return LM75_SUCCESS;
+}
+
+int lm75_get_temperature(lm75_t *dev, int *temperature) {
+
+    uint16_t temp;
+    if (i2c_acquire(I2C_BUS) != 0) {
+        return LM75_ERROR_I2C;
+    }
+    /* read the temperature register */
+    if (i2c_read_regs(I2C_BUS, I2C_ADDR, LM75_TEMP_REG, &temp, 2, 0) != 0) {
+        i2c_release(I2C_BUS);
+        return LM75_ERROR_I2C;
+    }
+    i2c_release(I2C_BUS);
+    *temperature = (int16_t)htons(temp) * dev->lm75_params.lm75_res.temp_res >> dev->lm75_params.lm75_res.temp_bits;

Please add the formula used here as a comment for easier understanding 

> +        return LM75_ERROR_I2C;
+    }
+
+    i2c_release(I2C_BUS);
+    *temperature = (int16_t)htons(temp) * dev->lm75_params.lm75_res.os_res >> dev->lm75_params.lm75_res.os_bits;
+    return LM75_SUCCESS;
+}
+
+int lm75_shutdown(lm75_t *dev) {
+
+    if (dev->lm75_params.shutdown_mode == SHUTDOWN_MODE) {
+        LOG_INFO("device already in shutdown mode\n");
+        return LM75_ERROR;
+    }
+
+    if(i2c_acquire(I2C_BUS) != 0) {

```suggestion
    if (i2c_acquire(I2C_BUS) != 0) {
```
and below 

> +}
+
+/* Performs a single temperature conversion from shutdown mode and goes back into shutdown afterwards */
+int tmp1075_one_shot(lm75_t *dev) {
+
+    if (i2c_acquire(I2C_BUS) != 0) {
+        return LM75_ERROR_I2C;
+    }
+
+    #if !MODULE_TMP1075
+        i2c_release(I2C_BUS);
+        LOG_ERROR("Device incompatible with the one shot conversion function\n");
+        return LM75_ERROR;
+
+    #else
+

shouldn't we check the device type still? 

> @@ -0,0 +1,10 @@
+include ../Makefile.tests_common
+
+DRIVER ?= tmp1075
+
+USEMODULE += $(DRIVER)
+USEMODULE += xtimer
+
+CFLAGS += -DTHREAD_STACKSIZE_MAIN="(3*THREAD_STACKSIZE_LARGE+FLASHPAGE_SIZE)"

Why do we need such a large stack size?
And what has flashpage have to do with this? 

> +	lm75_get_os_temp(dev, &threshold);
+	lm75_get_hyst_temp(dev, &hysteresis);
+    printf("Set OS temp is %d.%dºC\n", threshold / dev->lm75_params.lm75_res.os_mult, threshold % dev->lm75_params.lm75_res.os_mult);
+    printf("Set HYST temp is %d.%dºC\n", hysteresis / dev->lm75_params.lm75_res.os_mult, hysteresis % dev->lm75_params.lm75_res.os_mult);

Please keep the lines aligned :wink: 

> +    uint16_t    os_res;
+    uint8_t     os_bits;
+    uint16_t    os_mult;
+    uint16_t    temp_res;
+    uint8_t     temp_bits;
+    uint16_t    temp_mult;

btw: you can eliminate padding by ordering the struct elements by size, descending

http://www.catb.org/esr/structure-packing/

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16678#pullrequestreview-713800078
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210723/b4100e60/attachment-0001.htm>


More information about the notifications mailing list