<p></p>
<p><b>@benpicco</b> commented on this pull request.</p>

<p>First round of comments</p><hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419616544">boards/cc1312-launchpad/Makefile.dep</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,3 @@
+ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))
</pre>
⬇️ Suggested change
<pre style="color: #555">-ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))
+ifneq (,$(filter netdev_default,$(USEMODULE)))
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419616661">boards/cc1352p-launchpad/Makefile.dep</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,3 @@
+ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))
</pre>
⬇️ Suggested change
<pre style="color: #555">-ifneq (,$(filter gnrc_netdev_default,$(USEMODULE)))
+ifneq (,$(filter netdev_default,$(USEMODULE)))
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419666729">cpu/cc26x2_cc13x2/include/cc26x2_cc13x2_rfc.h</a>:</p>
<pre style='color:#555'>> +#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),
</pre>
⬇️ Suggested change
<pre style="color: #555">-void rfc_init(rfc_op_t *radio_setup, void (* cpe_patch_fn)(void),
+void cc26x2_cc13x2_rfc_init(rfc_op_t *radio_setup, void (* cpe_patch_fn)(void),
</pre>

<p>?</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419667152">cpu/cc26x2_cc13x2/include/cc26x2_cc13x2_rfc.h</a>:</p>
<pre style='color:#555'>> + * @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);
</pre>
⬇️ Suggested change
<pre style="color: #555">-int rfc_enable(void);
+int cc26x2_cc13x2_rfc_enable(void);
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419683076">cpu/cc26x2_cc13x2/radio/rf_netdev.c</a>:</p>
<pre style='color:#555'>> +#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)
</pre>
<p>Why 144?</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419683702">cpu/cc26x2_cc13x2/radio/rf_netdev.c</a>:</p>
<pre style='color:#555'>> +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)));
</pre>
<p>I think this would look nicer as <code>static uint8_t _rx_buf[4][RX_BUF_SIZE];</code>, should be word-aligned by default too.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419686527">cpu/cc26x2_cc13x2/radio/rf_netdev.c</a>:</p>
<pre style='color:#555'>> +    if (_is_in_tx()) {
+        DEBUG_PUTS("_send: already in TX");
+        return -EAGAIN;
+    }
</pre>
<p>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.<br>
You will probably experience issues sending fragmented packets (<code>ping6 -s 200 …</code>)</p>
<p>See <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="606435613" data-permission-text="Title is private" data-url="https://github.com/RIOT-OS/RIOT/issues/13943" data-hovercard-type="pull_request" data-hovercard-url="/RIOT-OS/RIOT/pull/13943/hovercard" href="https://github.com/RIOT-OS/RIOT/pull/13943">#13943</a>, <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="424980787" data-permission-text="Title is private" data-url="https://github.com/RIOT-OS/RIOT/issues/11263" data-hovercard-type="pull_request" data-hovercard-url="/RIOT-OS/RIOT/pull/11263/hovercard" href="https://github.com/RIOT-OS/RIOT/pull/11263">#11263</a></p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419688358">cpu/cc26x2_cc13x2/radio/rf_netdev.c</a>:</p>
<pre style='color:#555'>> +
+    /* 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;
</pre>
<p>This should probably be a define.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419689850">cpu/cc26x2_cc13x2/radio/rf_commands.c</a>:</p>
<pre style='color:#555'>> +    (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),
</pre>
<p>Can you split those magic values in defines for the respective bits?<br>
That would probably also help when configuring different settings.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419691454">cpu/cc26x2_cc13x2/radio/rf_commands.c</a>:</p>
<pre style='color:#555'>> +    .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
</pre>
<p>You have already documented quite well what the individual bits mean in <code>rfc_cmd_prop_radio_div_setup_t</code> - so why not spell them out as macros so the reader is spared from looking up what the individual numbers mean.</p>
<p>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.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419697480">cpu/cc26x2_cc13x2/radio/rf_commands.c</a>:</p>
<pre style='color:#555'>> +    .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 =
</pre>
<p>What are those structs?<br>
Please add a comment here and above.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419697981">cpu/cc26x2_cc13x2/radio/rf_netdev.c</a>:</p>
<pre style='color:#555'>> +
+    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)) {
</pre>
⬇️ Suggested change
<pre style="color: #555">-            if (len != sizeof(bool)) {
+            if (len != sizeof(netopt_enable_t)) {
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419698064">cpu/cc26x2_cc13x2/radio/rf_netdev.c</a>:</p>
<pre style='color:#555'>> +        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)) {
</pre>
⬇️ Suggested change
<pre style="color: #555">-            if (len != sizeof(bool)) {
+            if (len != sizeof(netopt_enable_t)) {
</pre>


<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419699462">cpu/cc26xx_cc13xx/include/cc26xx_cc13xx_rfc_common_cmd.h</a>:</p>
<pre style='color:#555'>> +
+/**
+ * @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 */
</pre>
<p>What's with those command chains?</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419700807">cpu/cc26xx_cc13xx/include/cc26xx_cc13xx_rfc_common_cmd.h</a>:</p>
<pre style='color:#555'>> +    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 */
</pre>
<p>You can have a</p>
<div class="highlight highlight-source-c"><pre><span class="pl-k">struct</span> chain_cmd {
    <span class="pl-c1">uint16_t</span> command_no; <span class="pl-c"><span class="pl-c">/*</span>*< The command ID number <span class="pl-c">*/</span></span>
    <span class="pl-c1">uint16_t</span> status; <span class="pl-c"><span class="pl-c">/*</span>*< An integer telling the status of the command. <span class="pl-c">*/</span></span>
    <span class="pl-k">void</span> *next_op; <span class="pl-c"><span class="pl-c">/*</span>*< Pointer to the next operation to run <span class="pl-c">*/</span></span>
};</pre></div>
<p>that you include in all these sub-commands as the first elemet.</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419700944">cpu/cc26xx_cc13xx/include/cc26xx_cc13xx_rfc_common_cmd.h</a>:</p>
<pre style='color:#555'>> +    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;
</pre>
<p>4 should be the default alignment anyway</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419701483">cpu/cc26xx_cc13xx/include/cc26xx_cc13xx_rfc_common_cmd.h</a>:</p>
<pre style='color:#555'>> +    uint8_t __dummy0; /**< Reserved, always write 0 */
+    uint8_t __dummy1; /**< Reserved */
+    uint8_t __dummy2; /**< Reserved */
+    uint16_t __dummy3; /**< Reserved */
</pre>
<p>Is this written to a register?</p>

<hr>

<p>In <a href="https://github.com/RIOT-OS/RIOT/pull/13295#discussion_r419701917">cpu/cc26xx_cc13xx/include/cc26xx_cc13xx_rfc_mailbox.h</a>:</p>
<pre style='color:#555'>> +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 */
+};
</pre>
<p>Minor nit: please align the comments <g-emoji class="g-emoji" alias="wink" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f609.png">😉</g-emoji></p>

<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/RIOT-OS/RIOT/pull/13295#pullrequestreview-405212146">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABE7WYAM3EYLE2FGJX2ISN3RP4PMXANCNFSM4KQKU5HQ">unsubscribe</a>.<img src="https://github.com/notifications/beacon/ABE7WYCRAKET5NU2F5JHDODRP4PMXA5CNFSM4KQKU5H2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGODATQX4Q.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/RIOT-OS/RIOT/pull/13295#pullrequestreview-405212146",
"url": "https://github.com/RIOT-OS/RIOT/pull/13295#pullrequestreview-405212146",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>