[riot-notifications] [RIOT-OS/RIOT] sys/senml: add SenML modules (#16384)

Francisco notifications at github.com
Tue May 18 11:30:27 CEST 2021


@fjmolinas commented on this pull request.

In general, this looks nice. I'm not sure about the `senml_saul` module though. I'm not sure many users will need it since it reads all `saul_reg_t` by default, it seems like something that would be better fitted to be in the examples.

I think what would make more sense is having a:

```c
void senml_saulreg_encode(void *ctx, saul_reg_t *dev);
ssize_t senml_saulregs_encode(coid *ctx, saul_reg_t **dev, uint8_t num);
```

Where the second would be an array of `saul_regs_t`. I think in most cases the user would know how many `sensors` it will encode and therefore a definite array is a better solution, a convenience wrapper could be provided as well, so something like:

```c
ssize_t senml_saulregs_encode_cbor(uint8_t *buf, size_t len, saul_reg_t **dev, uint8_t num)
{
    nanocbor_encoder_t enc;

    nanocbor_encoder_init(&enc, buf, len);
    nanocbor_fmt_array(&enc, num);
    for (uint8_t i = 0; i < num; i++) {
        senml_saulreg_encode(&enc, *dev++);
    }
    return nanocbor_encoded_len(&enc);
}
```


> @@ -125,6 +125,7 @@ PSEUDOMODULES += saul_pwm
 PSEUDOMODULES += scanf_float
 PSEUDOMODULES += sched_cb
 PSEUDOMODULES += semtech_loramac_rx
+PSEUDOMODULES += senml_%

Its better to use explicit modules, it at least keeps a clear list of the modules that are expected or not.

> +        double d;
+        struct { int32_t e; int32_t m; } df /** Decimal fraction */;
+    } value; /**< Value data */
+} senml_numeric_t;
+
+/**
+ * @brief SenML common record attributes.
+ * All of these values are optional: empty or 0 values will not be encoded.
+ */
+typedef struct {
+    const char *base_name;          /**< Base Name */
+    senml_numeric_t base_time;      /**< Base Time */
+    senml_unit_t base_unit;         /**< Base Unit */
+    senml_numeric_t base_value;     /**< Base Value */
+    senml_numeric_t base_sum;       /**< Base Sum */
+    uint64_t base_version;          /**< Base Version */

Does it make sense to have compile-time configuration remove some of the optional fields and save some RAM?, specifically `base_version`, `update_time`, `sum`, which will be used more rarely.

> +/**
+ * @brief SenML data value.
+ */
+typedef struct {
+    senml_attr_t attr;      /**< SenML attributes */
+    const uint8_t *value;   /**< Value */
+    size_t len;             /**< Value length */
+} senml_data_value_t;
+
+/**
+ * @brief Create a floating point numeric value.
+ *
+ * @param v Value to encode.
+ * @return Numeric value containing the given value.
+ */
+inline senml_numeric_t senml_float(float v)

```suggestion
static inline senml_numeric_t senml_float(float v)
```

And for other cases below.

> +/**
+ * @brief SenML data value.
+ */
+typedef struct {
+    senml_attr_t attr;      /**< SenML attributes */
+    const uint8_t *value;   /**< Value */
+    size_t len;             /**< Value length */
+} senml_data_value_t;
+
+/**
+ * @brief Create a floating point numeric value.
+ *
+ * @param v Value to encode.
+ * @return Numeric value containing the given value.
+ */
+inline senml_numeric_t senml_float(float v)

Could we add static initializers as well? Some of the data structures are quite heavy (150bytes) and could be statically allocated to save stack space in many cases.

> +    case SENML_UNIT_BEL:                          return "Bspl";
+    case SENML_UNIT_COUNT:                        return "count";
+    case SENML_UNIT_RATIO:                        return "/";
+    case SENML_UNIT_RATIO_2:                      return "%";
+    case SENML_UNIT_RELATIVE_HUMIDITY_PERCENT:    return "%RH";
+    case SENML_UNIT_REMAINING_BATTERY_PERCENT:    return "%EL";
+    case SENML_UNIT_REMAINING_BATTERY_SECONDS:    return "EL";
+    case SENML_UNIT_RATE:                         return "1/s";
+    case SENML_UNIT_RPM:                          return "1/min";
+    case SENML_UNIT_BEAT_PER_MINUTE:              return "beat/min";
+    case SENML_UNIT_BEATS:                        return "beats";
+    case SENML_UNIT_SIEMENS_PER_METER:            return "S/m";
+
+    case SENML_UNIT_BYTE:                         return "B";
+    case SENML_UNIT_VOLT_AMPERE:                  return "VA";
+    case SENML_UNIT_VOLT_AMPERE_SECOND:           return "VAs";

Can you add this false positive?

-- 
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/16384#pullrequestreview-654541966
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210518/f23a9c12/attachment-0001.htm>


More information about the notifications mailing list