drivers/sx126x : r/NETOPT_RX_TIMEOUT/NETOPT_RX_SYMBOL_TIMEOUT #16599

akshaim · 2021-06-30T18:04:46Z

Contribution description

This PR adds NETOPT_RX_SYMBOL_TIMEOUT which is required by the sx126x radio and also removes NETOPT_RX_TIMEOUT which is no longer required. The PR has been made as per discussions in thread #16579 , which would integrate a periph version of sx1261 radio driver used in STMWL55JC SoC.

Forced Radio Rx timeout timer to stop on preamble detection, else it breaks the current stack.

Testing procedure

The changes were already tested in #16579 using Nucleo-WL55JC and logs can be found in its description.

Issues/PRs references

#16579
Depends on #16604

drivers/sx126x/sx126x_netdev.c

fjmolinas · 2021-07-06T10:15:13Z

Please rebase!

fjmolinas · 2021-07-06T11:24:19Z

Spurious commits are still present from dependency PR

akshaim · 2021-07-06T11:39:15Z

Spurious commits are still present from dependency PR

Rebased to current master 👍🏼

fjmolinas · 2021-07-06T14:48:01Z

Could you maybe retest with your branch and post the results @akshaim ? I'm trying to rest with @jia200x but I'm running into issues with an llcc68

akshaim · 2021-07-06T15:31:47Z

Could you maybe retest with your branch and post the results @akshaim ? I'm trying to rest with @jia200x but I'm running into issues with an llcc68

Yes. Please see here

fjmolinas · 2021-07-06T16:33:47Z

Since I don't have a gateway I sued the following patch @jia200x provided:

diff --git a/tests/driver_sx126x/main.c b/tests/driver_sx126x/main.c
index 313c95b4ed..f860c1a13a 100644
--- a/tests/driver_sx126x/main.c
+++ b/tests/driver_sx126x/main.c
@@ -30,6 +30,7 @@
 #include "net/lora.h"
 #include "net/netdev.h"
 #include "net/netdev/lora.h"
+#include "mutex.h"

 #include "sx126x.h"
 #include "sx126x_params.h"
@@ -47,6 +48,8 @@ static char message[SX126X_MAX_PAYLOAD_LEN];

 static sx126x_t sx126x;

