[riot-notifications] [RIOT-OS/RIOT] driver_lis2dh12: functionality extension (#15871)

benpicco notifications at github.com
Thu Jan 28 21:28:11 CET 2021


@benpicco commented on this pull request.

Looking good.

CI has now also added some [style comments](https://github.com/RIOT-OS/RIOT/pull/15871/files).



> +    LIS2DH12_HP_MODE_NORMAL    = 0b00, /**< normal mode, reset by reading REG_REFERENCE */
+    LIS2DH12_HP_MODE_REFERENCE = 0b01, /**< uses the reference signal for filtering */
+    LIS2DH12_HP_MODE_AUTORESET = 0b11, /**< automatically resets on interrupt generation */

```suggestion
    LIS2DH12_HP_MODE_NORMAL    = 0x0, /**< normal mode, reset by reading REG_REFERENCE */
    LIS2DH12_HP_MODE_REFERENCE = 0x1, /**< uses the reference signal for filtering */
    LIS2DH12_HP_MODE_AUTORESET = 0x3, /**< automatically resets on interrupt generation */
```

Binary constants are unfortunately a GCC extension and don't work with Clang. 

> +/**
+ * @brief   ACT_THS definitions
+ */
+typedef union{
+    struct{
+        uint8_t ACTH:7;         /**< Sets the threshold sleep-to-wake or return-to-sleep */
+        uint8_t _RESERVED:1;    /**<  */
+    } bit;                      /**< Structure used for bit access */
+    uint8_t reg;                /**< Type used for register access */
+}LIS2DH12_ACT_THS_t;
+
+/**
+ * @brief   ACT_DURATION definitions
+ */
+typedef union{
+    uint8_t reg; /**< Sleep-to-wake and return-to-sleep duration */

What unit is that in?

> +/**
+ * @brief   Read the FIFO source register
+ *
+ * @param[in] dev       device descriptor
+ * @param[out] data     register values
+ *
+ * @return  LIS2DH12_OK on success
+ */
+int lis2dh12_read_fifo_src(const lis2dh12_t *dev, uint8_t *data);
+
+/**
+ * @brief   Read the FIFO data
+ *
+ * @param[in] dev       device descriptor
+ * @param[out] data     FIFO data
+ * @param[in] number    amount of FIFO data to be read

What happens if there is less data in the fifo than `number`? 

> +
+/**
+ * @brief   Restart the FIFO mode
+ *          this sets the FIFO mode in BYPASS mode and then back to previous mode
+ *
+ * @param[in] dev       device descriptor
+ *
+ * @return  LIS2DH12_OK on success
+ */
+int lis2dh12_restart_fifo(const lis2dh12_t *dev);
+
+/**
+ * @brief   Read the FIFO source register
+ *
+ * @param[in] dev       device descriptor
+ * @param[out] data     register values

What is this data and what would it be used for? 

>   */
-#define LIS2DH12_STATUS_XDA     (0x01)  /**< X-axis new data available */
-#define LIS2DH12_STATUS_YDA     (0x02)  /**< Y-axis new data available */
-#define LIS2DH12_STATUS_ZDA     (0x04)  /**< Z-axis new data available */
-#define LIS2DH12_STATUS_ZYXDA   (0x08)  /**< on X-, Y-, Z-axis new data available */
-#define LIS2DH12_STATUS_XOR     (0x10)  /**< X-axis data overrun */
-#define LIS2DH12_STATUS_YOR     (0x20)  /**< Y-axis data overrun */
-#define LIS2DH12_STATUS_ZOR     (0x40)  /**< Y-axis data overrun */
-#define LIS2DH12_STATUS_ZYXOR   (0x80)  /**< on X-, Y-, Z-axis data overrun */
+typedef struct {
+    lis2dh12_fifo_mode_t FIFO_mode; /**< set FIFO mode */
+    uint8_t FIFO_watermark:5;       /**< set the FIFO watermark level */
+    bool FIFO_set_INT2;             /**< sets the FIFO interrupt to INT2, otherwise INT1 */
+    bool FIFO_enable;               /**< enables the FIFO */

Doesn't `FIFO_mode` being != 0 already imply that the fifo should be enabled? 

> @@ -231,6 +257,47 @@ int lis2dh12_set_int(const lis2dh12_t *dev, const lis2dh12_int_params_t *params,
  * @return  LIS2DH12_NOBUS on bus errors
  */
 int lis2dh12_read_int_src(const lis2dh12_t *dev, uint8_t *data, uint8_t int_line);
+
+/**
+ * @brief   Set the FIFO configuration
+ *
+ * @param[in] dev       device descriptor
+ * @param[in] config    device FIFO configuration
+ *
+ * @return  LIS2DH12_OK on success
+ */
+int lis2dh12_set_fifo(const lis2dh12_t *dev, lis2dh12_fifo_t *config);

Maybe
```suggestion
int lis2dh12_configure_fifo(const lis2dh12_t *dev, lis2dh12_fifo_t *config);
```

> @@ -256,6 +323,85 @@ int lis2dh12_init(lis2dh12_t *dev, const lis2dh12_params_t *params);
  */
 int lis2dh12_read(const lis2dh12_t *dev, int16_t *data);
 
+/**
+ * @brief   Clear the LIS2DH12 memory

What memory is there on the LIS2DH12 other than the FIFO?
Or will this just reset the configuration? 

> +
+/**
+ * @brief   Set the high pass configuration
+ *
+ * @param[in] dev       device descriptor
+ * @param[in] config    device high pass configuration
+ *
+ * @return  LIS2DH12_OK on success
+ */
+int lis2dh12_set_highpass(const lis2dh12_t *dev, lis2dh12_highpass_t *config);
+
+/**
+ * @brief   Set the reference value
+ *
+ * @param[in] dev           device descriptor
+ * @param[in] reference     value to write into device

What kind of value is expected here?

> + * @param[in] dev           device descriptor
+ * @param[in] powermode     change to given power mode
+ *
+ * @return  LIS2DH12_OK on success
+ */
+int lis2dh12_change_powermode(const lis2dh12_t *dev, uint8_t powermode);
+
+/**
+ * @brief   Set the high pass configuration
+ *
+ * @param[in] dev       device descriptor
+ * @param[in] config    device high pass configuration
+ *
+ * @return  LIS2DH12_OK on success
+ */
+int lis2dh12_set_highpass(const lis2dh12_t *dev, lis2dh12_highpass_t *config);

```suggestion
int lis2dh12_set_highpass(const lis2dh12_t *dev, const lis2dh12_highpass_t *config);
```

same for the others where the memory the pointer points to is not modified. 

> + * @param[in] dev       device descriptor
+ * @param[in] config    device click configuration
+ *
+ * @return  LIS2DH12_OK on success
+ */
+int lis2dh12_set_click(const lis2dh12_t *dev, lis2dh12_click_t *config);
+
+/**
+ * @brief   Read click source register
+ *
+ * @param[in] dev       device descriptor
+ * @param[out] data     click source register value
+ *
+ * @return  LIS2DH12_OK on success
+ */
+int lis2dh12_read_click_src(const lis2dh12_t *dev, uint8_t *data);

not 
```suggestion
int lis2dh12_read_click_src(const lis2dh12_t *dev, lis2dh12_click_t *config);
```
?

Or do we get a `LIS2DH12_CLICK_SRC_t` here?

> +    bool INT1_enable;     /**< enables filter for AOI on interrupt 1 */
+    bool INT2_enable;     /**< enables filter for AOI on interrupt 2 */
+    bool DATA_OUT_enable; /**< enables filter for data output */
+} lis2dh12_highpass_t;
+
+/**
+ * @brief   LIS2DH12 click config values
+ */
+typedef struct {
+    bool enable_DOUBLE;     /**< otherwise single click for given axis are enabled */
+    bool enable_X_CLICK;    /**< enable double pr single click for X axes */
+    bool enable_Y_CLICK;    /**< enable double pr single click for Y axes */
+    bool enable_Z_CLICK;    /**< enable double pr single click for Z axes */
+    bool noINT_latency;     /**< if "0" interrupt stays high for TIME_latency setting \
+                                if "1" interrupt stays high until CLICK_SRC is read */
+    uint8_t curr_ODR_Hz;    /**< give current ODR setting (current frequency in Hz) */

Since you are doing `1000 / curr_ODR_Hz` I take this must not exceed 1000 Hz, better document this - or just use an interval time in ms here.

> +    bool DATA_OUT_enable; /**< enables filter for data output */
+} lis2dh12_highpass_t;
+
+/**
+ * @brief   LIS2DH12 click config values
+ */
+typedef struct {
+    bool enable_DOUBLE;     /**< otherwise single click for given axis are enabled */
+    bool enable_X_CLICK;    /**< enable double pr single click for X axes */
+    bool enable_Y_CLICK;    /**< enable double pr single click for Y axes */
+    bool enable_Z_CLICK;    /**< enable double pr single click for Z axes */
+    bool noINT_latency;     /**< if "0" interrupt stays high for TIME_latency setting \
+                                if "1" interrupt stays high until CLICK_SRC is read */
+    uint8_t curr_ODR_Hz;    /**< give current ODR setting (current frequency in Hz) */
+    uint8_t CLICK_thold:7;  /**< set click threshold */
+    uint8_t TIME_limit:7;   /**< set time limit over threshold value */

What unit is the time in?

> +    _write(dev, REG_CTRL_REG5, reg5.reg);
+    _write(dev, REG_FIFO_CTRL_REG, fifo_reg.reg);
+    _release(dev);
+
+    return LIS2DH12_OK;
+}
+
+int lis2dh12_restart_fifo(const lis2dh12_t *dev) {
+
+    assert(dev);
+
+    _acquire(dev);
+    uint8_t reg5 = _read(dev, REG_CTRL_REG5);
+    LIS2DH12_FIFO_CTRL_REG_t fifo_reg = {0};
+    fifo_reg.reg = _read(dev, REG_FIFO_CTRL_REG);
+    _release(dev);

Just keep the device acquired till the end of the function, multiple acquire / release cycles are needless expensive. (e.g. SPI bus is powered down on release) 

> +
+int lis2dh12_restart_fifo(const lis2dh12_t *dev) {
+
+    assert(dev);
+
+    _acquire(dev);
+    uint8_t reg5 = _read(dev, REG_CTRL_REG5);
+    LIS2DH12_FIFO_CTRL_REG_t fifo_reg = {0};
+    fifo_reg.reg = _read(dev, REG_FIFO_CTRL_REG);
+    _release(dev);
+
+    uint8_t fifo_mode_old = fifo_reg.bit.FM;
+    fifo_reg.bit.FM = LIS2DH12_FIFO_MODE_BYPASS;
+
+    _acquire(dev);
+    // switch to Bypass mode

```suggestion
    /* switch to Bypass mode */
```

C style comments according to [Coding Conventions
](https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#comments)

> +int lis2dh12_read_fifo_src(const lis2dh12_t *dev, uint8_t *data) {
+
+    assert(dev && data);
+
+    _acquire(dev);
+    *data = _read(dev, REG_FIFO_SRC_REG);
+    _release(dev);
+
+    return LIS2DH12_OK;
+}
+
+int lis2dh12_read_fifo_data(const lis2dh12_t *dev, uint8_t *data, uint8_t number) {
+
+    assert(dev && data);
+
+    uint8_t regs[6] = {REG_OUT_X_L, REG_OUT_X_H, REG_OUT_Y_L, REG_OUT_Y_H, REG_OUT_Z_L, REG_OUT_Z_H};

Ah so `data` will actually be something like 

```C
struct {
    uint16_t x;
    uint16_t y;
    uint16_t z;
}
```

can we have a struct for that?

>  #endif /* MODULE_LIS2DH12_INT */
 
+int lis2dh12_clear_memory(const lis2dh12_t *dev) {
+
+    assert(dev);
+
+    LIS2DH12_CTRL_REG5_t ctrl_reg5 = {0};
+    ctrl_reg5.bit.BOOT = 1;

So this actually does a reboot?
Better then use a function name to express that. 

> +    reg1.reg = _read(dev, REG_CTRL_REG1);
+    reg4.reg = _read(dev, REG_CTRL_REG4);

Why read back those two registers?

-- 
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/15871#pullrequestreview-578688961
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210128/986b287c/attachment-0001.htm>


More information about the notifications mailing list