[riot-notifications] [RIOT-OS/RIOT] Driver for INA3221 current and power and bus voltage monitor (#12055)

Marian Buschsieweke notifications at github.com
Fri Sep 13 15:20:31 CEST 2019


maribu commented on this pull request.

Looks pretty good in general and works reliable for me. I have some suggestions to improve further:

- Move `static inline` functions from `drivers/ina3221/ina3221.c` e.g. to `drives/ina3221/include/ina3221_internal.h`. That way you can include that file from the test and move the test code guarded by an `#ifdef ... #endif` from `drivers/ina3221/ina3221.c` to the driver
- Some functions of the user facing API can be made a bit easier to use without increasing ROM size, by adding `static inline` functions as wrapper in front of the implementation. E.g. that could be used to create a bitmask in the static inline function, which is than passed to the implementing function. (See inline comments for more details)
- The use of submodules could make the code a bit cleaner, by moving the code guarded by `#ifdef MODULE_INA3221_WITH_ALERTS` to its own file
- Some style issues need to be fixed
    - No use of CamelCase (e.g. use `int32_t in_uv` or `int32_t in_u_v` instead of `int32_t in_uV`)
    - White space errors (e.g. trailing new line). The CI (travis) will tell you about this, or you can check for this by running `make static-test` in the root of the Repo. But ***beware***: This will rebase your code on top of master. So run e.g. `git checkout -b tmp-for-static-test` to create a new temporary branch that `make static-test` can mess up, and that can be removed later. 
    - Missing spaces e.g. between `if` and `(`. You can use `uncrustify` to address these.

> +
+/**
+ * @brief Convert register value to shunt voltage in uV
+ * 
+ * @param[in]   reg_val Register value
+ * 
+ * @pre         reg_val must be in host byte order
+ * 
+ * @returm      Shunt voltage in uV
+ */
+static inline int32_t reg_val_to_shunt_voltage_uV(int16_t reg_val)
+{
+    assert(reg_val <= INA3221_MAX_SHUNT_REG_VAL);
+
+    return reg_val / 8 * INA3221_SHUNT_VOLTAGE_PRECISION_uV;
+}

I'm personally I fan of strict c style variable names, that is all preprocessor macros and `enum` values in all-upper-case, and all function, variable, parameter and type-names in all-lower-case.

(So `reg_val_to_shunt_voltage_uv` or `reg_val_to_shunt_voltage_u_v` instead of `reg_val_to_shunt_voltage_uV`. And `INA3221_SHUNT_VOLTAGE_PRECISION_UV` or `INA3221_SHUNT_VOLTAGE_PRECISION_U_V` instead of `INA3221_SHUNT_VOLTAGE_PRECISION_uV`.)

More instances below.

> +        return -INA3221_RESET_FAILED;
+    }
+    if(ina3221_set_config(dev, params->config) != INA3221_OK)
+    {
+        return -INA3221_CONFIG_FAILED;
+    }
+    uint16_t cfg;
+    if(ina3221_get_config(dev, &cfg) != INA3221_OK || cfg != params->config)
+    {
+        return -INA3221_CONFIG_FAILED;
+    }
+    memset(dev->alert_callbacks, 0, sizeof(dev->alert_callbacks));
+    memset(dev->alert_callback_arguments, 0, sizeof(dev->alert_callback_arguments));
+
+#ifdef TESTS_DRIVER_INA3221
+    /* Could not make a test case for that because of static functions */

You could move the static inline functions into a header in `drivers/ina3221/include`, e.g. `ina3221_internal.h`. This file could be included in the test. I think this will make the code cleaner to have the test and the driver be strictly separated.

> +        return -ENOTSUP;
+    }
+    dev->alert_callbacks[alert] = cb;
+    dev->alert_callback_arguments[alert] = arg;
+    int check = gpio_init_int(
+        dev->params.alert_pins[alert],
+        GPIO_IN,
+        GPIO_FALLING,
+        cb,
+        arg
+    );
+    return check ? check : INA3221_OK;
+}
+#endif
+
+#ifdef MODULE_INA3221_WITH_ALERTS

