[riot-notifications] [RIOT-OS/RIOT] CC1312R Sub-GHz netdev driver (Proof of Concept) (#13295)

benpicco notifications at github.com
Mon May 4 22:19:23 CEST 2020


@benpicco commented on this pull request.

First round of comments

> @@ -0,0 +1,3 @@
+ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))

```suggestion
ifneq (,$(filter netdev_default,$(USEMODULE)))
```

> @@ -0,0 +1,3 @@
+ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))

```suggestion
ifneq (,$(filter netdev_default,$(USEMODULE)))
```

> +#include "cc26xx_cc13xx_rfc_common_cmd.h"
+#include "cc26xx_cc13xx_rfc_mailbox.h"
+#include "cc26xx_cc13xx_rfc_queue.h"
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+
+/**
+ * @brief   Initialize RF driver
+ *
+ * @param[in] radio_setup  The radio setup command.
+ * @param[in] cpe_patch_fn CPE patch function, can be NULL.
+ * @param[in] handler_cb   IRQ handler.
+ */
+void rfc_init(rfc_op_t *radio_setup, void (* cpe_patch_fn)(void),

```suggestion
void cc26x2_cc13x2_rfc_init(rfc_op_t *radio_setup, void (* cpe_patch_fn)(void),
```
?

> + * @brief   Initialize RF driver
+ *
+ * @param[in] radio_setup  The radio setup command.
+ * @param[in] cpe_patch_fn CPE patch function, can be NULL.
+ * @param[in] handler_cb   IRQ handler.
+ */
+void rfc_init(rfc_op_t *radio_setup, void (* cpe_patch_fn)(void),
+              void ( *handler_cb)(void));
+
+/**
+ * @brief   Enable radio.
+ *
+ * @return 0 on success.
+ * @return -1 on failure.
+ */
+int rfc_enable(void);

```suggestion
int cc26x2_cc13x2_rfc_enable(void);
```

> +#include "cc26x2_cc13x2_rfc.h"
+#include "cc26xx_cc13xx_rfc_mailbox.h"
+#include "cc26xx_cc13xx_rfc_prop_mailbox.h"
+#include "cc26xx_cc13xx_rfc_queue.h"
+#include "vendor/rf_patch_cpe_prop.h"
+
+#define ENABLE_DEBUG (0)
+#include "debug.h"
+
+#include "cpu.h"
+
+#define TX_BUF_SIZE (144)
+
+static uint8_t _tx_buf0[TX_BUF_SIZE] __attribute__ ((aligned(4)));
+
+#define RX_BUF_SIZE (144)

Why 144?

> +static uint8_t _rx_buf0[RX_BUF_SIZE] __attribute__ ((aligned(4)));
+static uint8_t _rx_buf1[RX_BUF_SIZE] __attribute__ ((aligned(4)));
+static uint8_t _rx_buf2[RX_BUF_SIZE] __attribute__ ((aligned(4)));
+static uint8_t _rx_buf3[RX_BUF_SIZE] __attribute__ ((aligned(4)));

I think this would look nicer as `static uint8_t _rx_buf[4][RX_BUF_SIZE];`, should be word-aligned by default too.


> +    if (_is_in_tx()) {
+        DEBUG_PUTS("_send: already in TX");
+        return -EAGAIN;
+    }

Unfortunately you have to come up with some way of blocking here until TX is possible, as handling this is not yet implemented in the layer above.
You will probably experience issues sending fragmented packets (`ping6 -s 200 …`)

See #13943, #11263

> +
+    /* https://tools.ietf.org/html/rfc4944#section-12
+     * Requires the first bit to 0 for unicast addresses */
+    netdev->netdev.short_addr[1] &= 0x7F;
+
+    /* Initialize data entries */
+    rfc_data_entry_gen_init(_rx_buf0, sizeof(_rx_buf0), sizeof(uint16_t), _rx_buf1);
+    rfc_data_entry_gen_init(_rx_buf1, sizeof(_rx_buf1), sizeof(uint16_t), _rx_buf2);
+    rfc_data_entry_gen_init(_rx_buf2, sizeof(_rx_buf2), sizeof(uint16_t), _rx_buf3);
+    rfc_data_entry_gen_init(_rx_buf3, sizeof(_rx_buf3), sizeof(uint16_t), _rx_buf0);
+
+    /* Initialize RX data queue */
+    rfc_data_queue_init(&_rx_queue, _rx_buf0);
+
+    /* Tune the radio to the given frequency */
+    rf_cmd_fs.frequency = 915;

This should probably be a define.

> +    (uint32_t)0x00F788D3,
+    /* override_tc146.xml
+     * Tx: Configure PA ramp time, PACTL2.RC=0x3 (in ADI0, set PACTL2[4:3]=0x3)
+     */
+    RFC_ADI_2HALFREG_OVERRIDE(0, 16, 0x8, 0x8, 17, 0x1, 0x1),
+    /* Tx: Configure PA ramping, set wait time before turning off (0x1A ticks
+     of 16/24 us = 17.3 us).
+     */
+    RFC_HW_REG_OVERRIDE(0x6028, 0x001A),
+    /* Rx: Set AGC reference level to 0x16 (default: 0x2E) */
+    RFC_HW_REG_OVERRIDE(0x609C, 0x0016),
+    /* Rx: Set RSSI offset to adjust reported RSSI by -1 dB (default: -2),
+     * trimmed for external bias and differential configuration */
+    (uint32_t)0x000188A3,
+    /* Rx: Set anti-aliasing filter bandwidth to 0x8 (in ADI0, set
+     * IFAMPCTL3[7:4]=0x8) */
+    RFC_ADI_HALFREG_OVERRIDE(0, 61, 0xF, 0x8),
+    /* TX power override
+     * Tx: Set PA trim to max to maximize its output power (in ADI0,
+     * set PACTL0=0xF8) */
+    RFC_ADI_REG_OVERRIDE(0, 12, 0xF8),

Can you split those magic values in defines for the respective bits?
That would probably also help when configuring different settings.

> +    .command_no = 0x3807,
+    .status = 0x0000,
+    .next_op = NULL, /* set by us */
+    .start_time = 0x00000000,
+    .start_trigger.type = RFC_TRIG_NOW,
+    .start_trigger.ena_cmd = 0x0,
+    .start_trigger.trigger_no = 0x0,
+    .start_trigger.past_trig = 0x0,
+    .condition.rule = 0x1,
+    .condition.skip_no = 0x0,
+    .modulation.mod_type = 0x1,
+    .modulation.deviation = 0xC8,
+    .modulation.deviation_step_sz = 0x0,
+    .symbol_rate.prescale = 0xF,
+    .symbol_rate.rate_word = 0x20000,
+    .symbol_rate.decim_mode = 0x0,
+    .rx_bw = 0x59,
+    .pream_conf.pream_bytes = 0x7,
+    .pream_conf.pream_mode = 0x0,
+    .format_conf.sw_bits = 0x18,
+    .format_conf.bit_reversal = 0x0,
+    .format_conf.msb_first = 0x1,
+    .format_conf.fec_mode = 0x0,
+    .format_conf.whiten_mode = 0x7,
+    .config.front_end_mode = 0x0,
+    .config.bias_mode = 0x1,
+    .config.analog_cfg_mode = 0x0,
+    .config.no_fs_powerup = 0x0,
+    .tx_power = 0x04C0,
+    .reg_override = rf_prop_overrides,
+    .center_freq = 0x0393,
+    .int_freq = 0x0999,
+    .lo_divider = 0x05

You have already documented quite well what the individual bits mean in `rfc_cmd_prop_radio_div_setup_t` - so why not spell them out as macros so the reader is spared from looking up what the individual numbers mean.

I'm also wondering why you put them into linked lists instead of a static array - that might make the code a bit simpler, but I just skimmed it.

> +    .pkt_conf.crc_inc_hdr = 0x0,
+    .num_hdr_bits = 0x10,
+    .pkt_len = 0x0014,
+    .start_conf.ext_tx_trig = 0x0,
+    .start_conf.input_mode = 0x0,
+    .start_conf.source = 0x0,
+    .pre_trigger.type = RFC_TRIG_NOW,
+    .pre_trigger.ena_cmd = 0x0,
+    .pre_trigger.trigger_no = 0x0,
+    .pre_trigger.past_trig = 0x0,
+    .pre_time = 0x00000000,
+    .sync_word = 0x0055904E,
+    .pkt = 0 /* set by us */
+};
+
+rfc_cmd_prop_rx_adv_t rf_cmd_prop_rx_adv =

