drivers/sx1276: LoRa transceiver phy driver proposal by Cr0s · Pull Request #6002 · RIOT-OS/RIOT

Cr0s · 2016-10-27T00:58:24Z

Hello to RIOT folks!

I'd like to propose this initial implementation of LoRa physical layer driver.

Driver is basically a port of Semtech's LoRaMAC's physical layer implementation.
Driver is working well and tested on 868 MHz band
Driver is able to do LoRa RX and TX

Features that are considered as "done":

What will be done to make this driver complete (by me or someone else who wish and could):

FSK modulation
Make sure that CAD is working properly
Adaptation to the netdev2 interface
Proper time-on-air calculation formula
Polishing the code and documentation to the it's perfect possible form
Chip built-in temperature sensor
More testing/example applications

I appreciate any feedback, fix proposals and related points!

Best regards,
Cr0s

jia200x · 2016-10-28T15:35:57Z

It's working for us. We were able to send and receive data.
We are starting the netdev2 adoption. I will let you know when we make the PR.

Cheers!

OlegHahm · 2016-10-28T15:46:28Z

drivers/include/sx1276.h

@@ -0,0 +1,450 @@
+/*
+ * Copyright (C) 2016 Cr0s


It's usually preferred to provide an e-mail address - either here or in the doxygen @author line.

kaspar030 · 2016-10-28T17:48:49Z

also a real name.

-----Original Message-----
From: Oleg Hahm [email protected]
To: RIOT-OS/RIOT [email protected]
Sent: Fr., 28 Okt. 2016 17:44
Subject: Re: [RIOT-OS/RIOT] drivers/sx1276: LoRa transceiver phy driver proposal (#6002)

OlegHahm commented on this pull request.

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

* Copyright (C) 2016 Cr0s

It's usually preferred to provide an e-mail address - either here or in the doxygen @author line.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
#6002 (review)

fjmolinas · 2016-11-01T21:24:00Z

drivers/sx1276/sx1276.c

+            sx1276_reg_write(dev,
+                             SX1276_REG_DIOMAPPING1,
+                             (sx1276_reg_read(dev, SX1276_REG_DIOMAPPING1)
+                              & SX1276_RF_LORA_DIOMAPPING1_DIO0_MASK) | SX1276_RF_LORA_DIOMAPPING1_DIO0_00);


Not sure why the SX1276_RF_LORA_DIOMAPPING1_DIO0_MASK) | SX1276_RF_LORA_DIOMAPPING1_DIO0_00 are "DIO0", if you are in lora mode, shouldn't both the mask and the mode be for "DIO3"? so to make sure "DIO3" generates de CADDONE ISR. Cheers.

Well spotted. Since CAD mode didn't tested, this obvious copy-paste mistake was not seen before.

Gonna fix it asap.

P.S.: the actual error present in the original Semtech's code:
https://github.com/Lora-net/LoRaMac-node/blob/master/src/radio/sx1276/sx1276.c#L1164
so it was inherited as it is.

OlegHahm · 2016-10-28T15:47:56Z

drivers/include/sx1276.h

+ * Radio driver supported modems.
+ */
+typedef enum {
+    SX1276_MODEM_FSK = 0, SX1276_MODEM_LORA,


Please spit into multiple lines.

OlegHahm · 2016-10-28T15:48:48Z

drivers/include/sx1276.h

+ * Radio driver internal state machine states definition.
+ */
+typedef enum {
+    SX1276_RF_IDLE = 0, SX1276_RF_RX_RUNNING, SX1276_RF_TX_RUNNING, SX1276_RF_CAD,


Multiple lines here again.

OlegHahm · 2016-11-02T09:54:06Z

drivers/include/sx1276.h
Binary file not shown.

Huh?

fjmolinas · 2016-11-02T16:15:06Z

drivers/sx1276/sx1276.c

+    (void) arg;
+}
+
+void *dio_polling_thread(void *arg)


Hi!, I'm not sure why would you choose to use threads over pin interrupts? Doesn't this make packet loss more likely? Plus doesn't this implement a pooling system witch is cpu and energy intensive? Also, is it really needed since you are using stack memory and other resources. Cheers

Hi! I'll try to clarify reasons why I did that.

The main point is to not doing complex stuff inside DIO's ISRs and do it in separate thread instead.

Doesn't this make packet loss more likely?

I think no, the RX handling is pretty straightforward and doesn't involve complex mechanisms such as packet queues which may overflow or something like that.

Plus doesn't this implement a pooling system witch is cpu and energy intensive?

The dio_polling_thread doesn't poll a DIO pins as busy-waiting one. The major part of a time this thread is blocked on awaiting messages that comes from DIO interrupt handlers.

As this thread is awaiting for messages in blocked state, it's not CPU-intensive at all.

Also, is it really needed since you are using stack memory and other resources

This thread is needed since the handler of DIO interrupt might be pretty heavy in matter of stack usage. For example, consider the DIO0 interrupt on RX. After the RX event occurs, if we just call the event handler's callback and If we put some stack-heavy MAC logic into that callback, the ISR stack will definitely be overflowed than if we will do event callback from the separate thread.

The stack space is allocated for this thread only.

Another solution I came up with is to force user to create a thread to handle messages from the driver, but it's seems to me a clear violation of implementation hiding principle and that will lead to more ways to do something wrong.

I hope I made the rationale clear for you.

Cheers.

Hi @Cr0s!

Just read this discussion.
Netdev2 allows a similar flow without the need of another thread on the driver side. For instance, the enc28j60 driver calls netdev->event_callback with NETDEV2_ISR_EVENT and this is translated to a message to the gnrc_netdev2 thread. In the end, the isr function of the driver is called (as shown here).

In this case the dio_polling_thread could be the isr function of the driver (note the isr function is not in ISR context, the name is a bit tricky :P). Probably it would need some flags to identify which interrupt occured.

Hello, @jia200x.

Getting rid of a separate thread could be made as a part of the netdev2 interface adaption.

OlegHahm · 2016-11-03T14:41:53Z

drivers/sx1276/sx1276.c

+
+static inline void send_event(sx1276_t *dev, sx1276_event_type_t event_type)
+{
+    if (dev->sx1276_event_cb != NULL) {


Couldn't we use an assert here instead?

OlegHahm · 2016-11-03T14:42:27Z

drivers/sx1276/sx1276.c

+    spi_release(dev->spi);
+
+    if (res < 0) {
+        printf("sx1276: error initializing SPI_%i device (code %i)\n",


Please change all printfs to DEBUG.

OlegHahm · 2016-11-03T14:42:51Z

drivers/sx1276/sx1276.c

+
+    /* Check presence of SX1276 */
+    if (!sx1276_test(dev)) {
+        DEBUG("init_radio: test failed");


Missing \n.

OlegHahm · 2016-11-03T14:43:12Z

drivers/sx1276/sx1276.c

+    sx1276_set_channel(dev, dev->settings.channel);
+
+    /* Create DIO event lines handler */
+    kernel_pid_t pid = thread_create((char *) dev->_internal.dio_polling_thread_stack, sizeof(dev->_internal.dio_polling_thread_stack), THREAD_PRIORITY_MAIN,


Line is too long.

OlegHahm · 2016-11-03T14:44:38Z

drivers/sx1276/sx1276.c

+    /* Set radio in continuous reception */
+    sx1276_set_op_mode(dev, SX1276_RF_OPMODE_RECEIVER);
+
+    for (i = 0; i < 32; i++) {


Why 32? Please use a macro/constant here.

32 is the number of random bits that we will get from the transceiver and then we will put them into 32 bit integer.

@OlegHahm Any update on this? Should I add macro or leave it as is?

Replaced by macro.

OlegHahm · 2016-11-03T14:46:20Z

drivers/sx1276/sx1276.c

+                     | SX1276_RF_IMAGECAL_IMAGECAL_START);
+
+    while ((sx1276_reg_read(dev, SX1276_REG_IMAGECAL) & SX1276_RF_IMAGECAL_IMAGECAL_RUNNING)
+           == SX1276_RF_IMAGECAL_IMAGECAL_RUNNING) {


@PeterKietzmann, @haukepetersen, how do we handle this in general? Didn't we agree on a failsafe counter for these kind of loops to avoid infinitely trying?

Yes, it's better to replace that with some timeout-based polling to avoid infinite loops in bad cases. I've seen lot of similar loops in RIOT's peripheral drivers. Example.

@OlegHahm Any update on this? Should I leave it as is or add timeout counters?

…to sx1276-radio-driver Forget to pull as always

jia200x · 2017-01-18T19:24:15Z

Hi you all.
Here's the netdev2 adoption. It has been tested with LoRaWAN and works with no known issues.

Cheers
[1]:https://github.com/Inria-Chile/RIOT/tree/sx1276-netdev2/drivers/sx1276

Cr0s · 2017-01-21T13:48:56Z

@emmanuelsearch anything stated as "working" in description is actually working fine, it can be merged at the moment.

aabadie · 2017-01-21T14:36:14Z

I have a couple of Semtech SX1272 devices that should be compatible with SX1276 : they only covers bands from 850MHz to 1GHz and can be configured with 3 bandwidths (500KHz, 250KHz and 125KHz).
I read a bit the PR and the default parameters should be fine with what I have, so I wish I can give a try next week.

aabadie · 2017-03-17T12:59:41Z

@Cr0s any update on your side with this one ?
It seems that this PR is still using the old SPI api. As I said above, I have a couple of ST Nucleo LoRa Kit that I'd like to try with.

aabadie · 2017-03-23T13:47:01Z

ping @Cr0s, are planning to continue your work on this PR ?

PeterKietzmann · 2017-03-27T12:59:16Z

@Cr0s which board and which radio device did you use for testing?

PeterKietzmann · 2017-03-27T13:00:04Z

Do you know if the sx1272 radio is with your sx1276 driver?

aabadie · 2017-03-27T13:07:15Z

@PeterKietzmann, I started to work on adapting this work (mainly #6645) to SX1272 and SX1276. Still work in progress but in a better shape that those ones.

dylad · 2017-03-27T13:12:59Z

@PeterKietzmann I don't think this driver can be used for sx1272 without some work.

PeterKietzmann · 2017-03-27T13:16:05Z

Thanks for both your quick responses! @aabadie where can I find your work, is it already visible for the public?

aabadie · 2017-03-27T13:20:04Z

@PeterKietzmann, yes. here : https://github.com/aabadie/RIOT/tree/driver_sx127x

(Something I did without hardware yesterday) I quickly tested it this morning on sx1272 but it didn't work. I'm wondering if SPI works on Nucleo boards.

dylad · 2017-03-27T13:27:11Z

@aabadie
I'm not sure about the others, but nucleo-l1 SPI works (if you remove the LED pin init at startup)
I have 2 SX1272 and one SX1276. I'll try to test your PR later.

aabadie · 2017-03-27T13:28:21Z

if you remove the LED pin init at startup

is it mandatory ?

PeterKietzmann · 2017-03-27T13:29:12Z

@aabadie do I understand it correctly that your work is based on #6645? @aabadie, @dylad which radio devices (talking about external SPI devices) are you using?

aabadie · 2017-03-27T13:31:43Z

@aabadie do I understand it correctly that your work is based on #6645?

More or less. I have to admit that I had to largely adapt it to fit the coding style of RIOT.

dylad · 2017-03-27T13:33:54Z

Yes, the GPIO definition seems to prevail on the SPI definition.
More information at #6501
As I said I have 2 SX1272 (SX1272RF1 board) and one SX1276 (SX1276MB1MAS board)

aabadie · 2017-03-27T13:41:17Z

@dylad, @PeterKietzmann, I opened a PR with my rework : #6797

drivers/sx127x: rework of implementations from #6645 and #6002

aabadie · 2017-06-30T18:38:13Z

Closing since #6797 is merged.

[sx1276] Initial commit of driver + testing app

330776f

OlegHahm added Area: network Area: Networking Area: drivers Area: Device drivers labels Oct 27, 2016

OlegHahm assigned emmanuelsearch Oct 27, 2016

OlegHahm added this to the Release 2017.01 milestone Oct 27, 2016

Cr0s changed the title ~~[sx1276] LoRa transceiver phy driver proposal~~ drivers/sx1276: LoRa transceiver phy driver proposal Oct 27, 2016

OlegHahm reviewed Oct 28, 2016

View reviewed changes

Cr0s added 6 commits October 28, 2016 22:20

sx1276.h: update header comments

20e49b9

sx1276_regs_fsk.h: updated header comment

57173bf

sx1276.c: updated header comment

e81d6a4

tests/drivers_sx1276/common.h: updated header comment

7cd4cd7

sx1276_regs_lora.h: updated header comment

dfc698d

drivers/sx1276: cleared defines namespace by appending SX1276_ prefix

2977d6f

fjmolinas reviewed Nov 1, 2016

View reviewed changes

sx1276.c: DIO3/CadDone interrupt fix

58c5535

OlegHahm reviewed Nov 2, 2016

View reviewed changes

Cr0s added 2 commits November 2, 2016 17:20

sx1276.h: removed strange symbol from the header comment

2ce250d

sx1276.h: splitted enums on separate lines

3871635

fjmolinas reviewed Nov 2, 2016

View reviewed changes

OlegHahm requested changes Nov 3, 2016

View reviewed changes

Cr0s added 7 commits November 3, 2016 20:26

sx1276.c: added assert() instead if

d26e4f9

sx1276.c: replaced all printfs by DEBUGs

4564de0

sx1276.c: added missing \n to DEBUG`s

2e073aa

sx1276.c: getting rid of too long lines

144884e

sx1276.c: getting rid of single line comments

fedce06

drivers/sx1276: moved RNG modem settings defines outside .c file

0769248

Merge branch 'sx1276-radio-driver' of https://github.com/Cr0s/RIOT in…

76b12ab

…to sx1276-radio-driver Forget to pull as always

PeterKietzmann modified the milestones: Release 2017.01, Release 2017.04 Jan 26, 2017

jia200x mentioned this pull request Feb 23, 2017

[WIP] GNRC LoRaWAN #6645

Closed

5 tasks

aabadie mentioned this pull request Mar 27, 2017

drivers/sx127x: rework of implementations from #6645 and #6002 #6797

Merged

kaspar030 modified the milestones: Release 2017.04, Release 2017.07 Apr 21, 2017

miri64 added a commit that referenced this pull request Jun 30, 2017

Merge pull request #6797 from aabadie/driver_sx127x

d61be01

drivers/sx127x: rework of implementations from #6645 and #6002

aabadie closed this Jun 30, 2017

Conversation

Cr0s commented Oct 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jia200x commented Oct 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaspar030 commented Oct 28, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cr0s Nov 1, 2016 • 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!

OlegHahm commented Nov 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cr0s Nov 2, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Cr0s Nov 3, 2016 • 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!

jia200x commented Jan 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cr0s commented Jan 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aabadie commented Jan 21, 2017

Uh oh!

aabadie commented Mar 17, 2017

Uh oh!

aabadie commented Mar 23, 2017

Uh oh!

PeterKietzmann commented Mar 27, 2017

Uh oh!

PeterKietzmann commented Mar 27, 2017

Uh oh!

aabadie commented Mar 27, 2017

Uh oh!

dylad commented Mar 27, 2017

Uh oh!

PeterKietzmann commented Mar 27, 2017

Uh oh!

aabadie commented Mar 27, 2017

Uh oh!

Cr0s commented Oct 27, 2016 •

edited

Loading

Cr0s Nov 1, 2016 •

edited

Loading

Cr0s Nov 2, 2016 •

edited

Loading

Cr0s Nov 3, 2016 •

edited

Loading

jia200x commented Jan 18, 2017 •

edited

Loading

Cr0s commented Jan 21, 2017 •

edited

Loading