[riot-notifications] [RIOT-OS/RIOT] Add LoRa Radio support for Nucleo -WL55JC (#16579)

José Alamos notifications at github.com
Thu Jun 24 11:01:48 CEST 2021


@jia200x requested changes on this pull request.

Here's a list of comments.
Overall it looks fine, although it's important to make sure both the "periph" and external version still work and that Semtech LoRaMAC + GNRC LoRaWAN run as expected.

> @@ -70,6 +77,23 @@ extern "C" {
 #define BTN2_MODE           GPIO_IN_PU
 /** @} */
 
+/**
+ * @name    RF 3-port switch (SP3T) control
+ *
+ * Refer Section 6.6.3 RF Overview in User Manual (UM2592)
+ * @{
+ */
+#define FE_CTRL1            GPIO_PIN(PORT_C, 4)
+#define FE_CTRL2            GPIO_PIN(PORT_C, 5)
+#define FE_CTRL3            GPIO_PIN(PORT_C, 3)
+/** @} */
+
+/**
+ * @brief Initialize board specific hardware
+ *

```suggestion
```

> -#define CONFIG_USE_CLOCK_PLL            0
+#define CONFIG_USE_CLOCK_PLL 0
 #else
-#define CONFIG_USE_CLOCK_PLL            1     /* Use PLL by default */
+#define CONFIG_USE_CLOCK_PLL 1 /* Use PLL by default */

Unrelated change

>  static void _dio1_isr(void *arg)
 {
     netdev_trigger_event_isr((netdev_t *)arg);
 }
+#endif
+
+uint8_t sx126x_symbol_to_msec(sx126x_t *dev, uint16_t symbols)

```suggestion
static int sx126x_symbol_to_msec(sx126x_t *dev, uint16_t symbols)
```

This one can be static inline. Also, `uint8_t` will easily overflow

> @@ -148,6 +166,7 @@ int sx126x_init(sx126x_t *dev)
 
     DEBUG("[sx126x] init: SPI_%i initialized with success\n", dev->params->spi);
 
+#if !defined(CPU_FAM_STM32WL)

Could we model this with something like "PERIPH_SX126X" instead of hardcoding the CPU family?

>      /* Reset the device */
     sx126x_reset(dev);
 
+#if defined(BOARD_NUCLEO_WL55JC)

there shouldn't be board dependencies in the driver. As described above, you can use a module if you really need to run these lines

> @@ -30,6 +30,10 @@
 #include "sx126x.h"
 #include "sx126x_netdev.h"
 
+#if defined(BOARD_NUCLEO_WL55JC)

There shouldn't be a board dependency in the driver

> @@ -39,6 +43,19 @@
 #define SX126X_MAX_SF       LORA_SF12
 #endif
 
+static netdev_t *_dev;
+
+#if defined(CPU_FAM_STM32WL)

ditto

> @@ -39,6 +43,19 @@
 #define SX126X_MAX_SF       LORA_SF12
 #endif
 
+static netdev_t *_dev;
+
+#if defined(CPU_FAM_STM32WL)
+void isr_subghz_radio(void)
+{
+    /* Disable NVIC to avoid ISR conflict in CPU. Please note this does not
+        clear the interrupts not disable IRQ masks in radio*/
+    NVIC_DisableIRQ(SUBGHZ_Radio_IRQn);

Instead of disabling IRQs, you can simply clear the IRQ flags.

> @@ -51,28 +68,29 @@ static int _send(netdev_t *netdev, const iolist_t *iolist)
         return -ENOTSUP;
     }
 
-    uint8_t size = iolist_size(iolist);

here and below: This was already fixed in https://github.com/RIOT-OS/RIOT/pull/16570. Maybe we should base on that one

> @@ -106,13 +124,13 @@ static int _recv(netdev_t *netdev, void *buf, size_t len, void *info)
     }
 
     sx126x_read_buffer(dev, rx_buffer_status.buffer_start_pointer, buf, size);
-
-    return 0;
+    return size;

same as above

>  }
 
 static int _init(netdev_t *netdev)
 {
     sx126x_t *dev = (sx126x_t *)netdev;
+    _dev = netdev;

this is only required in the "periph" version. Maybe we should keep it inside #ifdef... #endif?

> +#if defined(CPU_FAM_STM32WL)
+    NVIC_EnableIRQ(SUBGHZ_Radio_IRQn);
+#endif

Same as above. You don't need to do this if you clear the interrupts.

> +#if defined(BOARD_NUCLEO_WL55JC)
+        gpio_set(FE_CTRL1);
+        gpio_clear(FE_CTRL2);
+        gpio_set(FE_CTRL3);

should we maybe use a weak symbol here and implement board specific stuff in "board.h"? If more boards do the same, it will start growing like crazy

>          sx126x_cfg_rx_boosted(dev, true);
-        if (dev->rx_timeout != 0) {
-            sx126x_set_rx(dev, dev->rx_timeout);
+        /* Get the multiplier for timeout */
+        const uint32_t _timeout_xr = ((sx126x_symbol_to_msec(dev, dev->rx_timeout) * 100 ) \
+                                       / 15.625);
+        if (_timeout_xr != 0) {
+            sx126x_set_rx(dev, _timeout_xr);

Note this function receives the timeout in `ms`:

```
/**
 * @brief Set the chip in reception mode
 *
 * @remark The packet type shall be configured with @ref sx126x_set_pkt_type before using this command.
 *
 * @remark By default, the chip returns automatically to standby RC mode as soon as a packet is received
 * or if no packet has been received before the timeout. This behavior can be altered by @ref
 * sx126x_set_rx_tx_fallback_mode.
 *
 * @remark The timeout argument can have the following special values:
 *
 * | Special values        | Meaning                                                                               |
 * | ----------------------| --------------------------------------------------------------------------------------|
 * | SX126X_RX_SINGLE_MODE | Single: the chip stays in RX mode until a reception occurs, then switch to standby RC |
 *
 * @param [in] context Chip implementation context.
 * @param [in] timeout_in_ms The timeout configuration in millisecond for Rx operation
 *
 * @returns Operation status
 */
sx126x_status_t sx126x_set_rx( const void* context, const uint32_t timeout_in_ms );
```

Therefore it should contain the output of "sx126x_symbol_to_msec"

> @@ -299,6 +329,12 @@ static int _set_state(sx126x_t *dev, netopt_state_t state)
 
     case NETOPT_STATE_TX:
         DEBUG("[sx126x] netdev: set NETOPT_STATE_TX state\n");
+#if defined(BOARD_NUCLEO_WL55JC)

ditto

> @@ -396,7 +432,7 @@ static int _set(netdev_t *netdev, netopt_t opt, const void *val, size_t len)
             res = -EINVAL;
             break;
         }
-        sx126x_set_tx_params(dev, power, SX126X_RAMP_10_US);
+        sx126x_set_tx_params(dev, power, SX126X_RAMP_40_US);

why?

>  
 #define SX126X_SPI_SPEED    (SPI_CLK_1MHZ)
 #define SX126X_SPI_MODE     (SPI_MODE_0)
 
+#if defined(CPU_FAM_STM32WL)

ditto

> -    spi_transfer_bytes(dev->params->spi, dev->params->nss_pin, data_length != 0, command, NULL,
-                       command_length);
+    spi_transfer_bytes(dev->params->spi, dev->params->nss_pin, data_length != 0,
+                       command, NULL, command_length);

unrelated change

> +        spi_transfer_bytes(dev->params->spi, dev->params->nss_pin, false, data,
+                           NULL, data_length);

unrelated change

> -#define ENABLE_DEBUG 0
+#define ENABLE_DEBUG 1

unrelated change

> -#define ENABLE_DEBUG      0
+#define ENABLE_DEBUG      1

unrelated change

> -#define ENABLE_DEBUG 0
+#define ENABLE_DEBUG 1

unrelated change

> -#define ENABLE_DEBUG 0
+#define ENABLE_DEBUG 1

unrelated change

>      return 0;
 }
 
 sx126x_hal_status_t sx126x_hal_wakeup(const void *context)
 {
     DEBUG("[sx126x_hal] wakeup\n");
+#if defined(CPU_FAM_STM32WL)
+    (void)context;
+    /* Pull NSS low */
+    PWR->SUBGHZSPICR &= ~PWR_SUBGHZSPICR_NSS;
+    ztimer_sleep(ZTIMER_USEC, 1000);

```suggestion
    ztimer_sleep(ZTIMER_USEC, SX126X_PERIPH_WAKEUP_TIME);
```

And please define it above.

> @@ -178,6 +204,7 @@ int sx126x_init(sx126x_t *dev)
         SX126X_IRQ_TX_DONE |
         SX126X_IRQ_RX_DONE |
         SX126X_IRQ_PREAMBLE_DETECTED |
+        SX126X_IRQ_SYNC_WORD_VALID |

is this event reflected in the netdev events? if not, I would say we can simply drop it.

>  #endif
 
 #ifndef CONFIG_SX126X_RAMP_TIME_DEFAULT
-#define CONFIG_SX126X_RAMP_TIME_DEFAULT         (SX126X_RAMP_10_US)

why?

> @@ -92,6 +92,16 @@ uint32_t sx126x_get_channel(const sx126x_t *dev);
  */
 void sx126x_set_channel(sx126x_t *dev, uint32_t freq);
 
+/**

as stated above, this can be static inline (and should return int)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/16579#pullrequestreview-691480255
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210624/cf185d3a/attachment-0001.htm>


More information about the notifications mailing list