drivers/gsm: add a (generic) gsm driver, with support for UBlox and Quectel. by maxvankessel · Pull Request #10086 · RIOT-OS/RIOT

maxvankessel · 2018-09-30T20:49:51Z

Contribution description

I've extended the PR #8602 "gsm" driver from @vincent-d.
Features of the PR (which incorporates functionality from other PR's):

Generic modem commands, e.g. IMEI, IMSI, SIM PIN & PUK.
GPRS commands, e.g. activating contexts, getting IP addresses of contexts.
Basic call interface, which needs to be extended.
SMS functionality.
Basic API for modem specific commands, e.g. poweron, poweroff, sleep.
Specific implementation for Quectel and UBlox devices.

Testing procedure

Example is included in the example directory. It holds numerous shell commands for communicating with the modem.

The example uses the generic gsm module for communicating with the modem. And due to the generic nature, it doesn't know how to power up the modem (which is also specific for different modems). So it has to be powered manually. Specific implementations hold these power up sequences.

Issues/PRs references

#8943

toonst · 2019-02-14T13:56:18Z

drivers/include/gsm/quectel.h

+    bool        invert_status_pin;  /**< select inversion of status pin */
+    gpio_t      reset_pin;          /**< quectel reset pin*/
+    bool        invert_reset_pin;   /**< select inversion of reset pin*/
+    gpio_t      dtr_pin;            /**< quectel dtr pin (modem intput) */


this dtr_pin seems to be very common in modem equipment. I think it makes sense to make it part of the gsm params, similar to the ri_pin. Also, typo in comment: intput.

stale · 2019-08-18T14:15:18Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

miri64 · 2019-08-28T05:23:08Z

@leandrolanzieri could you maybe have a look at this?

leandrolanzieri · 2019-09-10T06:45:05Z

@leandrolanzieri could you maybe have a look at this?

Sure, will do

leandrolanzieri

@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.

EDIT: Also, please rebase before addressing any changes.

leandrolanzieri · 2019-09-17T07:51:58Z

drivers/include/gsm/ublox.h

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


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

leandrolanzieri · 2019-09-17T07:52:26Z

drivers/include/gsm/ublox.h

+extern "C" {
+#endif
+
+typedef enum {


Please add documentation for these enums and structs.

leandrolanzieri · 2019-09-17T07:57:12Z

drivers/include/gsm.h

+/**
+ * @brief   gsm thread stack size
+ */
+#ifndef GSM_THREAD_STACKSIZE


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

leandrolanzieri · 2019-09-17T08:00:05Z

drivers/include/gsm.h

+ * @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.

leandrolanzieri · 2019-09-17T08:14:15Z

drivers/include/gsm.h

+     *
+     * @param[in]   dev     device to act on
+     *
+     * @return 0 for succes


Suggested change

* @return 0 for succes

* @return 0 for success

leandrolanzieri · 2019-09-17T12:19:57Z

drivers/gsm/gsm.c

+        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.

leandrolanzieri · 2019-09-17T12:40:53Z

drivers/gsm/gsm.c

+    return res;
+}
+
+int __attribute__((weak)) gsm_signal_to_rssi(unsigned rssi)


What about moving the specific implementation to the driver?

leandrolanzieri · 2019-09-17T12:41:40Z

drivers/gsm/gsm.c

+
+    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?

leandrolanzieri · 2019-09-17T13:15:34Z

drivers/gsm/gsm.c

+}
+
+
+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.

leandrolanzieri · 2019-09-17T13:21:16Z

drivers/gsm/gsm.c

+
+#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.

stale · 2020-03-20T14:06:29Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

maxvankessel added 5 commits September 30, 2018 22:12

drivers/gsm: added (generic) gsm driver

c582866

examples/gsm: added generic gsm example

fc9ee8c

drivers/gsm: added quectel driver

e96bd57

drivers/gsm: added ublox driver

2f83aa8

drivers/gsm: comply to checks

a1d8fb5

miri64 added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: drivers Area: Device drivers labels Sep 30, 2018

maxvankessel changed the title ~~pr/modem~~ drivers/gsm: add a (generic) gsm driver, with support for UBlox and Quectel. Oct 1, 2018

tcschmidt requested a review from vincent-d October 19, 2018 16:51

miri64 mentioned this pull request Dec 16, 2018

Feature/add_gsm_generic #8943

Closed

toonst reviewed Feb 14, 2019

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 18, 2019

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Aug 28, 2019

leandrolanzieri reviewed Sep 17, 2019

View reviewed changes

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Mar 20, 2020

stale bot closed this Apr 22, 2020

benpicco mentioned this pull request Jun 19, 2020

boards/sodaq-sara-aff: Initial support #10435

Merged

		}


		ssize_t gsm_cmd(gsm_t dev, const char cmd, uint8_t *buf, size_t len, unsigned timeout)


		#endif

		#ifdef GSM_HAS_THREAD

Conversation

maxvankessel commented Sep 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Aug 18, 2019

Uh oh!

miri64 commented Aug 28, 2019

Uh oh!

leandrolanzieri commented Sep 10, 2019

Uh oh!

leandrolanzieri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Mar 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maxvankessel commented Sep 30, 2018 •

edited

Loading

leandrolanzieri left a comment •

edited

Loading