drivers/wireless: Add support for the RN903 and RN2483 LoRa modules #15828

linguini1 · 2025-02-13T05:19:48Z

Summary

Add support for the RN903 and RN2483 family of LoRa radio transceivers. This initial support includes transmit and receive functionality and configuration and reading of radio parameters like frequency and bandwidth.

Impact

NuttX users can now use these radio transceivers!

This changes touches the build system, wireless drivers and documentation. The documentation includes examples of how to use the driver and its available commands.

Testing

Testing was performed from an RP2040 based custom flight computer. The chip this code was tested again was the RN2903, but commands for both modules are the same. The only difference is the values allowed for each command, but the radio module does its own error checking so the driver code will not need to change between modules.

The test script that was used to test the commands was:

#include <errno.h>
#include <fcntl.h>
#include <stdint.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/ioctl.h>
#include <syslog.h>
#include <unistd.h>

#include <nuttx/wireless/lpwan/rn2xx3.h>

#define handle_errno(msg, err)                                               \
  if ((err) < 0)                                                             \
    {                                                                        \
      fprintf(stderr, msg ": %d\n", errno);                                  \
      close(radio);                                                          \
      return EXIT_FAILURE;                                                   \
    }

int main(int argc, FAR char *argv[])
{
  int err;
  int radio;

  radio = open("/dev/rn2903", O_RDWR);
  if (radio < 0)
    {
      fprintf(stderr, "Couldn't open RN2903: %d\n", errno);
      return EXIT_FAILURE;
    }

  /* Set frequency */

  err = ioctl(radio, WLIOC_SETRADIOFREQ, 902400000);
  handle_errno("Couldn't set freq", err);

  /* Get frequency */

  uint32_t freq;
  err = ioctl(radio, WLIOC_GETRADIOFREQ, &freq);
  handle_errno("Couldn't get freq", err);

  printf("Frequency: %lu Hz\n", freq);

  /* Set TX power */

  float txpower = 13.0;
  err = ioctl(radio, WLIOC_SETTXPOWERF, &txpower);
  handle_errno("Couldn't set txpwr", err);

  printf("Transmit power set to %.2f dBm\n", txpower);

  /* Get TX power */

  float txpwr;
  err = ioctl(radio, WLIOC_GETTXPOWERF, &txpwr);
  handle_errno("Couldn't get txpwr", err);

  printf("Tx power level: %.2f\n", txpwr);

  /* Get SNR */

  int8_t snr;
  err = ioctl(radio, WLIOC_GETSNR, &snr);
  handle_errno("Couldn't get snr", err);

  printf("SNR: %d\n", snr);

  /* Set preamble length */

  err = ioctl(radio, WLIOC_SETPRLEN, 8);
  handle_errno("Couldn't set prlen", err);

  /* Get preamble length */

  uint16_t prlen;
  err = ioctl(radio, WLIOC_GETPRLEN, &prlen);
  handle_errno("Couldn't get prlen", err);

  printf("Preamble length: %u\n", prlen);

  /* Set spread */

  err = ioctl(radio, WLIOC_SETSPREAD, 8);
  handle_errno("Couldn't set spread factor", err);

  /* Get spread */

  uint8_t spread;
  err = ioctl(radio, WLIOC_GETSPREAD, &spread);
  handle_errno("Couldn't get spread factor", err);

  printf("Spread factor: sf%u\n", spread);

  usleep(10000);

  /* Set modulation */

  err = ioctl(radio, WLIOC_SETMOD, RN2XX3_MOD_FSK);
  handle_errno("Couldn't set modulation", err);

  /* Get modulation */

  enum rn2xx3_mod_e modulation;
  err = ioctl(radio, WLIOC_GETMOD, &modulation);
  handle_errno("Couldn't get modulation", err);

  printf("Modulation: %d\n", modulation);

  usleep(10000);

  /* Set bandwidth */

  uint32_t bandwidth = 250;
  err = ioctl(radio, WLIOC_SETBANDWIDTH, bandwidth);
  handle_errno("Couldn't set bw", err);

  /* Get bandwidth */

  err = ioctl(radio, WLIOC_GETBANDWIDTH, &bandwidth);
  handle_errno("Couldn't get bw", err);

  printf("Bandwidth: %lu kHz\n", bandwidth);

  /* Set sync word */

  uint64_t syncword = 0xdeadbeefdeadbeef;
  err = ioctl(radio, WLIOC_SETSYNC, &syncword);
  handle_errno("Couldn't set sync", err);

  /* Get sync word */

  err = ioctl(radio, WLIOC_GETSYNC, &syncword);
  handle_errno("Couldn't get sync", err);

  printf("Sync word is: %016llX\n", syncword);

  /* Set bit rate */

  err = ioctl(radio, WLIOC_SETBITRATE, 1542);
  handle_errno("Couldn't set bitrate", err);

  /* Get bit rate */

  uint32_t bitrate = 300000;
  err = ioctl(radio, WLIOC_GETBITRATE, &bitrate);
  handle_errno("Couldn't get bitrate", err);

  printf("Bit rate is: %lu\n", bitrate);

  /* Disable IQI */

  err = ioctl(radio, WLIOC_IQIEN, 0);
  handle_errno("Couldn't disable IQI", err);

  printf("IQI disabled\n");

  /* Enable CRC */

  err = ioctl(radio, WLIOC_CRCEN, 1);
  handle_errno("Couldn't enable CRC", err);

  printf("CRC enabled\n");

  /* Set coding rate */

  err = ioctl(radio, WLIOC_SETCODERATE, RN2XX3_CR_4_8);
  handle_errno("Couldn't set coding rate", err);

  /* Get coding rate */

  enum rn2xx3_cr_e cr;
  err = ioctl(radio, WLIOC_GETCODERATE, &cr);
  handle_errno("Couldn't get coding rate", err);

  printf("Coding rate: %d\n", cr);

  /* Reset radio module */

  printf("Resetting radio.\n");

  err = ioctl(radio, WLIOC_RESET, 0);
  handle_errno("Couldn't reset radio", err);

  printf("Radio reset.\n");

  close(radio);
  return 0;
}
}

