drivers/sx127x: rework of implementations from #6645 and #6002 by aabadie · Pull Request #6797 · RIOT-OS/RIOT

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

This PR is a rework of the initial SX1276 driver implementation from #6002 that was adapted by @jia200x in #6645.

The idea here is to make it ready for merge as fast as possible (targeting the coming release).

While adapting, I added basic support for SX1272, hence the driver name is changed to sx127x. I also kept the netdev adaptation.

I dropped the lorawan adaptation from #6645 because I think this should be provided as an external package (but maybe I'm wrong) and in a follow-up PR.

Note that I open this PR without heavy testing of the code (I don't have sx1276, only sx1272).

This is the hardware I have.

PeterKietzmann

@Cr0s, @jia200x, @aabadie thanks for all your great work. Please answer/address my comments. This is a quite big PR and I think it needs further iterations of review and testing. Until now I did not run tests but I already got my 1272 and 1276 shields.

PeterKietzmann · 2017-04-03T09:54:10Z

drivers/include/sx127x.h

+ * @brief SX127X LoRa configuration
+ * @{
+ */
+#define RF_FREQUENCY                                       (915000000UL)  /**< RF frequency in Hz, 868.9MHz */


Definition and doc not consistent. Maybe we should be able to overwrite this parameter? Especially when looking at the sx1276 which hast two different mudulation frequencies.

PeterKietzmann · 2017-04-03T09:58:02Z

drivers/include/sx127x.h

+#define SX127X_RADIO_WAKEUP_TIME                           (1000U)        /**< [us] */
+#define SX127X_RX_BUFFER_SIZE                              (256)          /**< RX buffer size */
+#define SX127X_RF_MID_BAND_THRESH                          (525000000UL)  /**< Mid-band threshold */
+#define SX127X_CHANNEL_HF                                  (868000000UL)  /**< HF channel */


I'm yet not too familiar with that device. How does this parameter correlate to the RF frequency? Also here we should consider other frequencies/channels (?)

SX127x has some kind of differentiation for so-called High and Low-frequency bands. There is a two different matching network layouts are required for both of the bands and two different outputs for each of them are implemented in a device. So we have to differentiate Low (below 868 MHz) and High modes in software by currently used channel frequency to correctly setup device's registers and use correct output that is dependent on a current channel frequency.

PeterKietzmann · 2017-04-03T11:21:42Z

drivers/include/sx127x.h

+    gpio_t dio3_pin;                                  /**< Interrupt line DIO3 */
+    gpio_t dio4_pin;                                  /**< Interrupt line DIO4 (not used) */
+    gpio_t dio5_pin;                                  /**< Interrupt line DIO5 (not used) */
+} sx127x_params_t;


Could you give a hint what the I/O lines are used for?

PeterKietzmann · 2017-04-03T11:26:59Z

drivers/sx127x/include/sx127x_internal.h

+ * @ingroup     drivers_sx127x
+ * @{
+ * @file
+ * @brief       Semtech SX1276 internal functions


... and following

PeterKietzmann · 2017-04-03T11:31:18Z

drivers/sx127x/include/sx127x_params.h

+
+#ifndef SX127X_PARAM_DIO3
+#define SX127X_PARAM_DIO3                   GPIO_PIN(1, 4)       /* D5 */
+#endif


All needed?

PeterKietzmann · 2017-04-03T12:53:22Z

drivers/sx127x/sx127x_getset.c

+
+uint16_t sx127x_get_preamble_length(sx127x_t *dev)
+{
+    return 0;


Preamble length always 0?

PeterKietzmann · 2017-04-03T12:55:18Z

drivers/sx127x/sx127x_getset.c

+
+void sx127x_set_rx_timeout(sx127x_t *dev, uint32_t timeout)
+{
+    dev->settings.window_timeout = timeout;


Just by the name of the function I would have assumed rx_timeout to be set.

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

sys/include/net/netopt.h

    NETOPT_STATE_RESET,         /**< triggers a hardware reset. The resulting
                                 *   state of the network device is @ref NETOPT_STATE_IDLE */
+    NETOPT_STATE_STANDBY,       /**< standby mode. The devices is awake but
+                                 *   not listening to packets. */


Just out of interest, what do we need this mode for?

I think this is because the SX127x transceivers can be put in sleep mode and not listening to incoming packets. I guess this is one potential reason. @jia200x why did you add this ?

Hi, I'm back.
There's a standby mode in LoRa (is not the same as Sleep, the crystal oscillator is still running but doesn't receive packets). It's part of the RX-SINGLE workflow (after an rx timeout, the SX127X automatically goes back to standby, so shouldn't be ignored).

Cheers

PeterKietzmann · 2017-04-03T13:02:23Z

sys/include/net/netopt.h

+    NETOPT_LORA_MAX_PAYLOAD,                      /**< Maximum payload size */
+    NETOPT_LORA_RANDOM,                           /**< Random value */
+    NETOPT_LORA_TIME_ON_AIR,                      /**< Time on air */
+


I don't have a solution at hand but I'm wondering if we need/want all these options as generic netopts.

PeterKietzmann · 2017-04-03T13:03:46Z

tests/driver_sx127x/main.c

+//        default:
+//            break;
+//    }
+//}


This is not so nice.

aabadie · 2017-04-03T14:02:34Z

@PeterKietzmann thanks for the review, I'll try to address the comments. They all make a lot of sense.

FYI, the test application doesn't work as expected : I see some activity on the other side but no packet received yet.

aabadie · 2017-04-03T16:27:29Z

@PeterKietzmann, I made small progress today : I can send and receive packet between 2 SX1272 modules. I need to cleanup the branch before pushing though.

dylad

Small fixes I saw when testing your PR ;)

dylad · 2017-04-03T20:29:49Z

drivers/include/sx127x.h

+ * @param[in] dev                      The sx127x device descriptor
+ * @return the LoRa implicit mode
+ */
+bool sx12376_get_implicit_mode(sx127x_t *dev);


dylad · 2017-04-03T20:31:53Z

tests/driver_sx127x/README.md

+
+# Usage
+To setup bandwidth parameters, use lora_setup
+To change frequency, go to common.h


I think common.h is a legacy from #6002

dylad · 2017-04-03T20:38:04Z

tests/driver_sx127x/main.c

+    return 0;
+}
+
+int config_channel_cmd(int argc, char **argv)


