[riot-notifications] [RIOT-OS/RIOT] reference implementation of the Sensor Markup Language (SenML) (#5544)

Hauke Petersen notifications at github.com
Tue Dec 19 12:21:44 CET 2017


haukepetersen requested changes on this pull request.



> @@ -0,0 +1,28 @@
+# name of your application
+APPLICATION = senml-json-example
+
+# If no BOARD is found in the environment, use this default:
+BOARD ?= native
+
+# the remote-* boards are blacklisted because RAM is insufficient for the unittests;
+# all other boards are in this list because atof, atoi, etc. are not supported
+BOARD_BLACKLIST := remote-pa remote-reva remote-revb chronos \
+		   msb-430 msb-430h seeeduino_arch-pro telosb \
+		   wsn430-v1_3b wsn430-v1_4 z1

indention is off

> +# If no BOARD is found in the environment, use this default:
+BOARD ?= native
+
+# the remote-* boards are blacklisted because RAM is insufficient for the unittests;
+# all other boards are in this list because atof, atoi, etc. are not supported
+BOARD_BLACKLIST := remote-pa remote-reva remote-revb chronos \
+		   msb-430 msb-430h seeeduino_arch-pro telosb \
+		   wsn430-v1_3b wsn430-v1_4 z1
+
+# This has to be the absolute path to the RIOT base directory:
+RIOTBASE ?= $(CURDIR)/../..
+
+# Comment this out to disable code in RIOT that does safety checking
+# which is not needed in a production environment but helps in the
+# development process:
+CFLAGS +=

I guess its safe to remove this complete block?!

> +    printf("Base Unit:\t%s\n", base_info->base_unit ? base_info->base_unit : "NULL");
+    printf("Base Value:\t%f\n", base_info->base_value);
+    printf("\n");
+}
+
+
+static void senml_print_record(const senml_record_t *record)
+{
+    printf("Name:\t\t%s\n", record->name ? record->name : "NULL");
+    printf("Unit:\t\t%s\n", record->unit ? record->unit : "NULL");
+    printf("Link:\t\t%s\n", record->link ? record->link : "NULL");
+    printf("Time:\t\t%f\n", record->time);
+    printf("Update Time:\t%f\n", record->update_time);
+
+    if (record->value_type == SENML_TYPE_FLOAT) {
+        printf("Float Value:\t%f\n", record->value.f);

This will fail on `Cortex-M` platforms (and possible other architectures as well), as the newlib-nano which we are using as default does not support `float` values for `printf()`... I suggest to use `fmt` here.

> +    printf("Base Time:\t%f\n", base_info->base_time);
+    printf("Base Unit:\t%s\n", base_info->base_unit ? base_info->base_unit : "NULL");
+    printf("Base Value:\t%f\n", base_info->base_value);
+    printf("\n");
+}
+
+
+static void senml_print_record(const senml_record_t *record)
+{
+    printf("Name:\t\t%s\n", record->name ? record->name : "NULL");
+    printf("Unit:\t\t%s\n", record->unit ? record->unit : "NULL");
+    printf("Link:\t\t%s\n", record->link ? record->link : "NULL");
+    printf("Time:\t\t%f\n", record->time);
+    printf("Update Time:\t%f\n", record->update_time);
+
+    if (record->value_type == SENML_TYPE_FLOAT) {

coding style: isn't a `switch()` the better choice for this block?

> +    }
+    else if (record->value_type == SENML_TYPE_BOOL) {
+        printf("Boolean Value:\t%s\n", record->value.b ? "true" : "false");
+    }
+    else if (record->value_type == SENML_TYPE_BINARY) {
+        printf("Data Value:\t%s\n", record->value.d);
+    }
+
+    if (record->value_sum != 0) {
+        printf("Value Sum:\t%f\n", record->value_sum);
+    }
+
+    printf("\n");
+}
+
+

here and below: remove additional newline

> +    }
+    else if (record->value_type == SENML_TYPE_BOOL) {
+        printf("Boolean Value:\t%s\n", record->value.b ? "true" : "false");
+    }
+    else if (record->value_type == SENML_TYPE_BINARY) {
+        printf("Data Value:\t%s\n", record->value.d);
+    }
+
+    if (record->value_sum != 0) {
+        printf("Value Sum:\t%f\n", record->value_sum);
+    }
+
+    printf("\n");
+}
+
+

and above as well :-)

