[riot-notifications] [RIOT-OS/RIOT] drivers/gsm: add a (generic) gsm driver, with support for UBlox and Quectel. (#10086)

Leandro Lanzieri notifications at github.com
Tue Sep 17 15:27:31 CEST 2019


leandrolanzieri commented on this pull request.

@maxvankessel thanks for this. It's a long PR so I still haven't gone through all the modules, here are some comments mostly on the basic gsm module. More to come ;-) I'm trying to test this on an UBlox module, yet with no success. It would be nice to have someone test this on a Quectel.

> @@ -0,0 +1,97 @@
+/*

Why are the vendor specific headers being added in this commit?

> + */
+
+#ifndef GSM_UBLOX_H
+#define GSM_UBLOX_H
+
+#include <stdint.h>
+#include <stdbool.h>
+
+#include "periph/gpio.h"
+#include "gsm.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+typedef enum {

Please add documentation for these enums and structs.

> +#include "rmutex.h"
+#include "thread.h"
+
+#include "periph/gpio.h"
+#include "periph/uart.h"
+
+#include "at.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief   gsm thread stack size
+ */
+#ifndef GSM_THREAD_STACKSIZE

I guess some of the macros could be added to the compile configuration group (`config`)?

> +#ifndef GSM_UART_BUFSIZE
+#define GSM_UART_BUFSIZE                (128U)
+#endif
+
+/**
+ * @brief   at device default timeout in micro seconds
+ */
+#ifndef GSM_SERIAL_TIMEOUT_US
+#define GSM_SERIAL_TIMEOUT_US           (1000000U)
+#endif
+
+/**
+ * @brief   small line buffer size
+ */
+#ifndef GSM_AT_LINEBUFFER_SIZE_SMALL
+#define GSM_AT_LINEBUFFER_SIZE_SMALL    (32)

Maybe you could spare a comment on what's the use of these two sizes? So the user knows what this config is affecting.

> +    int (*power_off)(gsm_t *dev);
+
+    /**
+     * @brief   Set module to sleep
+     *
+     * @param[in]   dev     device to act on
+     *
+     */
+    void (*sleep)(gsm_t *dev);
+
+    /**
+     * @brief   Wake module
+     *
+     * @param[in]   dev     device to act on
+     *
+     * @return 0 for succes

```suggestion
     * @return 0 for success
```

> +    int (*wake_up)(gsm_t *dev);
+
+    /**
+     * @brief   Reset module
+     *
+     * @param[in]   dev     device to act on
+     *
+     */
+    void (*reset)(gsm_t *dev);
+};
+
+/**
+ * @brief   Initialize gsm module and base module.
+ *
+ * @param[in] dev       Device to initialize
+ * @param[in] params

Please add description for the params

> + *
+ * @return    true for success, otherwise false
+ */
+bool gsm_is_alive(gsm_t *dev, uint8_t retries);
+
+/**
+ * @brief   Set sim puk into modem and pin
+ *
+ * @param[in] dev   Device to write to
+ * @param[in] puk   Puk to set
+ * @param[in] pin   Set new pin
+ *
+ * @return    0 for success
+ * @return  < 0 for failure
+ */
+int gsm_set_puk(gsm_t *dev, const char *puk, const char *pin);

Should be this function called something like `set_new_pin`? Also maybe add to the description that it actually replaces the PIN.

> + *
+ * @return    0 for success
+ * @return  < 0 for failure
+ */
+int gsm_set_puk(gsm_t *dev, const char *puk, const char *pin);
+
+/**
+ * @brief   Set sim pin into modem
+ *
+ * @param[in] dev   Device to write to
+ * @param[in] pin   Pin to set
+ *
+ * @return    0 for success
+ * @return  < 0 for failure
+ */
+int gsm_set_pin(gsm_t *dev, const char *pin);

Do you think something like `gsm_unlock_sim` or similar would describe better the function?

