drivers/rn2xx3: basic initial implementation (without netdev) by aabadie · Pull Request #7505 · RIOT-OS/RIOT

aabadie · 2017-08-23T15:14:41Z

This PR adds basic support for the LoRaBee device based on Microchip RN2483 or RN2903 LoRa modules.

Those modules provides a full LoRaMAC stack + basic LoRa/FSK radio capacities that can be controlled using an UART interface. This is a bit similar to the XBee except that it doesn't use AT commands. Here the commands are sent in text mode and data using their hexadecimal representation.

For the moment, this driver provides a very simple API to initialize the device and methods to send text commands on the UART and wait for network replies.
There's also a test application that provides 3 shells commands: sys, mac, radio. One can directly use them as described in the command reference document: RN2483, RN2903.

My setup for testing is: Arduino-zero + Arduino wireless shield + LoRaBee. One can also use Nucleo144 boards and maybe other Arduino compatible boards.

Future improvements required (in follow-up PRs):

~~Better integrate into netdev~~
~~Auto init with default parameters (deveui, appeui, appkey, etc)~~
ifconfig integration

Since my module comes from Sodaq, maybe @keestux will be able to test this PR ;)

Depends on ~~#7592~~

miri64 · 2017-08-23T17:40:29Z

Why didn't you use #7084 for this?

aabadie · 2017-08-23T17:44:14Z

Why didn't you use #7084 for this?

because the module is not controlled by AT commands...

aabadie · 2017-08-24T07:40:02Z

@miri64, I had a look at #7084 and maybe I can reuse things (especially isrpipe, expected replies, timeouts, etc) but it's very related to AT commands behavior. Maybe it can be even more generalized ?

miri64 · 2017-08-24T07:44:24Z

Ah okay, I just saw device controlled via UART and thought they were AT commands, so nvm. But looking at the code again in more detail: why are you controlling the device purely via the shell? Wouldn't something similar to the xbee driver be better suited?

miri64 · 2017-08-24T07:45:55Z

(don't get me wrong: as a proof-of-concept this is awesome, but for long-term integration I prefer you to go direct to netdev and not just in future PRs).

aabadie · 2017-08-24T15:56:01Z

why are you controlling the device purely via the shell?

It was faster to implement and smaller changes. I wanted to have other people opinion on it before going further.

Wouldn't something similar to the xbee driver be better suited?

The module behaves differently. I guess I can take inspiration on the state machine, provide an API similar to schedule sx127x for radio level and an API for Mac layer.

miri64 · 2017-08-24T19:15:18Z

The module behaves differently. I guess I can take inspiration on the state machine, provide an API similar to schedule sx127x for radio level and an API for Mac layer.

Well ideally it should implement netdev ;-)

keestux · 2017-08-25T10:45:10Z

drivers/include/lorabee.h

+ * @param[in] cmd           Command to send as string
+ *
+ * @return                  LORABEE_OK on success
+ * @return                  -LORABEE_ERR_INVALID_PARAM if command is invalid


Why not avoid the minus here (and in _parse_response) and make the enum values explicit negative?

because I don' t have to explicitly set a value to all enum values. I don't know what's best I admit.

But then I have to ask, why do you need negative numbers at all? Why not just compare for != OK when you want to know if there is an error (instead of looking for negative numbers)?

keestux · 2017-08-25T10:56:55Z

drivers/lorabee/lorabee.c

+    return 0;
+}
+
+int lorabee_write_cmd(lorabee_t *dev, uint8_t *cmd)


I'd prefer that cmd is a const char*. Every command you send to the RN device is ASCII. No need to pretend it can be binary too. Only where uart_write is being called you need a type cast.

keestux · 2017-08-25T10:59:33Z

drivers/include/lorabee.h

+    /* buffer and synchronization for command responses */
+    mutex_t resp_lock;                         /**< mutex for waiting for command
+                                                *   response */
+    uint8_t resp_buf[LORABEE_MAX_MSG_LENGTH]; /**< command response data buffer */


Why not make this a char[] array. We're only expecting ASCII, don't we?

keestux · 2017-08-25T11:00:10Z

drivers/lorabee/lorabee.c