> +
+    for (size_t i = 0; i < pack->num; i++) {
+        senml_print_record(&(pack->records[i]));
+    }
+}
+
+
+int main(void)
+{
+    /* decode a SenML document in JSON format */
+
+    char input[] = "[{\"bver\":5, \"bn\":\"urn:dev:mac:0024befffe804ff1\", \"bt\":1468341362,"
+                   "\"n\":\"voltage\", \"u\":\"V\", \"v\":230.05, \"t\":1, \"ut\":1},"
+                   "{\"n\":\"current\", \"u\":\"A\", \"v\":1.65, \"t\":1, \"ut\":1},"
+                   "{\"n\":\"bindump\", \"l\": \"[{\\\"href\\\":\\\"humidity\\\","
+                   "\\\"foo\\\":\\\"bar\\\"}]\", \"vd\":\"U2VuTUwgaXMgc28gY29vbCEK\"}]";

the common practice is to allocate 'larger' blocks of data in the .data RAM and not on the stack...

> +/*
+ * Copyright (C) 2016 Freie Universität Berlin
+ * Copyright (C) 2016 Lennart Dührsen <lennart.duehrsen at fu-berlin.de>
+ *
+ * This file is subject to the terms and conditions of the GNU Lesser
+ * General Public License v2.1. See the file LICENSE in the top level
+ * directory for more details.
+ */
+
+/**
+ * @defgroup sys_senml SenML
+ * @ingroup  sys
+ * @{
+ *
+ * @file
+ * @brief Reference implementation of the Media Types for Sensor Measurement Lists (SenML)

indention after @brief is off 

> +#define SC_BOOL_VALUE    (4)    /**< Key for the boolean value attribute in CBOR documents */
+#define SC_VALUE_SUM     (5)    /**< Key for the value sum attribute in CBOR documents     */
+#define SC_TIME          (6)    /**< Key for the time attribute in CBOR documents          */
+#define SC_UPDATE_TIME   (7)    /**< Key for the update time attribute in CBOR documents   */
+#define SC_DATA_VALUE    (8)    /**< Key for the data value attribute in CBOR documents    */
+#define SC_LINK          (9)    /**< Key for the link attribute in CBOR documents          */
+
+/**
+ * @brief The different data types SenML supports
+ */
+typedef enum {
+    SENML_TYPE_UNDEF  = 0,  /**< Placeholder for when no value is provided */
+    SENML_TYPE_FLOAT  = 1,  /**< Indicates a float value                   */
+    SENML_TYPE_STRING = 2,  /**< Indicates a string value                  */
+    SENML_TYPE_BOOL   = 3,  /**< Indicates a boolean value                 */
+    SENML_TYPE_BINARY = 4   /**< Indicates a binary (data) value           */

do I see it right, that SenML does not support any Integer type, but only float?

> + */
+
+#ifndef SENML_H
+#define SENML_H
+
+#include <stdint.h>
+#include <stdbool.h>
+#include <stddef.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif  /* __cplusplus */
+
+#define SENML_SUPPORTED_VERSION (5)   /**< The highest SenML version this implementation supports */
+
+#define SJ_VERSION       "bver" /**< Key for the version attribute in JSON documents       */

here and all over the file: please check your indention! Indention should be aligned to 4 spaces...

> +    double              base_sum;        /**< If not zero, add this to sum in each record           */
+} senml_base_info_t;
+
+/**
+ * @brief struct that contains the values of a SenML record
+ */
+typedef struct {
+    char              *name;        /**< Sensor's name, will be appended to base name if provided */
+    char              *unit;        /**< Unit of the measurement                                  */
+    char              *link;        /**< Link to additional information (must be escaped!)        */
+    double             time;        /**< Time (seconds since unix epoch) of the measurement       */
+    double             update_time; /**< Time before sensor provides an updated measurement       */
+    double             value_sum;   /**< Integrated sum of the values over time                   */
+    senml_value_type_t value_type;  /**< Indicates which type the value is of                     */
+    union {
+        double            f;  /**< A float value                              */

are you sure you want to use `double` (64-bit long!) for handling floating point numbers? This seems to be very bad for contrained devices in multiple ways:
- a lot o memory is needed to store them
- even most HW floating point units do not support them (32-bit float only)

> +#define SC_BASE_UNIT     (-4)   /**< Key for the base unit attribute in CBOR documents     */
+#define SC_BASE_VALUE    (-5)   /**< Key for the base value attribute in CBOR documents    */
+#define SC_BASE_SUM      (-6)   /**< Key for the base sum attribute in CBOR documents      */
+#define SC_NAME          (0)    /**< Key for the name attribute in CBOR documents          */
+#define SC_UNIT          (1)    /**< Key for the unit attribute in CBOR documents          */
+#define SC_VALUE         (2)    /**< Key for the value attribute in CBOR documents         */
+#define SC_STRING_VALUE  (3)    /**< Key for the string value attribute in CBOR documents  */
+#define SC_BOOL_VALUE    (4)    /**< Key for the boolean value attribute in CBOR documents */
+#define SC_VALUE_SUM     (5)    /**< Key for the value sum attribute in CBOR documents     */
+#define SC_TIME          (6)    /**< Key for the time attribute in CBOR documents          */
+#define SC_UPDATE_TIME   (7)    /**< Key for the update time attribute in CBOR documents   */
+#define SC_DATA_VALUE    (8)    /**< Key for the data value attribute in CBOR documents    */
+#define SC_LINK          (9)    /**< Key for the link attribute in CBOR documents          */
+
+/**
+ * @brief The different data types SenML supports

Check for this file: sometime you start the comments with a capital letter, sometimes with small letter.

You might also want to put 3 space chars after the @brief (tab alignment) - this is the default in RIOT...

> + * directory for more details.
+ */
+
+/**
+ * @author Lennart Dührsen <lennart.duehrsen at fu-berlin.de>
+ */
+
+#include <stdlib.h>
+#include <string.h>
+#include <stdbool.h>
+#include <ctype.h>
+#include <stdio.h>
+
+#include "senml.h"
+#include "jsmn.h"
+#include "cbor.h"

nothing from this file is currently used in the implementation, right? So just drop the include for now.

> + *                   records and base info will be stored.
+ *
+ * @return 0 on success, -1 if @p input is not a valid SenML document, or a
+ *         positive number indicating by how much the number of records in
+ *         @p input exceeds `pack->num`
+ */
+int senml_decode_json_s(char *input, senml_pack_t *pack);
+
+/**
+ * @brief Creates a SenML document in JSON format from a SenML pack
+ *
+ * @param[in]  pack   The pack that contains the optional base info and the records
+ * @param[out] output The resulting JSON document
+ * @param[in]  len    The length of @p output in bytes
+ *
+ * @return 0 on success, or -1 when @p output is not big enough

I would prefer to add named return values, something like
```c
// somewhere in the top of the file
enum {
    SENML_OK     = 0,
    SENML_OVRFLW = -1,
    ...
};

// here
/**
 * @return  SENML_OK on success
 * @return  SENML_OVRFLW if @p output is not large enough
 */
```
Same goes of course for `senml_decode_json_s()`

> +    double             update_time; /**< Time before sensor provides an updated measurement       */
+    double             value_sum;   /**< Integrated sum of the values over time                   */
+    senml_value_type_t value_type;  /**< Indicates which type the value is of                     */
+    union {
+        double            f;  /**< A float value                              */
+        char             *s;  /**< A string value                             */
+        bool              b;  /**< A boolean value                            */
+        char             *d;  /**< A data value (binary data, base64 encoded) */
+    } value;
+} senml_record_t;
+
+/**
+ * @brief struct that holds a SenML pack (optional base info and 1..n records)
+ */
+typedef struct {
+    senml_base_info_t  *base_info;  /**< Pointer to the base info, may be NULL           */

What is the reason for having `senml_base_info_t` as separate type and not merge it directly into the `senml_pack_t` structure? In the implementation it is used always through the `senml_pack_t` structure anyway... IMHO this would not only simplify the interface, but also save some de-referencing operations...

> +                                "\"v\":0.500000},{\"n\":\"temperature\",\"t\":-10.000000,"
+                                "\"ut\":5.000000,\"v\":0.700000}]";
+
+static char expected_no_baseinfo[]   = "[{\"n\":\"float_measure\",\"u\":\"m\",\"t\":1480778196.431000,"
+                                "\"ut\":10.000000,\"v\":42.195000},{\"n\":\"with_sum\","
+                                "\"u\":\"m\",\"t\":1480778906.541000,\"ut\":10.000000,"
+                                "\"s\":1423576.331000,\"v\":39.190000},{\"n\":\"string_measure\","
+                                "\"t\":1480778388.934000,\"ut\":30.000000,\"vs\":\"hello i am a "
+                                "string value\"},{\"n\":\"bool_measure\",\"t\":1480778500.871000,"
+                                "\"ut\":1.000000,\"vb\":false},{\"n\":\"data_measure\","
+                                "\"l\":\"[{\\\"href\\\":\\\"derp\\\",\\\"foo\\\":"
+                                "\\\"bar\\\"}]\",\"t\":1480778629.230000,\"ut\":120.000000,"
+                                "\"vd\":\"VGhpcyBpcyBiaW5hcnkgdmFsdWUK\"}]";
+
+
+static bool double_almost_equal(float a, float b)

In the senml implementation you use `double`, here `float`. Any reason for this?

-- 
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/5544#pullrequestreview-84398894
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20171219/07ed2b19/attachment-0001.html>


More information about the notifications mailing list