> +        else if (!strcmp(linebuf, "+CPIN: SIM PIN")) {
+            /* sim needs pin */
+            res = 1;
+        }
+        else {
+            LOG_INFO(MODULE"unexpected response: %s\n", linebuf);
+            res = -1;
+        }
+    }
+
+    return res;
+}
+
+int gsm_check_operator(gsm_t *dev)
+{
+    char buf[GSM_AT_LINEBUFFER_SIZE];

You could reuse some logic from `gsm_get_operator` here.

> +    }
+
+    gsm_unlock(dev);
+
+    return res;
+}
+
+ssize_t gsm_get_imsi(gsm_t *dev, char *buf, size_t len)
+{
+    gsm_lock(dev);
+
+    int res = at_send_cmd_get_resp(&dev->at_dev, "AT+CIMI", buf, len,
+            GSM_SERIAL_TIMEOUT_US);
+
+    if (res > 0) {
+        if ((strncmp(buf, "+CME ERROR:", 10) == 0)

Any special reason to check for an error here and not in the IMEI function?

> +        }
+    }
+    else {
+        res = -1;
+    }
+
+    gsm_unlock(dev);
+
+    return res;
+}
+
+ssize_t gsm_get_simcard_identification(gsm_t *dev, char *outbuf, size_t len)
+{
+    int err = -EINVAL;
+
+    if(dev) {

This type of checks should be done with `assert` (you may want to check `outbuf` as well), also there is some inconsistencies as the check is not being done everywhere.

> + * @brief   Gets simcard identification
+ *
+ * @param[in]   dev    Device to write to
+ * @param[out]  outbuf Buffer to store ccid data in
+ * @param[in]   len    Length of buffer
+ *
+ * @return  Length of data written into @p outbuf.
+ * @return  < 0 for failure
+ */
+ssize_t gsm_get_simcard_identification(gsm_t *dev, char *outbuf, size_t len);
+
+/**
+ * @brief   Gets modem identification
+ *
+ * @param[in]  dev  Device to write to
+ * @param[out] buf  Buffer to write identication in

```suggestion
 * @param[out] buf  Buffer to write identification in
```

> +    }
+
+    res = gsm_get_imsi(dev, buf, GSM_AT_LINEBUFFER_SIZE);
+    if (res >= 0) {
+        printf("IMSI: \"%s\"\n", buf);
+    }
+    else {
+        printf("error getting IMSI\n");
+    }
+
+    res = gsm_get_registration(dev, &loc, &cell);
+    if (res > 0) {
+        uint8_t tech = -1;
+        printf("registration code: %d\n", res);
+        printf("location id: %X\n", loc);
+        printf("cell id: %X\n", cell);

This is throwing an error when building for `sodaq-sara-aff` board, better use the macros from inttypes.h

> +        printf("IMSI: \"%s\"\n", buf);
+    }
+    else {
+        printf("error getting IMSI\n");
+    }
+
+    res = gsm_get_registration(dev, &loc, &cell);
+    if (res > 0) {
+        uint8_t tech = -1;
+        printf("registration code: %d\n", res);
+        printf("location id: %X\n", loc);
+        printf("cell id: %X\n", cell);
+
+        res = gsm_get_operator(dev, buf, GSM_AT_LINEBUFFER_SIZE, &tech);
+        if (res >= 0) {
+            if ((char)tech != -1) {

`tech` is unsigned

> +/*
+ * Copyright (C) 2018 OTA keys S.A.
+ *
+ * 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.
+ */
+
+#include <assert.h>
+#include <errno.h>
+
+#include "fmt.h"
+#include "log.h"
+#include "xtimer.h"
+#include "ublox.h"
+

The inclusion of `gsm_internal.h` is missing.

> +    UBLOX_GPIO_MODE_MASTER_CLK,
+    UBLOX_GPIO_MODE_UART,
+    UBLOX_GPIO_MODE_WIFI_EN,
+    UBLOX_GPIO_MODE_RI = 18,
+    UBLOX_GPIO_MODE_LAST_GAP_EN,
+    UBLOX_GPIO_MODE_DISABLED = 255,
+    /* Do not change mode */
+    UBLOX_GPIO_MODE_DEFAULT,
+} ublox_gpio_mode_t;
+
+#define UBLOX_GPIO_OUTPUT_HIGH  (1 << 9)
+#define UBLOX_GPIO_OUTPUT_LOW   (0)
+
+typedef struct ublox_params {
+    gsm_params_t base;                  /**< gsm base parameters */
+    uint32_t    change_over_baudrate;   /**< initial baudrate */

What's the use of having this here as well?

> +/**
+ * @ingroup     drivers_gsm
+ * @{
+ *
+ * @file
+ * @brief       Generic gsm implementation.
+ *
+ * @author
+ *
+ * @}
+ */
+
+#define MODULE  "gsm: "
+
+#ifdef MODULE_AT_URC
+#define GSM_HAS_THREAD

This is already defined in `gsm.h`

> + * @file
+ * @brief       Generic gsm implementation.
+ *
+ * @author
+ *
+ * @}
+ */
+
+#define MODULE  "gsm: "
+
+#ifdef MODULE_AT_URC
+#define GSM_HAS_THREAD
+#endif
+
+#ifdef MODULE_AT_URC
+static void * idle_thread(void *arg);

These two can be inside the same `#ifdef` and depend on `GSM_HAS_THREAD`.

> +            LOG_WARNING(MODULE"failed to initialize at parser with %d\n", err);
+            goto out;
+        }
+
+        if((dev->driver) && (dev->driver->init_base)) {
+            err = dev->driver->init_base(dev);
+
+            if(err) {
+                LOG_WARNING(MODULE"failed to initialize base with %d\n", err);
+                goto out;
+            }
+        }
+
+#ifdef MODULE_PERIPH_GPIO_IRQ
+        if(dev->params->ri_pin != GPIO_UNDEF) {
+            gpio_init_int(dev->params->ri_pin, GPIO_IN, GPIO_FALLING, ring_cb, dev);

Is this always a falling flank?

> + * @file
+ * @brief       Generic gsm implementation.
+ *
+ * @author
+ *
+ * @}
+ */
+
+#define MODULE  "gsm: "
+
+#ifdef MODULE_AT_URC
+#define GSM_HAS_THREAD
+#endif
+
+#ifdef MODULE_AT_URC
+static void * idle_thread(void *arg);