You could guard the whole code relevant within a single `#ifdef MODULE_INA3221_WITH_ALERTS`.

Btw., how about using submodules instead, e.g. by changing the Makefile (in `drivers/ina3221`) to:

```Makefile

# enable submodules
SUBMODULES := 1

# Always build base module and SAUL integration (rely on linker to garbage collect SAUL)
SRC := ina3221.c ina3221_saul.c

include $(RIOTBASE)/Makefile.base
```

If you would move the alert specific code into `drivers/ina3221/alerts.c`, you could enable the functions with using `USEMODULE += ina3221_alerts`.

> +
+#ifdef MODULE_INA3221_WITH_ALERTS
+int ina3221_disable_alert(ina3221_t *dev, ina3221_alert_t alert)
+{
+    if(alert >= INA3221_NUM_ALERTS)
+    {
+        return -ERANGE;
+    }
+    if(dev->params.alert_pins[alert] == GPIO_UNDEF)
+    {
+        return -ENOTSUP;
+    }
+    gpio_irq_disable(dev->params.alert_pins[alert]);
+    return INA3221_OK;
+}
+#endif

```C
#endif /* MODULE_INA3221_WITH_ALERTS */
```

> + * @param[in]       ns Number of samples
+ * @param[in]       ctbadc Conversion time for bus voltage ADC
+ * @param[in]       ctsadc Conversion time for shunt voltage ADC
+ * @param[in]       mode Device operation mode
+ * 
+ * @return      Configuration register value
+ */
+inline uint16_t ina3221_make_config(
+    ina3221_enable_ch_t chs,
+    ina3221_num_samples_t ns,
+    ina3221_conv_time_badc_t ctbadc,
+    ina3221_conv_time_sadc_t ctsadc,
+    ina3221_mode_t mode)
+{
+    return (chs | ns | ctbadc | ctsadc | mode);
+}

How about renaming `ina3221_set_config()` to `_ina3221_set_config()` and provide

``` C
static inline int ina3221_set_config(ina3221_t* dev, ina3221_enable_ch_t chs,
                                     ina3221_num_samples_t ns, ina3221_conv_time_badc_t ctbadc,
                                     ina3221_conv_time_sadc_t ctsadc,
                                     ina3221_mode_t mode)
{
    return _ina3221_set_config(dev, chs | ns | ctbadc | ctsadc | mode);
}
```

instead. That way the user facing API would be easier to use, and still the bitmask is generated at compile time.