+        return;
+    }
+    else {
+        dev->resp_buf[dev->resp_size++] = c;


Buffer overflow protection ??

keestux · 2017-08-25T11:01:04Z

drivers/lorabee/lorabee.c

+    }
+
+    if (lorabee_write_cmd(dev, (uint8_t*)"") < 0) {
+        return -1;


Why not pass back the error cause?

aabadie · 2017-08-25T13:54:58Z

@keestux, thanks for having a look. I think I addressed most of your comment. The uint8_t => char conversion was indeed a pain and is now fixed.

aabadie · 2017-08-28T11:11:51Z

@keestux, I updated the PR with a better error management (like you suggested).

You remark regarding the Sodaqone board makes me wonder if it wouldn't be better to call this driver rn2xx3 (for rn2483 and rn2903) instead of lorabee. This is because the microchip module can be used directly on a board and not via a Bee-like pinout. What do you think ?

aabadie · 2017-09-01T14:19:26Z

There are quite a few LoRaBee-like modules on the market, the RN2483/RN2903 is one of them. They are all controller by UART but with different commands. The one here uses a kind of custom ascii commands, the others I saw are controlled with AT commands. These ones will be good candidates for #7084 (here and here).

I renamed the driver in this PR to rn2xx3 since it supports rn2483 and rn2903 modules.

aabadie · 2018-01-09T21:04:25Z

Also I removed the netdev/netopt related part and will open a PR based on this one later.

kaspar030

minors

kaspar030 · 2018-01-09T21:04:26Z

drivers/rn2xx3/rn2xx3.c

+
+#if ENABLE_DEBUG
+/* we are interested by the debug message in the sleep timer callback */
+#define ISR_STACKSIZE               (THREAD_STACKSIZE_DEFAULT)


Setting this here doesn't change the ISR stacksize

what's the best place for this then ?

No good place for this. But here it is useless. :)
Maybe a warning comment before the debug.h include is better?

added a comment explaining how to set it via CFLAGS (untested). So not sure if it works or not.

I tested this morning and modifying the ISR_STACKSIZE has no effect because the debug message is displayed from the idle thread. Modifying THREAD_STACKSIZE_IDLE using CFLAGS works. I updated the comment accordingly.

kaspar030 · 2018-01-09T21:11:39Z

drivers/rn2xx3/rn2xx3_internal.c

+
+#define RESP_TIMEOUT_SEC            (5U)
+
+const char * closing_seq = "\r\n";


kaspar030 · 2018-01-09T21:11:56Z

drivers/rn2xx3/rn2xx3_internal.c

+    };
+}
+
+void rn2xx3_bytes_to_hex(const uint8_t *byte_array, char *hex, uint8_t max_len)


I already read that somewhere ;)

@kaspar030 I opened a PR for fmt: #8343

adapted using the fmt functions.

kaspar030 · 2018-01-09T21:12:34Z

drivers/rn2xx3/rn2xx3_internal.c