The output is:

nsh> hello
Frequency: 902400000 Hz
Transmit power set to 13.00 dBm
Tx power level: 13.00
SNR: -128
Preamble length: 8
Spread factor: sf8
Modulation: 1
Bandwidth: 250 kHz
Sync word is: DEADBEEFDEADBEEF
Bit rate is: 5442
IQI disabled
CRC enabled
Coding rate: 3
Resetting radio.
Radio reset.

I also performed transmit/receive testing using a second RN2903 which I used to transmit/receive from a Python script. I was able to successfully receive messages sent from the NuttX driver, and I was also able to successfully receive messages with the NuttX driver sent from the Python script controlled transceiver.

The code works with read/write functions, so it can be echoed into or catted from to transmit and receive in the shell.

Documentation was built locally and verified for correct render in the browser.

nuttxpr · 2025-02-13T05:33:36Z

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although there's room for improvement in the level of detail provided.

Strengths:

Clear Summary: The summary explains the what and why of the change well.
Impact Description: Addresses most impact areas, highlighting key changes in build, drivers, and documentation.
Testing Evidence: Provides test code, output, and a description of additional transmit/receive tests, which is good. The mention of documentation build verification is also positive.

Areas for Improvement:

Summary - How it works: The summary should briefly explain how the change is implemented (e.g., new driver using UART, specific files modified).
Impact - More Detail: While the impact sections are addressed, they lack specifics. For example:
- Build: How does the build process change? New Kconfig options? Build dependencies?
- Hardware: Which specific arch/board was tested? Mentioning RP2040 is good, but the full board name/configuration would be better. Clarify the RN2903/RN2483 compatibility – are both fully supported or is there limited functionality for one?
- Documentation: Where are the documentation changes located? (e.g., "Added a new section to the wireless drivers documentation").
- Compatibility: Are there any known backward compatibility issues?
Testing - Before/After Logs: The provided test code is good, but the "Testing logs before change" and "Testing logs after change" sections should show the actual console output before and after the driver was added. While the provided output demonstrates the driver functionality, it doesn't demonstrate the change itself. The transmit/receive test with Python is good, but some snippets of that code/output would further strengthen the testing evidence.
Testing - Build Host: Provide details about your build host environment (OS, compiler version, etc.).