> + * @brief Create config register value
+ * 
+ * For all enabled channels (1 <= i <= INA3221_NUM_CH), the time to
+ * complete a full measurment cycle is:
+ * 
+ * num_samples * (shunt_conv_time_ch_i + bus_voltage_conv_time_ch_i)
+ * 
+ * @param[in]       chs Channel enable flags
+ * @param[in]       ns Number of samples
+ * @param[in]       ctbadc Conversion time for bus voltage ADC
+ * @param[in]       ctsadc Conversion time for shunt voltage ADC
+ * @param[in]       mode Device operation mode
+ * 
+ * @return      Configuration register value
+ */
+inline uint16_t ina3221_make_config(

Be careful with `inline` without `static`. All modern compiler basically decided on their own whether or not to inline a function, so the keyword `inline` has practically no meaning. It is more like an indication to reviewers that you expect a proper compiler to inline the call.

If the call is not inlined, without `static` a symbol is generated. If two compilation units generate the same symbol, mischief is going to happen. Therefore: Always use `static inline` instead of `inline` to be safe.

> +int ina3221_init(ina3221_t *dev, const ina3221_params_t *params);
+
+#if defined(MODULE_PERIPH_GPIO_IRQ) || defined(DOXYGEN)
+/**
+ * @brief Enable alert callback and argument for alert @p alert
+ *
+ * @param[in,out] dev       Device handle
+ * @param[in] alert         Alert index
+ * @param[in] cb            Alert callback
+ * @param[in] arg           Alert callback argument
+ *
+ * @return INA3221_OK, on success
+ * @return -ERANGE, if @p alert was out of bounds
+ * @return -ENUTSUP, if alert pin was initialized with GPIO_UNDEF
+ */
+int ina3221_enable_alert(ina3221_t *dev, ina3221_alert_t alert, ina3221_alert_cb_t cb, void* arg);

I'd personally split this up into 4 `static inline` functions, one for each alert type. Those functions could internally call `_ina3221_enable_alert(dev, <INDEX>, cb, arg);` to share the same implementation. This way you have a cleaner user facing API, without having to pay an increase in ROM size.

(The idea to only have one implementation is however very nice. I'd expect that having four full blown distinct functions to indeed have about 4x the ROM size.)

> + */
+int ina3221_enable_alert(ina3221_t *dev, ina3221_alert_t alert, ina3221_alert_cb_t cb, void* arg);
+#endif
+
+#if defined(MODULE_PERIPH_GPIO_IRQ) || defined(DOXYGEN)
+/**
+ * @brief Disable alert callback and argument for alert @p alert
+ *
+ * @param[in,out] dev       Device handle
+ * @param[in] alert         Alert index
+ *
+ * @return INA3221_OK, on success
+ * @return -ERANGE, if @p alert was out of bounds
+ * @return -ENUTSUP, if alert pin was initialized with GPIO_UNDEF
+ */
+int ina3221_disable_alert(ina3221_t *dev, ina3221_alert_t alert);

See above.

> + * @return      INA3221_OK
+ */
+int ina3221_get_config(const ina3221_t* dev, uint16_t* cfg);
+
+/**
+ * @brief Enable channels
+ * 
+ * @param[in,out]   dev Device handle
+ * @param[in]       ech Channel enable flags
+ * 
+ * @return      INA3221_OK, on success
+ * @return      -INA3221_I2C_ERROR, if I2C bus acquirement failed
+ * @return      @see i2c_read_regs
+ * @return      @see i2c_write_regs
+ */
+int ina3221_set_enable_channel(ina3221_t *dev, ina3221_enable_ch_t ech);

`enum` types and bitmask can be a pain in the ass. The problem is, that no one knows how big a `enum` type is, and what sign it as. It is only defined that all declared values of an `enum` fit in that type.

How about providing again an internal `int _ina3221_set_channel_states(ina3221_t *dev, uint16_t mask)` and provide an `static inline` function that is more convenient to use, e.g.

```C
typedef enum {
    CHANNEL_DISABLED,
    CHANNEL_ENABLED,
} ina3221_channel_state_t;

static inline int ina3221_set_channel_states(int3221_t *dev, ina3221_channel_state_t channel1, ina3221_channel_state_t channel2, ina3221_channel_state_t channel3)
{
    uint16_t mask = (channel1 ? INA3221_ENABLE_CH1 : 0) | (channel2 ? INA3221_ENABLE_CH2 : 0) | (channel3 ? INA3221_ENABLE_CH3 : 0);
    return _ina3221_set_channel_states(dev, mask);
}
```

That way again the bitmask can be calculated at compile time (if those are compile time constants), but the API is a bit easier to read. (I know that enabled and disabled could be easily translated to `true` and `false`, but when reading the code without having the API documentation at hand, reading `ina3221_set_channel_states(dev, CHANNEL_ENABLED, CHANNEL_ENABLED, CHANNEL_DISABLED)` will be clear to the reader, but `ina3221_set_channel_states(dev, true, true, false)` not.)

> + * @return      INA3221_OK, on success
+ * @return      -INA3221_I2C_ERROR, if I2C bus acquirement failed
+ * @return      @see i2c_read_regs
+ * @return      @see i2c_write_regs
+ */
+int ina3221_set_enable_channel(ina3221_t *dev, ina3221_enable_ch_t ech);
+
+/**
+ * @brief Read which channels are currently enabled
+ * 
+ * @param[in]       dev Device handle
+ * @param[out]      ech Pointer to enabled channels output variable
+ * 
+ * @return      Number of enabled channels
+ */
+int ina3221_get_enable_channel(const ina3221_t *dev, ina3221_enable_ch_t* ech);

I think again this would be easier to use by providing a `static inline` wrapper function that does the decoding of the bitmask. At compile time, the compiler will be able to garbage collect dead code. E.g. if I'm only interested in whether or not channel 2 is active, the compiler will see that the writes to variables where is stored whether channel1 and channel 3 is enabled are never read afterwards, and it will optimized that code out.

> + * @return  INA3221_OK
+ */
+int ina3221_get_num_samples(const ina3221_t *dev, ina3221_num_samples_t* ns);
+
+/**
+ * @brief Update conversion time of bus voltage ADC and write to configuration register
+ * 
+ * @param[in,out]   dev Device handle
+ * @param[in]       ctb Bus voltage conversion time
+ * 
+ * @return      INA3221_OK, on success
+ * @return      -INA3221_I2C_ERROR, if I2C bus acquirement failed
+ * @return      @see i2c_read_regs
+ * @return      @see i2c_write_regs
+ */
+int ina3221_set_conv_time_badc(ina3221_t* dev, ina3221_conv_time_badc_t ctb);

How about `bus_adc` instead of `badc`, as ADC is a pretty common abbreviation known by most readers, but BADC is not.

> + * @return      INA3221_OK
+ */
+int ina3221_get_conv_time_badc(const ina3221_t* dev, ina3221_conv_time_badc_t* ctb);
+
+/**
+ * @brief Update conversion time of shunt voltage ADC and write to configuration register
+ * 
+ * @param[in,out]   dev Device handle
+ * @param[in]       cts Shunt voltage conversion time value
+ * 
+ * @return      INA3221_OK, on success
+ * @return      -INA3221_I2C_ERROR, if I2C bus acquirement failed
+ * @return      @see i2c_read_regs
+ * @return      @see i2c_write_regs
+ */
+int ina3221_set_conv_time_sadc(ina3221_t* dev, ina3221_conv_time_sadc_t cts);

How about `shunt_adc` instead of `sadc`?

> + *
+ * @author      Fabian Hüßler <fabian.huessler at ovgu.de>
+ *
+ * @}
+ */
+
+#ifdef MODULE_INA3221
+
+#include "assert.h"
+#include "log.h"
+#include "saul_reg.h"
+#include "ina3221_params.h"
+#include "ina3221.h"
+
+#define INA3221_NUM           (sizeof(ina3221_params)      \
+                            / sizeof(ina3221_params[0]))

You can use `ARRAY_SIZE()` instead.

> +    for(i = 0; i < num; i++)
+    {
+        /* max 26V bus voltage */
+        /* (2^31)-1 resolution; 2.147483647 Watt in Nanowatt resolutiona */
+        /* 2.147483647 / 26000 = 82595.525 */
+        if(in_uA[i] < (82596 - 500))
+        {
+            out_uW[i] = (in_uA[i] * in_mV[i] + 500) / 1000;
+        }
+        else
+        {
+            out_uW[i] = (in_uA[i] + 500) / 1000 * in_mV[i];
+        }
+    }
+    return i;
+}

The last line should be terminated by exactly one newline. Here the last byte of the file is `}`. Most editors can be configured to add the terminating new line when saving, and most even do so by default.

-- 
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/12055#pullrequestreview-287974864
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190913/0dc70c34/attachment-0001.htm>


More information about the notifications mailing list