`idle_thread`?

> +                LOG_WARNING(MODULE"failed to power on\n");
+
+                dev->state = GSM_OFF;
+            }
+            else {
+                dev->state = GSM_ON;
+            }
+        }
+
+        if (dev->state == GSM_ON) {
+            at_dev_poweron(&dev->at_dev);
+
+            /* enable network registration unsolicited result code */
+            err = at_send_cmd_wait_ok(&dev->at_dev, "AT+CREG="GSM_URC_CREG, GSM_SERIAL_TIMEOUT_US);
+            if (err < 0) {
+                LOG_INFO(MODULE"failed to enable unsolicited result for CREG\n");

Should unlock and return?

> +        }
+    }
+    return alive;
+}
+
+int gsm_set_puk(gsm_t *dev, const char *puk, const char *pin)
+{
+    int res = -EINVAL;
+
+    if ((strlen(pin) == 4)) {
+        char buf[GSM_AT_LINEBUFFER_SIZE_SMALL];
+        char *pos = buf;
+        bool quotes = false;
+
+        pos += fmt_str(pos, "AT+CPIN=");
+        if (strlen(puk) == 8) {

I think this is not safe. Calling `strlen` with a NULL pointer has an undefined behaviour, and this is being directly called with NULL from `gsm_set_pin`.

> +                while (--pos != buf) {
+                    if (isalnum((uint8_t)*pos)) {
+                        break;
+                    }
+                }
+
+                *(++pos) = '\0';
+                res = pos - buf;
+            }
+        }
+    }
+
+    return res;
+}
+
+int __attribute__((weak)) gsm_signal_to_rssi(unsigned rssi)

What about moving the specific implementation to the driver? 

> +
+int __attribute__((weak)) gsm_signal_to_rssi(unsigned rssi)
+{
+    return (int)rssi;
+}
+
+int gsm_get_signal(gsm_t *dev, int *rssi, unsigned *ber)
+{
+    char buf[32];
+    char *pos = buf;
+
+    gsm_lock(dev);
+
+    int res = at_send_cmd_get_resp(&dev->at_dev, "AT+CSQ", buf, sizeof(buf),
+            GSM_SERIAL_TIMEOUT_US);
+    if ((res > 2) && strncmp(buf, "+CSQ: ", 6) == 0) {

Where does the 2 come from?

> +                    else {
+                        res = -ENOSPC;
+                    }
+                }
+                else {
+                    res = -1;
+                }
+            }
+        }
+    }
+
+    return res;
+}
+
+
+ssize_t gsm_cmd(gsm_t *dev, const char *cmd, uint8_t *buf, size_t len, unsigned timeout)

This function could be reused by others in the module.

> +
+#ifdef MODULE_AT_URC
+void gsm_register_urc_callback(gsm_t *dev, at_urc_t *urc)
+{
+    if(dev) {
+        gsm_lock(dev);
+
+        at_add_urc(&dev->at_dev, urc);
+
+        gsm_unlock(dev);
+    }
+}
+
+#endif
+
+#ifdef GSM_HAS_THREAD

Maybe this macro logic could be simplified as right now there is no use for the thread unless AT_URC is being used.

-- 
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/10086#pullrequestreview-289077474
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190917/4c8f6d88/attachment-0001.htm>


More information about the notifications mailing list