What are those structs?
Please add a comment here and above.

> +
+    return 0;
+}
+
+static int _set(netdev_t *netdev, netopt_t opt, const void *val, size_t len)
+{
+    cc26x2_cc13x2_rf_netdev_t *dev = (cc26x2_cc13x2_rf_netdev_t *)netdev;
+    int res = -ENOTSUP;
+
+    if (dev == NULL) {
+        return -ENODEV;
+    }
+
+    switch (opt) {
+        case NETOPT_RX_END_IRQ:
+            if (len != sizeof(bool)) {

```suggestion
            if (len != sizeof(netopt_enable_t)) {
```

> +        case NETOPT_RX_END_IRQ:
+            if (len != sizeof(bool)) {
+                return -EINVAL;
+            }
+            else {
+                if (*(const bool *)val) {
+                    RFC_DBELL_NONBUF->RFCPEIEN |= CPE_IRQ_RX_ENTRY_DONE;
+                }
+                else {
+                    RFC_DBELL_NONBUF->RFCPEIEN &= ~CPE_IRQ_RX_ENTRY_DONE;
+                }
+            }
+            return sizeof(netopt_enable_t);
+
+        case NETOPT_TX_END_IRQ:
+            if (len != sizeof(bool)) {

```suggestion
            if (len != sizeof(netopt_enable_t)) {
```

> +
+/**
+ * @brief   Condition for running next operation
+ */
+typedef struct {
+    uint8_t rule:4; /**< Rule for how to proceed */
+    uint8_t skip_no:4; /**< Skip number + 1 if the rule involves skipping */
+} rfc_cond_t;
+
+/**
+ * @brief   General radio operation.
+ */
+typedef struct {
+    uint16_t command_no; /**< The command ID number */
+    uint16_t status; /**< An integer telling the status of the command. */
+    void *next_op; /**< Pointer to the next operation to run */

What's with those command chains?

> +    uint16_t command_no; /**< The command ID number */
+    uint16_t status; /**< An integer telling the status of the command. */
+    void *next_op; /**< Pointer to the next operation to run */

You can have a

```C
struct chain_cmd {
    uint16_t command_no; /**< The command ID number */
    uint16_t status; /**< An integer telling the status of the command. */
    void *next_op; /**< Pointer to the next operation to run */
};
```

that you include in all these sub-commands as the first elemet.

> +    uint8_t rule:4; /**< Rule for how to proceed */
+    uint8_t skip_no:4; /**< Skip number + 1 if the rule involves skipping */
+} rfc_cond_t;
+
+/**
+ * @brief   General radio operation.
+ */
+typedef struct {
+    uint16_t command_no; /**< The command ID number */
+    uint16_t status; /**< An integer telling the status of the command. */
+    void *next_op; /**< Pointer to the next operation to run */
+    rfc_ratmr_t start_time; /**< Absolute or relative start time */
+    rfc_trigger_t start_trigger; /**< Identification of the trigger that
+                                         starts the operation */
+    rfc_cond_t condition; /**< Condition for running next command */
+} __attribute__ ((aligned (4))) rfc_op_t;

4 should be the default alignment anyway

> +    uint8_t __dummy0; /**< Reserved, always write 0 */
+    uint8_t __dummy1; /**< Reserved */
+    uint8_t __dummy2; /**< Reserved */
+    uint16_t __dummy3; /**< Reserved */

Is this written to a register?

> +enum {
+    RFC_TRIG_NOW = 0, /**< Triggers immediately */
+    RFC_TRIG_NEVER = 1, /**< Never trigs */
+    RFC_TRIG_ABSTIME = 2, /**< Trigs at an absolute time */
+    RFC_TRIG_REL_SUBMIT = 3, /**< Trigs at a time relative to the command was
+                                  submitted */
+    RFC_TRIG_REL_START = 4, /**< Trigs at a time relative to the command
+                                 started */
+    RFC_TRIG_REL_PREVSTART = 5, /**< Trigs at a time relative to the previous
+                                     command in the chain started */
+    RFC_TRIG_REL_FIRSTSTART = 6, /**< Trigs at a time relative to the first
+                                      command in the chain started */
+    RFC_TRIG_REL_PREVEND = 7, /**< Trigs at a time relative to the previous
+                                   command in the chain ended */
+    RFC_TRIG_REL_EVT1 = 8, /**< Trigs at a time relative to the context defined
+                                "Event 1" */
+    RFC_TRIG_REL_EVT2 = 9, /**< Trigs at a time relative to the context defined
+                                "Event 2" */
+    RFC_TRIG_EXTERNAL = 10, /**< Trigs at an external event to the radio
+                                 timer */
+    RFC_TRIG_PAST_BM = 0x80, /**< Bitmask for setting past trig bit in order to
+                                  trig immediately if trigger happened in the
+                                  past */
+};

Minor nit: please align the comments :wink: 

-- 
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/13295#pullrequestreview-405212146
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20200504/29c06d04/attachment-0001.htm>


More information about the notifications mailing list