I think we need more verbose for this one
For example we need to set a random argv[1] to set a channel

Cr0s · 2017-04-04T05:07:14Z

Thanks for the effort to put these pieces together. Will check that on my SX1276 boards soon.

aabadie · 2017-04-04T11:38:52Z

@PeterKietzmann, @Cr0s, @dylad thanks for your comments.
I just pushed a new branch (force push sorry) where I tried to address the required changes. At least the static tests pass locally and I can exchange messages between 2 modules.

I also refactored a bit the test application.

aabadie · 2017-04-06T16:50:09Z

@PeterKietzmann @dylad did you find time for some tests ?

dylad · 2017-04-06T17:42:28Z

@aabadie I plan to do some tests tomorrow I'll let you know.

BTW, I would like to know if someone has a SX1272RF1BAS board ? I would like to check something...

PeterKietzmann · 2017-04-07T06:18:15Z

Unfortunately not. I have 2 x SX1276MB1MAS and 2 x P-NUCLEO-LRWAN1. No SX1272RF1BAS, sorry.

aabadie · 2017-04-07T09:11:54Z

rebased.

@PeterKietzmann, do you plan to port RIOT to the P-NUCLEO-LRWAN1 ? It will be great and not so difficult I think (stm32l0 are supported by RIOT).

dylad · 2017-04-07T10:25:45Z

@aabadie
I did some tests today with my boards but I have (maybe ?) a problem with my SX1272RF1 board. There are old and it seems their revision is not 0x22 (from reg 0x42) because there are engineering sample released long ago. So it supposed to work but without any warranty.

I tried to exchange messages with one SX1276-based board and one of my old SX1272 and It didn't work. (Maybe because my samples are old ?)
But, I found out SX127X_REG_LR_MODEMCONFIG 1&2 are different for SX1272 and SX1276

(SX127X_REG_LR_MODEMCONFIG3 exists for SX1276 but is reserved for SX1272). So several functions cannot work for SX1272 chip like sx127x_set_bandwidth.

It may have others registers differents but I didn't have much time to investigate further.
EDIT: fix typos

PeterKietzmann · 2017-04-07T11:27:10Z

@aabadie I'm a bit confused. Actually I thought that you are working with that shield (based on your comment here).

