[riot-notifications] [RIOT-OS/RIOT] Jiminy Support for at86rfr2 transceiver (#9172)

benpicco notifications at github.com
Mon Sep 9 14:01:39 CEST 2019


benpicco commented on this pull request.

If you are still interested in that driver, I think we can get it merged.

But remove the additional debug code and check if some more code can be shared between at86rfr2 and at86rfr2xx. Especially the two separate code paths in `_recv()` will be hard to maintain.

The get/set rxsensitivity, txpower fuctions should also basically follow the same logic as the existing driver. Maybe the formulas for calculating the register values have changed, but the general logic / parameter checks should still be the same.

Also, a rebase is in order.

> @@ -164,6 +235,14 @@ void at86rf2xx_tx_exec(const at86rf2xx_t *dev)
 
     /* write frame length field in FIFO */
     at86rf2xx_sram_write(dev, 0, &(dev->tx_frame_len), 1);
+#if ENABLE_HEX_DUMP_TX
+    uint8_t len = dev->tx_frame_len;
+    uint8_t data[len];
+    memcpy( data, (void *)(AT86RF2XX_REG__TRXFBST), len);
+    puts("SENDING:");
+    od_hex_dump(data, len, OD_WIDTH_DEFAULT);
+#endif

I'd say remove that debug code, it's useful during development but just clutters the code afterwards.

> @@ -242,6 +257,16 @@ int16_t at86rf2xx_get_txpower(const at86rf2xx_t *dev)
 
 void at86rf2xx_set_txpower(const at86rf2xx_t *dev, int16_t txpower)
 {
+#ifdef MODULE_AT86RFR2
+    /* find the right configuration which is the nearest to the transmit power
+     * take the next value, it is the next smaller than the requested.
+     */
+    unsigned int i = 0;
+    while (tx_pow_to_dbm[i] > txpower) {

Is this really just necessary for AT86RFR2?
The dimensions of the array are the same for AT86RF233, AT86RF212B

>  }
 
 void at86rf2xx_set_rxsensitivity(const at86rf2xx_t *dev, int16_t rxsens)
 {
+#if MODULE_AT86RFR2
+    (void)dev;
+    /* Always set the treshhold which at least limits the requested value.
+    * requesting -50dBm will result in -48dBm */
+    rxsens = (rxsens - RSSI_BASE_VAL)/3;
+    if (rxsens >14){
+        rxsens =14;
+    }else if(rxsens <0){

`< MIN_RX_SENSITIVITY` ?

>  }
 
 void at86rf2xx_set_rxsensitivity(const at86rf2xx_t *dev, int16_t rxsens)
 {
+#if MODULE_AT86RFR2
+    (void)dev;
+    /* Always set the treshhold which at least limits the requested value.
+    * requesting -50dBm will result in -48dBm */
+    rxsens = (rxsens - RSSI_BASE_VAL)/3;
+    if (rxsens >14){

`> MAX_RX_SENSITIVITY`  ?

>  }
 
 void at86rf2xx_set_rxsensitivity(const at86rf2xx_t *dev, int16_t rxsens)
 {
+#if MODULE_AT86RFR2
+    (void)dev;
+    /* Always set the treshhold which at least limits the requested value.
+    * requesting -50dBm will result in -48dBm */
+    rxsens = (rxsens - RSSI_BASE_VAL)/3;
+    if (rxsens >14){
+        rxsens =14;
+    }else if(rxsens <0){
+        rxsens =0;
+    }else{
+        rxsens +=1;
+    }
+    *AT86RF2XX_REG__RX_SYN = (*AT86RF2XX_REG__RX_SYN & ~(AT86RF2XX_RX_SYN__RX_PDT_LEVEL)) | rxsens;

I have the feeling that everything but the calculation of `rxsens` could be shared with the !`MODULE_AT86RFR2` code (and you could also use the `at86rf2xx_reg_write()` path here).

That would make the `#ifdef` block much smaller.

> @@ -446,6 +497,16 @@ void at86rf2xx_set_option(at86rf2xx_t *dev, uint16_t option, bool state)
                           : (tmp & ~AT86RF2XX_IRQ_STATUS_MASK__RX_START);
             at86rf2xx_reg_write(dev, AT86RF2XX_REG__IRQ_MASK, tmp);
             break;
+#ifdef MODULE_AT86RFR2
+        case AT86RF2XX_OPT_TELL_TX_END:

Why is this only relevant for AT86RFR2?

> @@ -350,7 +413,7 @@ static int _get(netdev_t *netdev, netopt_t opt, void *val, size_t max_len)
             return sizeof(netopt_enable_t);
 
 /* Only radios with the XAH_CTRL_2 register support frame retry reporting */
-#if AT86RF2XX_HAVE_RETRIES
+#if AT86RF2XX_HAVE_RETRIES || MODULE_AT86RFR2

Just define `AT86RF2XX_HAVE_RETRIES` if `MODULE_AT86RFR2` is defined.

>          netdev->event_callback(netdev, NETDEV_EVENT_RX_STARTED);
         DEBUG("[at86rf2xx] EVT - RX_START\n");
     }
 
     if (irq_mask & AT86RF2XX_IRQ_STATUS_MASK__TRX_END) {
         if ((state == AT86RF2XX_STATE_RX_AACK_ON)
             || (state == AT86RF2XX_STATE_BUSY_RX_AACK)) {
+#ifdef MODULE_AT86RFR2
+            /* clear RX interrupt status*/
+            dev->irq_status &= ~AT86RF2XX_IRQ_STATUS_MASK__RX_END;

Are you sure you don't rather want to clear the interrupt in any `state`?

>      /* Only radios with the XAH_CTRL_2 register support frame retry reporting */
     uint8_t tx_retries;                 /**< Number of NOACK retransmissions */
+#endif
+#ifdef MODULE_AT86RFR2
+    /* ATmega256rfr2 signals transceiver events with different interrupts
+     * they have to be stored to mimic the same flow as external transceiver
+     * Use irq_status to map saved interrupts of SOC transceiver,
+     * as they clear after IRQ callback.
+     *
+     *  irq_status = IRQ_STATUS
+     *  irq_status = IRQ_STATUS1
+     * */
+    uint8_t irq_status;                     /**< save irq status */
+    uint8_t irq_status1;                    /**< save irq status1 */

`irq_status1` is unused.

> @@ -48,12 +55,34 @@ void at86rf2xx_setup(at86rf2xx_t *dev, const at86rf2xx_params_t *params)
     /* radio state is P_ON when first powered-on */
     dev->state = AT86RF2XX_STATE_P_ON;
     dev->pending_tx = 0;
+
+#ifdef MODULE_AT86RFR2
+    /* Store device pointer for interrupts */
+    at86rfr2_dev = (netdev_t *)dev;

Would be nicer if it were made `static` and you had a  `at86rfr2_init_isr(dev);` here.

> @@ -479,7 +540,9 @@ static inline void _set_state(at86rf2xx_t *dev, uint8_t state, uint8_t cmd)
      * in https://github.com/RIOT-OS/RIOT/pull/5244
      */
     if (state != AT86RF2XX_STATE_RX_AACK_ON) {
-        while (at86rf2xx_get_status(dev) != state) {}
+        while (at86rf2xx_get_status(dev) != state) {
+            DEBUG("_set_state: state is: 0x%02x\n", state);
+        }

Remove the additional debug output.

> @@ -499,6 +562,7 @@ uint8_t at86rf2xx_set_state(at86rf2xx_t *dev, uint8_t state)
      * in progress */
     do {
         old_state = at86rf2xx_get_status(dev);
+        DEBUG("at86rf2xx_set_state: 0x%02x old_state is: 0x%02x\n", state, old_state);

Remove the added debug output.

>  static inline void getbus(const at86rf2xx_t *dev)
 {
     spi_acquire(SPIDEV, CSPIN, SPI_MODE_0, dev->params.spi_clk);
 }
+#endif
+
+#ifdef MODULE_AT86RFR2
+void at86rf2xx_reg_write(const at86rf2xx_t *dev,

Maybe we can make those `static inline` directly in `at86rf2xx_internal.h` so the compiler can optimize them away entirely.

>      spi_transfer_bytes(SPIDEV, CSPIN, true, NULL, data, len);
+#endif
+
+#if ENABLE_HEX_DUMP_RX
+    puts("RECEIVED:");
+    od_hex_dump(data, len, OD_WIDTH_DEFAULT);
+#endif

Remove the added debug code.

>      size_t pkt_len;
 
+#ifdef MODULE_AT86RFR2
+    /* get the size of the received packet */
+    /* Transceiver Received Frame Length Register (refer p.35, 85, 154) */
+    /* subtract length of FCS */
+    pkt_len = (TST_RX_LENGTH & 0x7f) - 2;

Where is `TST_RX_LENGTH` defined?

Also, is the receive procedure so different that you need an entirely different code path for `AT86RFR2`?

> +/**
+ * @brief ISR for transceiver's transmit end interrupt
+ *
+ *  Is triggered when data or when acknowledge frames where send.
+ *
+ * Flow Diagram Manual p. 52 / 63
+ */
+ISR(TRX24_TX_END_vect, ISR_BLOCK){
+    __enter_isr();
+
+#if DEBUG_ATRFR2_PINS
+    DEBUG_ATRFR2_PORT |= DEBUG_ATRFR2_PIN_TX_END;
+#endif
+    /* This is a short light pulse*/
+#if RXTX_LED_ENABLE
+    RXTX_LED_ON;

You can save the #ifdef and just write

` RXTX_LED_ON;`

as you can just

```C
#if RXTX_LED_ENABLE
#define  RXTX_LED_ON LED0_ON
#else 
#define  RXTX_LED_ON
#endif 
```

But this seems more like a debug feature that should be removed / put into a separate commit.

-- 
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/9172#pullrequestreview-285440037
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190909/db4247d4/attachment.htm>


More information about the notifications mailing list