+static mutex_t lock;
+
 static void _event_cb(netdev_t *dev, netdev_event_t event)
 {
     if (event == NETDEV_EVENT_ISR) {
@@ -86,6 +89,10 @@ static void _event_cb(netdev_t *dev, netdev_event_t event)
             puts("Transmission timeout");
             break;

+        case NETDEV_EVENT_RX_TIMEOUT:
+            mutex_unlock(&lock);
+            break;
+
         default:
             printf("Unexpected netdev event received: %d\n", event);
             break;
@@ -318,6 +325,21 @@ static const shell_command_t shell_commands[] = {
     { NULL, NULL, NULL }
 };

+static void _measure_rx_timeout(netdev_t *netdev, uint8_t rx_timeout)
+{
+    netdev->driver->set(netdev, NETOPT_RX_SYMBOL_TIMEOUT, &rx_timeout, sizeof(rx_timeout));
+    netopt_state_t state = NETOPT_STATE_IDLE;
+    netdev->driver->set(netdev, NETOPT_STATE, &state, sizeof(state));
+    if (rx_timeout) {
+        printf("START (%i)\n", rx_timeout);
+        mutex_lock(&lock);
+        puts("STOP");
+    }
+    else {
+        puts("RX_CONTINUOUS");
+    }
+}
+
 int main(void)
 {
     sx126x_setup(&sx126x, &sx126x_params[0], 0);
@@ -341,10 +363,19 @@ int main(void)
         return 1;
     }

+    mutex_init(&lock);
+    mutex_lock(&lock);
     /* start the shell */
     puts("Initialization successful - starting the shell now");
     char line_buf[SHELL_DEFAULT_BUFSIZE];

+    uint8_t sf = LORA_SF11;
+    netdev->driver->get(netdev, NETOPT_SPREADING_FACTOR, &sf, sizeof(uint8_t));
+
+    _measure_rx_timeout(netdev, 10);
+    _measure_rx_timeout(netdev, 50);
+    _measure_rx_timeout(netdev, 0);
+
     shell_run(shell_commands, line_buf, SHELL_DEFAULT_BUFSIZE);
     return 0;
 }

With this and only pyterm timings, it seems its accurate:

2021-07-06 18:32:03,932 # main(): This is RIOT! (Version: 2021.10-devel-37-g34f086-pr-16599)
2021-07-06 18:32:03,939 # Initialization successful - starting the shell now
2021-07-06 18:32:03,940 # START (10)
2021-07-06 18:32:03,950 # STOP
2021-07-06 18:32:03,951 # START (50)
2021-07-06 18:32:04,001 # STOP
2021-07-06 18:32:04,006 # RX_CONTINUOUS

fjmolinas · 2021-07-06T16:34:11Z

I consider this tested, @jia200x are your changes addressed?

jia200x · 2021-07-07T08:03:52Z

With this and only pyterm timings, it seems its accurate:

Hmmm the only weird thing I see from the output is the fact the timings are similar to the timings of SF7 (Ts=1.024 ms). T=10 took around 10ms and T=50 around 50. The strange thing is, I explicitly set SF11 in the patch.

Maybe the command failed? It could also be something wrong with the function that calculates the timeout.

fjmolinas · 2021-07-08T08:31:34Z

With this and only pyterm timings, it seems its accurate:

Hmmm the only weird thing I see from the output is the fact the timings are similar to the timings of SF7 (Ts=1.024 ms). T=10 took around 10ms and T=50 around 50. The strange thing is, I explicitly set SF11 in the patch.

Maybe the command failed? It could also be something wrong with the function that calculates the timeout.

Changing the SF, does not seem to affect the result..

2021-07-08 10:30:59,795 # main(): This is RIOT! (Version: 2021.10-devel-37-g34f086-pr-16599)
2021-07-08 10:30:59,802 # Initialization successful - starting the shell now
2021-07-08 10:30:59,805 # [14653us]: START (10)
2021-07-08 10:30:59,814 # [24875us]: STOP
2021-07-08 10:30:59,817 # [26385us]: START (50)
2021-07-08 10:30:59,867 # [77436us]: STOP
2021-07-08 10:30:59,869 # [78946us]: RX_CONTINOUS

fjmolinas · 2021-07-08T08:32:37Z

drivers/include/sx126x.h

+    assert(dev && (dev->mod_params.bw <= SX126X_LORA_BW_500) && \
+           (dev->mod_params.bw >= SX126X_LORA_BW_125));
+
+    /* Refer section 4.1.1.7  Time on air in SX1276/77/78/79 datasheet */


Suggested change

/* Refer section 4.1.1.7 Time on air in SX1276/77/78/79 datasheet */

/* Refer section 4.1.1.7 Time on air in SX1276/77/78/79 datasheet */

Seems like its the wrong datasheet reference...

I can't find the section either

Yeah well its the wrong datasheet, this is sx126x not 127x, I find it weird to have a reference to another driver datasheet.

Can the correct datasheet be referenced?

It was more about the math. Let me see if there are references in the sx126x datasheet.

fjmolinas · 2021-07-08T08:42:43Z

drivers/sx126x/sx126x_netdev.c

@@ -288,8 +288,9 @@ static int _set_state(sx126x_t *dev, netopt_state_t state)
    case NETOPT_STATE_RX:
        DEBUG("[sx126x] netdev: set NETOPT_STATE_RX state\n");
        sx126x_cfg_rx_boosted(dev, true);
-        if (dev->rx_timeout != 0) {
-            sx126x_set_rx(dev, dev->rx_timeout);
+        const int _timeout = (sx126x_symbol_to_msec(dev, dev->rx_timeout));


Is this really const?

Just to be safe, compile optimisation and adding some clarity. The variable should only be set via sx126x_symbol_to_msec , wrong values will break the stack.

const have no meaning here. It should be dropped.

Oops. Thanks, Jose for the clarification. Updated

drivers/include/sx126x.h

fjmolinas · 2021-07-08T09:52:53Z

Amended output:

2021-07-08 11:51:13,309 # main(): This is RIOT! (Version: 2021.10-devel-37-g34f086-pr-16599)
2021-07-08 11:51:13,317 # Initialization successful - starting the shell now
2021-07-08 11:51:13,319 # [sx126x]: set timeout to 163
2021-07-08 11:51:13,321 # [17362us]: START (10)
2021-07-08 11:51:13,484 # [179980us]: STOP
2021-07-08 11:51:13,487 # [sx126x]: set timeout to 819
2021-07-08 11:51:13,489 # [184132us]: START (50)
2021-07-08 11:51:14,303 # [999750us]: STOP
2021-07-08 11:51:14,306 # [1001347us]: RX_CONTINOUS

diff --git a/drivers/sx126x/sx126x_netdev.c b/drivers/sx126x/sx126x_netdev.c
index da14679395..31edb09606 100644
--- a/drivers/sx126x/sx126x_netdev.c
+++ b/drivers/sx126x/sx126x_netdev.c
@@ -288,8 +288,9 @@ static int _set_state(sx126x_t *dev, netopt_state_t state)
     case NETOPT_STATE_RX:
         DEBUG("[sx126x] netdev: set NETOPT_STATE_RX state\n");
         sx126x_cfg_rx_boosted(dev, true);
-        const int _timeout = (sx126x_symbol_to_msec(dev, dev->rx_timeout));
+        int _timeout = (sx126x_symbol_to_msec(dev, dev->rx_timeout));
         if (_timeout != 0) {
+            printf("[sx126x]: set timeout to %d\n", _timeout);
             sx126x_set_rx(dev, _timeout);
         }
         else {
diff --git a/tests/driver_sx126x/Makefile b/tests/driver_sx126x/Makefile
index 68af3a584c..246dd6939c 100644
--- a/tests/driver_sx126x/Makefile
+++ b/tests/driver_sx126x/Makefile
@@ -6,5 +6,6 @@ USEMODULE += $(LORA_DRIVER)
 
 USEMODULE += shell
 USEMODULE += shell_commands
+USEMODULE += ztimer_usec
 
 include $(RIOTBASE)/Makefile.include
diff --git a/tests/driver_sx126x/main.c b/tests/driver_sx126x/main.c
index 313c95b4ed..fa3be48cae 100644
--- a/tests/driver_sx126x/main.c
+++ b/tests/driver_sx126x/main.c
@@ -26,10 +26,12 @@
 #include "msg.h"
 #include "thread.h"
 #include "shell.h"
+#include "ztimer.h"
 
 #include "net/lora.h"
 #include "net/netdev.h"
 #include "net/netdev/lora.h"
+#include "mutex.h"
 
 #include "sx126x.h"
 #include "sx126x_params.h"
@@ -47,6 +49,8 @@ static char message[SX126X_MAX_PAYLOAD_LEN];
 
 static sx126x_t sx126x;
 
+static mutex_t lock;
+
 static void _event_cb(netdev_t *dev, netdev_event_t event)
 {
     if (event == NETDEV_EVENT_ISR) {
@@ -86,6 +90,10 @@ static void _event_cb(netdev_t *dev, netdev_event_t event)
             puts("Transmission timeout");
             break;
 
+        case NETDEV_EVENT_RX_TIMEOUT:
+            mutex_unlock(&lock);
+            break;
+
         default:
             printf("Unexpected netdev event received: %d\n", event);
             break;
@@ -318,6 +326,21 @@ static const shell_command_t shell_commands[] = {
     { NULL, NULL, NULL }
 };
 
+static void _measure_rx_timeout(netdev_t *netdev, uint8_t rx_timeout)
+{
+    netdev->driver->set(netdev, NETOPT_RX_SYMBOL_TIMEOUT, &rx_timeout, sizeof(rx_timeout));
+    netopt_state_t state = NETOPT_STATE_IDLE;
+    netdev->driver->set(netdev, NETOPT_STATE, &state, sizeof(state));
+    if (rx_timeout) {
+        printf("[%"PRIu32"us]: START (%i)\n", ztimer_now(ZTIMER_USEC), rx_timeout);
+        mutex_lock(&lock);
+        printf("[%"PRIu32"us]: STOP\n", ztimer_now(ZTIMER_USEC));
+    }
+    else {
+        printf("[%"PRIu32"us]: RX_CONTINOUS\n", ztimer_now(ZTIMER_USEC));
+    }
+}
+
 int main(void)
 {
     sx126x_setup(&sx126x, &sx126x_params[0], 0);
@@ -341,10 +364,19 @@ int main(void)
         return 1;
     }
 
+    mutex_init(&lock);
+    mutex_lock(&lock);
     /* start the shell */
     puts("Initialization successful - starting the shell now");
     char line_buf[SHELL_DEFAULT_BUFSIZE];
 
+    uint8_t sf = LORA_SF11;
+    netdev->driver->set(netdev, NETOPT_SPREADING_FACTOR, &sf, sizeof(uint8_t));
+
+    _measure_rx_timeout(netdev, 10);
+    _measure_rx_timeout(netdev, 50);
+    _measure_rx_timeout(netdev, 0);
+
     shell_run(shell_commands, line_buf, SHELL_DEFAULT_BUFSIZE);
     return 0;
 }

jia200x

ACK. I trust the results of @fjmolinas

jia200x

NACK. This one is not yet addressed: #16599 (comment)

jia200x · 2021-07-08T10:18:51Z

please squash

r/NETOPT_RX_TIMEOUT/NETOPT_RX_SYMBOL_TIMEOUT Use sx126x_symbol_to_msec() to calculate the RX timeout in ms.

jia200x

Re-ACK

github-actions bot added Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support labels Jun 30, 2021

akshaim force-pushed the pr/sx126xsymbol_timeout branch from 9ccd437 to bfb8109 Compare June 30, 2021 18:09

akshaim added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 30, 2021

akshaim force-pushed the pr/sx126xsymbol_timeout branch from bfb8109 to 1398498 Compare June 30, 2021 19:01

akshaim added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 30, 2021

akshaim requested review from jia200x, aabadie and fjmolinas June 30, 2021 20:43

jia200x requested changes Jul 1, 2021

View reviewed changes

drivers/sx126x/sx126x_netdev.c Outdated Show resolved Hide resolved

akshaim added the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 1, 2021

akshaim force-pushed the pr/sx126xsymbol_timeout branch from 28c0ff4 to 521402b Compare July 1, 2021 15:00

akshaim mentioned this pull request Jul 1, 2021

drivers/sx126x: Add support for Nucleo -WL55JC #16579

Merged

akshaim force-pushed the pr/sx126xsymbol_timeout branch from 521402b to 8e9091b Compare July 5, 2021 13:42

akshaim requested review from cgundogan, haukepetersen, leandrolanzieri, MichelRottleuthner, miri64, PeterKietzmann and smlng as code owners July 5, 2021 13:42

github-actions bot added Area: network Area: Networking Area: pkg Area: External package ports Area: sys Area: System Area: tests Area: tests and testing framework labels Jul 5, 2021

akshaim force-pushed the pr/sx126xsymbol_timeout branch from 8e9091b to 1f3f934 Compare July 6, 2021 11:17

akshaim force-pushed the pr/sx126xsymbol_timeout branch from 1f3f934 to 34f086f Compare July 6, 2021 11:38

github-actions bot removed Area: network Area: Networking Area: pkg Area: External package ports Area: tests Area: tests and testing framework Area: sys Area: System labels Jul 6, 2021

akshaim removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jul 6, 2021

fjmolinas added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jul 6, 2021

fjmolinas reviewed Jul 8, 2021

View reviewed changes

drivers/include/sx126x.h Show resolved Hide resolved

jia200x approved these changes Jul 8, 2021

View reviewed changes

jia200x requested changes Jul 8, 2021

View reviewed changes

akshaim added 3 commits July 8, 2021 12:19

drivers/sx126x.h : Add sx126x_symbol_to_msec()

a3ed19e

drivers/sx126x : Update NETOPT_RX_xx

8d43dbf

r/NETOPT_RX_TIMEOUT/NETOPT_RX_SYMBOL_TIMEOUT Use sx126x_symbol_to_msec() to calculate the RX timeout in ms.

drivers/sx126x : Configure Rx timeout timer

4d890b1

akshaim force-pushed the pr/sx126xsymbol_timeout branch from 4b5553d to 4d890b1 Compare July 8, 2021 10:19

jia200x approved these changes Jul 8, 2021

View reviewed changes

jia200x merged commit d75f032 into RIOT-OS:master Jul 8, 2021

akshaim deleted the pr/sx126xsymbol_timeout branch July 9, 2021 07:10

jia200x mentioned this pull request Jul 13, 2021

net/lorawan: Revert #16604 and fix NETOPT_RX_SYMBOL_TIMEOUT documentation #16640

Merged

benpicco added this to the Release 2021.10 milestone Oct 20, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers/sx126x : r/NETOPT_RX_TIMEOUT/NETOPT_RX_SYMBOL_TIMEOUT #16599

drivers/sx126x : r/NETOPT_RX_TIMEOUT/NETOPT_RX_SYMBOL_TIMEOUT #16599

akshaim commented Jun 30, 2021 •

edited

Loading

fjmolinas commented Jul 6, 2021

fjmolinas commented Jul 6, 2021

akshaim commented Jul 6, 2021

fjmolinas commented Jul 6, 2021

akshaim commented Jul 6, 2021

fjmolinas commented Jul 6, 2021

fjmolinas commented Jul 6, 2021

jia200x commented Jul 7, 2021

fjmolinas commented Jul 8, 2021

fjmolinas Jul 8, 2021

fjmolinas Jul 8, 2021

akshaim Jul 8, 2021

akshaim Jul 8, 2021

fjmolinas Jul 8, 2021

fjmolinas Jul 8, 2021

akshaim Jul 8, 2021

akshaim Jul 8, 2021

fjmolinas Jul 8, 2021

akshaim Jul 8, 2021 •

edited

Loading

jia200x Jul 8, 2021

akshaim Jul 8, 2021 •

edited

Loading

fjmolinas commented Jul 8, 2021

jia200x left a comment

jia200x left a comment

jia200x commented Jul 8, 2021

jia200x left a comment

	/* Refer section 4.1.1.7 Time on air in SX1276/77/78/79 datasheet */
	/* Refer section 4.1.1.7 Time on air in SX1276/77/78/79 datasheet */

drivers/sx126x : r/NETOPT_RX_TIMEOUT/NETOPT_RX_SYMBOL_TIMEOUT #16599

drivers/sx126x : r/NETOPT_RX_TIMEOUT/NETOPT_RX_SYMBOL_TIMEOUT #16599

Conversation

akshaim commented Jun 30, 2021 • edited Loading

Contribution description

Testing procedure

Issues/PRs references

fjmolinas commented Jul 6, 2021

fjmolinas commented Jul 6, 2021

akshaim commented Jul 6, 2021

fjmolinas commented Jul 6, 2021

akshaim commented Jul 6, 2021

fjmolinas commented Jul 6, 2021

fjmolinas commented Jul 6, 2021

jia200x commented Jul 7, 2021

fjmolinas commented Jul 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshaim Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshaim Jul 8, 2021 • edited Loading

Choose a reason for hiding this comment

fjmolinas commented Jul 8, 2021

jia200x left a comment

Choose a reason for hiding this comment

jia200x left a comment

Choose a reason for hiding this comment

jia200x commented Jul 8, 2021

jia200x left a comment

Choose a reason for hiding this comment

akshaim commented Jun 30, 2021 •

edited

Loading

akshaim Jul 8, 2021 •

edited

Loading

akshaim Jul 8, 2021 •

edited

Loading