[riot-notifications] [RIOT-OS/RIOT] drivers/sx126x: Add support for Nucleo -WL55JC (#16579)

Leandro Lanzieri notifications at github.com
Fri Jun 25 10:35:13 CEST 2021


@leandrolanzieri commented on this pull request.

Some initial comments

> + *
+ * @file        board.c
+ * @brief       Board specific implementations for the Nucleo-wl55jc board
+ *
+ *
+ * @author      Akshai M <akshai.m at fu-berlin.de>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+
+#include "board.h"
+#include "periph/gpio.h"
+
+#define SUBGHZ_DEBUG                    0

This probably should be configurable.

> +    gpio_init(FE_CTRL1, GPIO_OUT);
+    gpio_init(FE_CTRL2, GPIO_OUT);
+    gpio_init(FE_CTRL3, GPIO_OUT);

Is it OK to always initialize these? Would it make sense to to it only when the radio driver is present?

> +#include "board.h"
+#include "periph/gpio.h"
+
+#define SUBGHZ_DEBUG                    0
+
+void board_init(void)
+{
+    /* initialize the CPU */
+    cpu_init();
+
+    /* Initialize the GPIO control for RF 3-port switch (SP3T) */
+    gpio_init(FE_CTRL1, GPIO_OUT);
+    gpio_init(FE_CTRL2, GPIO_OUT);
+    gpio_init(FE_CTRL3, GPIO_OUT);
+
+#if IS_ACTIVE(SUBGHZ_DEBUG)

```suggestion
    if (IS_ACTIVE(SUBGHZ_DEBUG)) {
```
?

>      return dev->channel;
 }
 
 void sx126x_set_channel(sx126x_t *dev, uint32_t freq)
 {
+    DEBUG("[sx126x.c] sx126x_get_channel \n");

Now that you are adding so many debug messages, for completeness I would try to print the values on the `set`s when possible.

> @@ -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)

On an offline discussion with @jia200x we concluded that in order to make this more generic, a sort of splitting of the driver would be the best. Having an SPI-specific part and another that applies to SOCs that integrate the radio. When specifying the ISR vector clearly `ifdef`ing the family is fine, but it is oddly specific to use it to avoid the compilation of the GPIO IRQ functionalities. Better go generic here.

> @@ -92,6 +92,23 @@ uint32_t sx126x_get_channel(const sx126x_t *dev);
  */
 void sx126x_set_channel(sx126x_t *dev, uint32_t freq);
 
+/**
+ * @brief   Converts symbol value to milliseconds.
+ *
+ * @param[in] dev                      Device descriptor of the driver
+ * @param[in] symbols                  Symbols
+ *
+ * @return Time for symbols in milliseconds
+ */
+static inline int sx126x_symbol_to_msec(sx126x_t *dev, uint16_t symbols)
+{
+    assert((dev->mod_params.bw <= 0x06) && dev->mod_params.bw >= 0x04);

Would be good also to `assert(dev)`

> + *
+ * @file        board.c
+ * @brief       Board specific implementations for the Nucleo-wl55jc board
+ *
+ *
+ * @author      Akshai M <akshai.m at fu-berlin.de>
+ *
+ * @}
+ */
+
+#include <stdio.h>
+
+#include "board.h"
+#include "periph/gpio.h"
+
+#define SUBGHZ_DEBUG                    0

And we should also properly document it

-- 
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-692575083
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210625/a3bc9e2e/attachment-0001.htm>


More information about the notifications mailing list