aabadie · 2017-04-07T11:32:48Z

@PeterKietzmann, sorry, you are right, those are the ones I have... I was thinking of this board: B-L072Z-LRWAN1. This one is very interesting for porting RIOT on.

aabadie · 2017-04-07T11:39:59Z

@dylad thanks for spotting those differences. I'll look into that.
Testing the communication between the 2 different types (SX1272 and SX1276) is also a good test for interop.

dylad · 2017-04-09T12:28:54Z

drivers/sx127x/sx127x.c

+    gpio_init_int(dev->params.dio0_pin, GPIO_IN, GPIO_RISING, sx127x_on_dio0_isr, dev);
+    gpio_init_int(dev->params.dio1_pin, GPIO_IN, GPIO_RISING, sx127x_on_dio1_isr, dev);
+    gpio_init_int(dev->params.dio2_pin, GPIO_IN, GPIO_RISING, sx127x_on_dio2_isr, dev);
+    gpio_init_int(dev->params.dio3_pin, GPIO_IN, GPIO_RISING, sx127x_on_dio3_isr, dev);


@aabadie Could you add some verbose if those pins are successfully init or not please ?
I had some surprises with this.

Just pushed a commit @dylad. With ST MCU, the gpio_init_int always return 0 so it won't be helpful. I guess your are trying with a SAMD21 ? Also don't forget to set ENABLE_DEBUG (1) at the beginning of the file.

Out of curiosity, what are the surprises you had ?

Many thanks @aabadie !
I used a SAML21 and I set PA08 as DIO0 without looking and of course..., I just found out PA08 can't be used as external interrupt.
I'll retest tomorrow (if possible) with another pin.

miri64

One minor grammatical error left ;-)

miri64 · 2017-06-30T16:41:58Z

drivers/include/sx127x.h


 /**
- * @brief   Check is the SX127X LoRa RX single mode is enabled/disabled
+ * @brief   Check if the SX127X LoRa RX single mode is enabled/disabled


@dylad

Apart from the (minor) error, I'm fine with merging. Since I can not test I dismiss my review and leave the ACK to @dylad (I may proxy-ACK if they ACK'd). Please squash.

aabadie · 2017-06-30T16:52:33Z

remaining nit fixed. @dylad please ACK :)

miri64 · 2017-06-30T16:59:55Z

@aabadie in the meantime you may squash :-).

aabadie · 2017-06-30T17:01:43Z

squashed

Somehow my review was not dismissed.... Trying it again

dylad · 2017-06-30T17:29:52Z

in fact what you are trying to do is not legal
@aabadie you're perfectly right and I know the EU regulation but I'm testing ;)
To tell you the truth, I'm currently digging into my code to spot a potential LoRa rx issue which kills reception.
I'm tracking a bug that I'm unable to reproduce at my company place but it seldom happens at my client's place. We're still not sure where the bug is located but I'm investigating. I would like to flood in reception one of our nodes to simulate a dense urban area using radio to check if I can create possible issue on reception.

Note : our bug may not be related to RIOT or even to software but nevermind I had to track it and patch it.

dylad · 2017-06-30T17:33:47Z

@aabadie @miri64
Sorry I cannot re-test the last commit right now. Best I can do is monday evening.
My last test was fine and I like the recent changes.
ACK now or monday, what do you prefer ? :)

aabadie · 2017-06-30T17:42:18Z

ACK now or monday, what do you prefer ?Now ;)Otherwise, we'll have to postpone and it won't be merged before mid-July at best.I'll announce feature freeze later today or during the week-end.By the way, we retested again with @adjih and it was all good.

miri64 · 2017-06-30T17:51:43Z

@aabadie then let @adjih ACK ;-)

aabadie · 2017-06-30T17:55:21Z

@aabadie then let @adjih ACK ;-) I think I'll postpone this one finally...

miri64 · 2017-06-30T17:58:23Z

I think I'll postpone this one finally...

Why is @adjih already gone? Then I'll proxy-ACK for both @adjih and @dylad... Two witnesses except you of a working device make me confident enough.

dylad · 2017-06-30T18:15:57Z

If we can merged now let's do this ! ACK on my side.

miri64

Proxy-ACK

miri64 · 2017-06-30T18:19:45Z

🎉 We made it

dylad · 2017-06-30T18:25:06Z

Great news ! It was a very long way to go ;)
BTW, you can safely close #6002 (dunno for #6645)

aabadie · 2017-06-30T18:33:25Z

Can't believe it's merged :)

Many thanks to all people involved. Next step is LoraWAN adaption.

aabadie · 2017-06-30T18:33:47Z

🍻

dylad · 2017-06-30T18:37:06Z

Next step is LoraWAN adaption. 👍
Let's do it !

emmanuelsearch · 2017-06-30T18:38:53Z

Congrats to all involved!

miri64 · 2019-02-26T14:18:41Z

drivers/sx127x/include/sx127x_netdev.h

+ */
+typedef struct netdev_radio_lora_packet_info {
+    uint8_t rssi;           /**< RSSI of a received packet */
+    uint8_t lqi;            /**< LQI of a received packet */


I just had a look at this code again due to an unrelated issue I discussed with @kYc0o, so sorry for the "late review" ;-).