+int rn2xx3_cmd_finalize(rn2xx3_t *dev)
+{
+    DEBUG("\n");
+    _uart_write_str(dev, "\r\n");


use closing_seq?

aabadie · 2018-01-12T14:27:52Z

@smlng @kaspar030 do you still have comments ?
@kYc0o can you confirm that it works for you ?

kYc0o · 2018-01-12T15:03:43Z

ACK

smlng · 2018-01-15T08:46:09Z

drivers/include/rn2xx3.h

+/**
+ * @brief   Command responses and server replies status codes
+ */
+enum {


looks better now, but you still mix error codes, replies, and others - I would recommend to order/group them, i.e.,

enum { RN2XX3_OK, RN2XX3_DATA, RN2XX3_TIMEOUT, RN2XX3_ERR_MAC_INIT, /* all other err codes ... */ RN2XX3_REPLY_TX_MAC_OK, /* all other reply codes ... */ }

smlng · 2018-01-15T08:47:09Z

drivers/include/rn2xx3.h

+ * @param[out] dev          RN2XX3 device to initialize
+ * @param[in]  params       parameters for device initialization
+ */
+void rn2xx3_setup(rn2xx3_t *dev, const rn2xx3_params_t *params);


smlng · 2018-01-15T08:51:23Z

drivers/rn2xx3/rn2xx3.c

+
+    /* initialize buffers and locks*/
+    dev->resp_size = 0;
+    dev->cmd_buf[0] = '\0';


nit-picky: this is inconsistent with the rest of the code, where you set dev->cmd_buf[0] = 0;.

smlng · 2018-01-15T08:54:44Z

drivers/include/rn2xx3.h

+#define RN2XX3_FREQ_BAND                (915U)
+#endif
+
+#if defined(MODULE_RN2483)


I would make this an #if #elif #else for RN2XX3_FREQ_BAND, with an #error in the #else branch.

aabadie · 2018-01-15T11:42:05Z

I just addressed your last comments @smlng

smlng · 2018-01-15T13:22:21Z

@aabadie: please squash. @kaspar030 please check if all comments are addressed, so we can get this into the release.

aabadie · 2018-01-15T16:26:29Z

@smlng squashed and rebased

aabadie · 2018-01-15T16:39:34Z

just missing your approval here @kaspar030 :)

aabadie · 2018-01-16T10:27:28Z

ping @kaspar030. After that, it will be straight forward to extend the support of the Sodaq ExpLoRer board with LoRa (see #7733, that needs a small update btw)

aabadie · 2018-01-16T16:47:58Z

just wanted to raise this one again, it's ready to be merged.

smlng · 2018-01-16T17:01:36Z

it's still on the list for the release, I'll look into it tomorrow but don't have the hardware, to no testing.

smlng · 2018-01-16T19:51:55Z

@kaspar030 please check if your comments are sufficiently addressed, thx!

kaspar030

some more things.

Please ignore my doxygen indentation comments, while the indentation is a bit off-standard, at least it is consistent.

kaspar030 · 2018-01-16T22:08:39Z

drivers/include/rn2xx3.h

+/**
+ * @brief   RN2483/RN2903 device descriptor
+ */
+typedef struct {


This struct is massive... And it wastes quite some memory on bad ordering. But I'm ok with improving later.

kaspar030 · 2018-01-16T22:09:05Z

drivers/include/rn2xx3.h

+ *
+ * @return                  RN2XX3_OK on success
+ * @return                  -ENXIO if UART initialization failed
+ * @return                  RN2XX3_TIMEOUT if UART communcation failed


communication

kaspar030 · 2018-01-16T22:09:49Z

drivers/include/rn2xx3.h

+/**
+ * @brief   Restarts the RN2XX2 device
+ *
+ *          dev->resp_buf contains the module name and version string.


indent off. Maybe "After calling this function, ..."?

kaspar030 · 2018-01-16T22:10:22Z

drivers/include/rn2xx3.h

+ * @brief   Resets the module’s configuration data and user EEPPROM to factory
+ *          default values and restarts the module
+ *
+ *          dev->resp_buf contains the module name and version string.


kaspar030 · 2018-01-16T22:10:44Z

drivers/include/rn2xx3.h

+ * @param[in] dev           RN2XX3 device descriptor
+ *
+ * @return                  RN2XX3_OK on success
+ * @return                  RN2XX3_TIMEOUT if UART communcation failed


here and below again communication ('i' missing)

kaspar030 · 2018-01-16T22:23:57Z

drivers/rn2xx3/rn2xx3_internal.c

+    ret = rn2xx3_process_response(dev);
+    rn2xx3_set_internal_state(dev, RN2XX3_INT_STATE_IDLE);
+
+    DEBUG("RET: %d, RESP: %s\n", ret, dev->resp_buf);


prefix with [rn2xx3]?

kaspar030 · 2018-01-16T22:25:11Z

drivers/rn2xx3/rn2xx3_internal.c

+{
+    char payload_str[2];
+    for (unsigned i = 0; i < payload_len; i++) {
+        fmt_bytes_hex(payload_str, &payload[i], 1);


wouldn't fmt_byte_hex() make more sense here?

kaspar030 · 2018-01-16T22:25:53Z

drivers/rn2xx3/rn2xx3_internal.c

+
+    rn2xx3_set_internal_state(dev, RN2XX3_INT_STATE_IDLE);
+
+    DEBUG("RET: %d, RESP: %s\n", ret, dev->resp_buf);


kaspar030 · 2018-01-16T22:26:17Z

drivers/rn2xx3/rn2xx3_internal.c

+    rn2xx3_cmd_finalize(dev);
+
+    rn2xx3_set_internal_state(dev, RN2XX3_INT_STATE_MAC_TX);
+    uint8_t ret = rn2xx3_process_response(dev);


fold this and the return?

kaspar030 · 2018-01-16T22:27:23Z

tests/driver_rn2xx3/Makefile

@@ -0,0 +1,10 @@
+APPLICATION = driver_rn2xx3


the include below sets this to tests_driver_rn2xx3, so remove?

aabadie · 2018-01-17T08:54:00Z

@kaspar030, sorry for doxygen comments indentation, I prefer this because it's aligned with input/output parameters.

Otherwise, I addressed your comments, and fixed other spaces misplaced around *.

smlng · 2018-01-17T19:57:59Z

ping @kaspar030, lets get this done.

kaspar030 · 2018-01-17T22:20:58Z

sorry for doxygen comments indentation,

The coding conventions say "Line length: aim for no more than 80 characters per line, the absolute maximum should be 100 characters per line."

This PR has 60 lines over 80 characters. More than 50 of that are either @brief lines or have lots of whitespace (some >20) due to your alignment preferences.

I prefer this because it's aligned with input/output parameters.

If this would be "objective readability vs coding conventions", I wouldn't mind. But I think this is more "I prefer this vs. coding conventions".

Also, this PR's doxygen formatting is quite different than in other files.

I think we should go for consistency here...

aabadie · 2018-01-18T08:48:26Z

@kaspar030, I updated the doxygen based on how this is already done in other drivers (Si70xx and At86RF2xx), it should be consistent now. I kept the line length below 100 (widest is 99)

aabadie · 2018-01-18T14:34:33Z

@kaspar030 do you ACK ?

kaspar030

ACK for the formal things (untested).

kaspar030 · 2018-01-18T14:37:03Z

Let's go!

kaspar030 · 2018-01-18T14:40:13Z

Gratulations for getting a quite large PR in! ;)

aabadie · 2018-01-18T14:42:05Z

Many thanks @keestux, @smlng and @kaspar030 for the in-depth review and @kYc0o for your tests and advices!
By the way, now there's a sister PR ready: #7733

(edited: added Sebastian)

aabadie added Area: drivers Area: Device drivers Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Aug 23, 2017

aabadie added this to the Release 2017.10 milestone Aug 23, 2017

aabadie assigned haukepetersen, astralien3000 and kaspar030 Aug 23, 2017

aabadie added the Area: network Area: Networking label Aug 23, 2017

keestux reviewed Aug 25, 2017

View reviewed changes

aabadie force-pushed the driver_lorabee branch from b403b15 to c833ce4 Compare August 25, 2017 13:49

aabadie force-pushed the driver_lorabee branch from e0ff205 to 2b4deae Compare August 28, 2017 11:04

aabadie force-pushed the driver_lorabee branch from ab73376 to f5fa930 Compare September 1, 2017 14:11

aabadie changed the title ~~drivers/lorabee: basic initial implementation~~ drivers/rn2xx3: basic initial implementation Sep 1, 2017

aabadie force-pushed the driver_lorabee branch from f5fa930 to 126b2d4 Compare September 4, 2017 10:59

aabadie mentioned this pull request Sep 4, 2017

cpu/sam0: optimizations to the shared UART driver #7556

Merged

aabadie force-pushed the driver_lorabee branch 2 times, most recently from aec9024 to fd5ff96 Compare September 6, 2017 15:56

kaspar030 requested changes Jan 9, 2018

View reviewed changes

This was referenced Jan 10, 2018

drivers/rn2xx3: add netdev adaption + auto_init #8342

Closed

sys/fmt: add hex/bytes sequence converters #8343

Merged

kYc0o approved these changes Jan 12, 2018

View reviewed changes

smlng suggested changes Jan 15, 2018

View reviewed changes

smlng approved these changes Jan 15, 2018

View reviewed changes

kaspar030 requested changes Jan 16, 2018

View reviewed changes

aabadie added 2 commits January 18, 2018 10:46

drivers: add rn2xx3 LoRa module driver

b63412e

tests/driver_rn2xx3: add test application

5459e19

kaspar030 approved these changes Jan 18, 2018

View reviewed changes

aabadie mentioned this pull request Jan 19, 2018

RFC: LoRaWAN in RIOT #7331

Closed


		#define RESP_TIMEOUT_SEC (5U)

		const char * closing_seq = "\r\n";


		rn2xx3_set_internal_state(dev, RN2XX3_INT_STATE_IDLE);

		DEBUG("RET: %d, RESP: %s\n", ret, dev->resp_buf);

Conversation

aabadie commented Aug 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 commented Aug 23, 2017

Uh oh!

aabadie commented Aug 23, 2017

Uh oh!

aabadie commented Aug 24, 2017

Uh oh!

miri64 commented Aug 24, 2017

Uh oh!

miri64 commented Aug 24, 2017

Uh oh!

aabadie commented Aug 24, 2017

Uh oh!

miri64 commented Aug 24, 2017

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!

aabadie commented Aug 25, 2017

Uh oh!

aabadie commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Sep 1, 2017

Uh oh!

aabadie commented Jan 9, 2018

Uh oh!

kaspar030 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

aabadie commented Jan 12, 2018

Uh oh!

kYc0o commented Jan 12, 2018

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!

aabadie commented Jan 15, 2018

Uh oh!

aabadie commented Aug 23, 2017 •

edited

Loading

aabadie commented Aug 28, 2017 •

edited

Loading