Specific Recommendations:

Expand the Summary's "How it Works": Briefly describe the implementation (e.g., "A new character driver was implemented for the RN903/RN2483 LoRa radio transceivers using the UART interface. Modifications were made to the wireless driver subsystem and the build system.").
Provide More Specifics in Impact Sections: For instance, in "Build," mention specific Kconfig options or build dependencies introduced. In "Hardware," specify the exact board used (e.g., "Custom RP2040 board with configuration X"). In "Documentation," give the file path of the updated documentation.
Improve Testing Logs: Show console output before the driver existed (likely an error message when trying to access the device) and after (the successful output already shown). Include snippets of the Python test code and its output.
Add Build Host Details: Specify your build host OS, compiler, and versions.

By addressing these points, the PR will be even stronger and provide reviewers with all the necessary information to efficiently evaluate the changes.

lupyuen · 2025-02-13T06:35:33Z

Thanks @linguini1! I suggest we rename the PR Title to "drivers/wireless: Add support for..."? So it looks nicer in the NuttX Release Notes :-) https://nuttx.apache.org/releases/12.8.0/

drivers/wireless/lpwan/rn2xx3/rn2xx3.c

tcpipchip · 2025-02-13T10:59:37Z

Hi, do you plans to port to STM32WL55 ?

linguini1 · 2025-02-13T14:17:31Z

Hi, do you plans to port to STM32WL55 ?

If that MCU is supported by NuttX, then this driver should work on it. This is a generic driver that should work with any UART interface.

tcpipchip · 2025-02-13T15:16:13Z

Hi, do you plans to port to STM32WL55 ?

If that MCU is supported by NuttX, then this driver should work on it. This is a generic driver that should work with any UART interface.

SX1262 ?

linguini1 · 2025-02-13T16:10:41Z

SX1262 ?

If NuttX supports the STM32WL55, so long as you connect the RN2xx3 to a UART interface on that chip, you can use this driver with that chip running NuttX. This driver is not for the SX1262. It appears that the WL55 is supported so this driver could be used on that chip.

mlyszczek · 2025-02-13T19:00:26Z

drivers/wireless/lpwan/rn2xx3/rn2xx3.c