In the recv() function of this driver it says that LoRA has no LQI, so why is it in this struct?

indeed it shouldn't be here. I will fix it, since I require it for #11022

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

aabadie added this to the Release 2017.04 milestone Mar 27, 2017

aabadie assigned haukepetersen, miri64 and PeterKietzmann Mar 27, 2017

aabadie mentioned this pull request Mar 27, 2017

drivers/sx1276: LoRa transceiver phy driver proposal #6002

Closed

17 tasks

aabadie assigned jia200x Mar 27, 2017

aabadie force-pushed the driver_sx127x branch 2 times, most recently from 925d23a to 2026c0c Compare March 31, 2017 10:44

PeterKietzmann reviewed Apr 3, 2017

View reviewed changes

dylad reviewed Apr 3, 2017

View reviewed changes

aabadie force-pushed the driver_sx127x branch 2 times, most recently from d9ec5ca to 17332c1 Compare April 4, 2017 11:37

aabadie force-pushed the driver_sx127x branch 2 times, most recently from 3c1cb1e to 1668db8 Compare April 5, 2017 17:08

aabadie force-pushed the driver_sx127x branch from 1668db8 to 1f0c4b1 Compare April 7, 2017 09:10

dylad reviewed Apr 9, 2017

View reviewed changes

miri64 previously requested changes Jun 30, 2017

View reviewed changes

sys/netopt: add new enum types for LoRa

b80c249

aabadie added 2 commits June 30, 2017 19:00

drivers: initial implementation of sx127x radio (LoRa mode only)

0ccc264

test: add sx127x test application

57c51e4

aabadie force-pushed the driver_sx127x branch from 92e8ec7 to 57c51e4 Compare June 30, 2017 17:01

miri64 approved these changes Jun 30, 2017

View reviewed changes

miri64 merged commit d61be01 into RIOT-OS:master Jun 30, 2017

aabadie mentioned this pull request Jun 30, 2017

[WIP] GNRC LoRaWAN #6645

Closed

5 tasks

aabadie deleted the driver_sx127x branch February 26, 2018 12:22

miri64 reviewed Feb 26, 2019

View reviewed changes

miri64 mentioned this pull request Feb 26, 2019

gnrc_lorawan: add initial support for GNRC based LoRaWAN stack (v2) #11022

Merged

jia200x mentioned this pull request Mar 8, 2019

netdev/lora: add netdev_lora_rx_info structure for RX info #11144

Merged

Conversation

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

Uh oh!

Uh oh!

PeterKietzmann 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!

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 Apr 3, 2017

Uh oh!

aabadie commented Apr 3, 2017

Uh oh!

dylad 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!

Cr0s commented Apr 4, 2017

Uh oh!

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

Uh oh!

Uh oh!

aabadie commented Apr 6, 2017

Uh oh!

dylad commented Apr 6, 2017

Uh oh!

PeterKietzmann commented Apr 7, 2017

Uh oh!

aabadie commented Apr 7, 2017

Uh oh!

dylad commented Apr 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PeterKietzmann commented Apr 7, 2017

Uh oh!

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

Uh oh!

Uh oh!

aabadie commented Apr 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

aabadie commented Mar 27, 2017 •

edited

Loading

aabadie commented Apr 4, 2017 •

edited

Loading

dylad commented Apr 7, 2017 •

edited

Loading

aabadie commented Apr 7, 2017 •

edited

Loading

dylad commented Jun 30, 2017 •

edited

Loading