+ *
+ ****************************************************************************/
+
+static ssize_t read_line(FAR struct rn2xx3_dev_s *priv, FAR char *buf,


Something still does not quite sit right here with me. You implicitly trust in data returned from the modem.

Can modem send more than 2 lines at a time? Can your driver lead to situation where 2 lines are in uart buffer? What will happend when there are 2 lines in buffer (like 10 char long) and you call it with nbytes == 15? You will read half of line, and next call will return invalid data.

Second case, what if line in uart buffer is longer than nbytes? You again will read half of frame - and report success, so at least 2 consecutive calls will return invalid data. I think you should sanitize received frame here to line level. If you did not read whole line, return -1, and discard from uart buffer until you reach \n.

I think you should implement small (at least 2 times the maximum size of single line modem can throw at you) ring buffer here and use that to read data. You can then call file_read when buffer gets empty to fill it back in.

Otherwise you will be returning invalid data to userspace.

Thank you for the catch on this case. I have resolved case 2. However, in regards to case 1, no it is not possible with this driver for 2 lines to be present in the UART buffer. All commands to the modem are call/response, and before each call/response sequence I flush the UART buffer to make sure there is no lingering data that will interrupt the regular flow. The modem cannot send more than one line at a time, unless I deliberately issue more than one command at a time.

drivers/wireless/lpwan/rn2xx3/rn2xx3.c

include/nuttx/wireless/ioctl.h

raiden00pl · 2025-02-16T07:40:49Z

include/nuttx/wireless/ioctl.h

+#define WLIOC_SETSPREAD     _WLCIOC(0x0009)  /* arg: Pointer to uint8_t, */
+                                             /* spread factor */
+#define WLIOC_GETSPREAD     _WLCIOC(0x000a)  /* arg: Pointer to uint8_t, */
+                                             /* spread factor */
+#define WLIOC_GETSNR        _WLCIOC(0x000b)  /* arg: Pointer to int8_t, */
+                                             /* signal to noise ratio */
+#define WLIOC_SETPRLEN      _WLCIOC(0x000c)  /* arg: uint16_t, */
+                                             /* preamble length */
+#define WLIOC_GETPRLEN      _WLCIOC(0x000d)  /* arg: Pointer to uint16_t, */
+                                             /* preamble length */
+#define WLIOC_SETMOD        _WLCIOC(0x000e)  /* arg: enum, */
+                                             /* modulation type */
+#define WLIOC_GETMOD        _WLCIOC(0x000f)  /* arg: char[10] pointer, */
+                                             /* modulation type */


can we reuse some of sx127x commands ? (include/nuttx/wireless/lpwan/sx127x.h)

The thing is that they are all called SX127XIOC_*. I think that would look confusing to be used on the RN2xx3. That's why I don't think there really should be different naming for "device specific" ioctl commands since they can be re-used by other devices that way.

If desired I could also convert the SX127X commands to general WLIOC commands? That way any module can use them since there would be some sharing between it and the rn2xx3?

If desired I could also convert the SX127X commands to general WLIOC commands? That way any module can use them since there would be some sharing between it and the rn2xx3?

Yes, I think this is the correct way. When I created the sx127x driver there was no other chips for Lora available, now there is much more chips supporting Lora, so some of these should be moved to common ioctl.
Ideally, we should be able to use different Lora chips with the same code.

My thought to implement this would be a separate PR after this merged. I would replace all the SX127 commands with WLIOC commands, just by modifying the #define statements to be WLIOC commands under a different name. That would avoid breaking some user code. Do you think this is worthwhile or should we just remove the device specific commands entirely since some of the arguments would change?

Yeah i also was thinking of just keeping the old SX127 ones. But turn them into macros that are actually the (to be added) common WLIOC when expanded. No need to go over existing boards using this driver.

personally I'm not a fan of keeping unnecessary legacy code that is trivial to fix on the user side. If there is someone who uses sx127x and thinks we should keep legacy ioctl then ok. If I was the only user of sx127x then I have nothing against breaking change here.

raiden00pl · 2025-02-16T07:45:35Z

drivers/wireless/lpwan/rn2xx3/rn2xx3.c

+ *
+ ****************************************************************************/
+
+static int8_t rn2903_txpwrlevel(FAR float *dbm)


are floats really required here? it's not friendly for MCU without FPU. You should avoid floats in shared code and use something more universal if possible, like fixedmath.h

I feel like this is more intuitive than fixed math. Is it really that unfriendly for MCUs without an FPU? It is just used in a comparison.

raiden00pl · 2025-02-16T07:49:05Z

drivers/wireless/lpwan/rn2xx3/rn2xx3.c

+
+    case WLIOC_SETTXPOWER:
+      {
+        FAR float *txpwr = (FAR float *)(arg);


this is wrong, should be int32_t:

#define WLIOC_SETTXPOWER _WLCIOC(0x0005) /* arg: Pointer to int32_t, */ /* output power (in dBm) */

How should I reflect fractional output power? I.e. something like 14.7dBm cannot be represented with an int32_t of units dBm.

Yeah, this one is problematic. How bad would it be for it to NOT use fractional? Setting TX power to 15dBm instead of 14.7dBm?

Yeah, this one is problematic. How bad would it be for it to NOT use fractional? Setting TX power to 15dBm instead of 14.7dBm?

In my case I don't believe it would be possible. For instance, some of the valid txpower levels that can be set for the RN2483 are -1.9, -1.7, -0.6, 0.4, 1.4 dBm. If -2 is passed as an int32_t parameter, I won't know to set -1.9 or -1.7. If -1 is passed, should it be -0.6 or -1.7?

These transceivers only support some specific levels as well, so my idea was to return back within the pointer the true dBm txpower that was set so the user is aware of any deviations from their specified level. It isn't possible to do that with an int32_t either. I think it might be wise to expand the command parameter to a float, as I'm sure this is not the only module that will have these issues.

I would be extremely careful when changing type to float. While beneficial in this case you will break all userspace programs that are using this. I would suggest to add new request WLIOC_SETTXPOWERF and add a thin wrapper to common code for WLIOC_SETTXPOWER, that when called will convert int to float and call WLIOC_SETTXPOWERF.

Ideally you would also change all driver code that use WLIOC_SETTXPOWER to actually use WLIOC_SETTXPOWERF instead. That way backward compatibility would be maintained with single function that should never see the need to be modified.

maybe we have someone experienced in RF who could comment on which unit makes the most sense. Personally, I have not encountered increment smaller than 0.1 dBm. 0.01 dBm is such a small quantity that most likely is not usable in real-life radio applications

It is indeed a very insignificant unit at lower powers. However, if you go to many dBms, it becomes more and more significant if you convert it to watts. 10dBm=10mW, 20dBm=100mW, 30dBm=1W, 40dBm=10W, 50dBm=100W. But its up to us to decide whether it makes sense to use 100W LoRa... since usually you are limited, even legally to 500mW.

Also to add more to the insignificance. Even if you theoretically could adjust per 0.1dBm, multipath, fading and other RF interfering factors would make such tiny adjustments useless. But considering we got 32bit... 0.01dBm is probably more than enough 🤔

Okay, I can implement this using 0.01dBm then? I will open another PR afterwards to correct the other drivers which use this command to use the updated units, since it appears the sentiment is that this would be okay.

honestly, sounds good to me.

drivers/wireless/lpwan/rn2xx3/rn2xx3.c

radio transceivers. This initial support includes transmit and receive functionality and configuration and reading of radio parameters like frequency and bandwidth. Signed-off-by: Matteo Golin <[email protected]>

raiden00pl · 2025-02-18T09:14:10Z

I think we should put some effort into the common API for LORA chips. Here is another PR with LORA chip #15795 and it would be good if all LORA chips use the same API.

I don't know if it's even possible because I'm not familiar with these chips, but we want to avoid a situation like in legacy sensors where each sensor has a different interface.

This'll most likely introduce some breaking changes for sx127xx but it's fully justified.

keever50 · 2025-02-18T09:22:14Z

I support this idea

linguini1 · 2025-02-18T19:11:13Z

I think we should put some effort into the common API for LORA chips. Here is another PR with LORA chip #15795 and it would be good if all LORA chips use the same API.

I like this idea as well. I believe the RN2XX3 chips both use an SX* chip under the hood, they just come with the impedance matching network and an MCU for handling the UART interface and LoRaWAN stuff. I think all the new WLIOC commands introduced in this PR should be compatible with the other PR for that reason.

However, I do think introducing this new API requires some conversation and work with many other developers. This seems outside the scope of this PR. Would that delay the merge of this PR or would it be something implemented afterward as a work in progress?

raiden00pl · 2025-02-23T08:34:02Z

However, I do think introducing this new API requires some conversation and work with many other developers. This seems outside the scope of this PR. Would that delay the merge of this PR or would it be something implemented afterward as a work in progress?

what about marking this driver as EXPERIMENTAL? to make it clear that in the future there may be some breaking changes in the API

keever50 · 2025-02-23T08:38:28Z

Good idea. The same can be done to my pr.
I really just want these to be pushed so that's out of the way now

linguini1 · 2025-02-23T16:43:31Z

However, I do think introducing this new API requires some conversation and work with many other developers. This seems outside the scope of this PR. Would that delay the merge of this PR or would it be something implemented afterward as a work in progress?

what about marking this driver as EXPERIMENTAL? to make it clear that in the future there may be some breaking changes in the API

Sure, I'll mark this in the docs and code comments.

github-actions bot added Area: Documentation Improvements or additions to documentation Area: Drivers Drivers issues Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Feb 13, 2025