Jiminy Support for at86rfr2 transceiver by Josar · Pull Request #9172 · RIOT-OS/RIOT

Josar · 2018-05-23T19:59:07Z

This PR adds support for the at86rfr2 transceiver into the at86rf2xx driver.

This is split out from #8742

Josar · 2018-05-29T11:34:21Z

@kYc0o maybe this could be a review where you could help out as you could test it on the RCB256RFR2-xpro. 😁

PeterKietzmann · 2018-05-29T16:30:16Z

I don't want / can't hop into the review process so late. Just wanted to inform that I can test on the Jiminy hardware once we are at that point.

kYc0o · 2018-05-29T21:22:29Z

Yes, but first I'd like to get #9130 in. Afterwards I want to add support for that board, then I can test this.

If the guys at HAW who have the jiminy board can test it before it's also OK.

Josar · 2018-06-15T08:37:40Z

@gebart could you find some time for this?

kYc0o

First round of review.

I'd like to have a more consistent use of the register reading and writing, maybe using at86rf2xx_reg_write/read a bit more extensively.

I'll try to test but I think I need first to see if the at86rf2xx driver is not broken with this PR.

kYc0o · 2018-05-29T21:23:21Z

boards/jiminy-mega256rfr2/include/board.h

- *  76923
- *  38400
+ *  76800
+ *  38400 fetching data from interrupt to slow for long data


What this comment is about? It's confusing.

kYc0o · 2018-07-05T11:29:46Z

drivers/at86rf2xx/at86rf2xx.c

+#include "board.h"
+#define ENABLE_HEX_DUMP_TX  (0)
+#if ENABLE_HEX_DUMP_TX
+#include "od.h"


Why is this necessary?

To enable a hexdump at the lowest possible layer.

kYc0o · 2018-07-05T11:33:03Z

boards/jiminy-mega256rfr2/Makefile.dep

 USEMODULE += boards_common_atmega
+
+ifneq (, $(filter gnrc_netdev_default netdev_default, $(USEMODULE)))
+USEMODULE += at86rfr2


Please add 2 spaces indentation.

kYc0o · 2018-07-05T11:34:28Z

drivers/at86rf2xx/at86rf2xx.c

+
+#ifdef MODULE_AT86RFR2
+    /* Store device pointer for interrupts */
+    at86rfr2_dev = (netdev_t *)dev;


From where this variable comes? I can't find any usage.

drivers/at86rf2xx/include/at86rf2xx_internal.h

It is used in the interrupts.

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

kYc0o · 2018-07-05T11:34:54Z

drivers/at86rf2xx/at86rf2xx.c

+                              | DEBUG_ATRFR2_PIN_TX_END|DEBUG_ATRFR2_PIN_RX_END);
+    /* Pin Low */
+    DEBUG_ATRFR2_PORT     &= ~( DEBUG_ATRFR2_PIN_TX_START
+                              |DEBUG_ATRFR2_PIN_TX_END|DEBUG_ATRFR2_PIN_RX_END);


Are not this done anyways in enable_rxtx_led();? Also, I think this touches too at86rf2xx driver which doesn't have such pins.

kYc0o · 2018-07-05T12:38:17Z

drivers/at86rf2xx/at86rf2xx.c


+    /* clear interrupt flags, AT86RF233 Manual p.33*/
+    at86rf2xx_reg_read(dev, AT86RF2XX_REG__IRQ_STATUS);
+


drivers/at86rf2xx/at86rf2xx_getset.c

drivers/at86rf2xx/at86rf2xx_internal.c

Josar · 2018-07-06T11:31:40Z

#9508
#9509
#9510

@gebart @kYc0o could we get this merged before and rebase on them as this will require some rework of this PR.

kYc0o · 2018-10-29T13:07:21Z

@Josar you may rebase this PR for a final review.

Josar

This rx_sens_to_dbm seems to be new. Please see my implementation. One question which i had was if this arrays with values are needed.

Josar · 2018-10-29T15:26:08Z

@kYc0o squashed and rebased.

stale · 2019-08-10T03:08:09Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

benpicco

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.

benpicco · 2019-09-09T11:24:51Z

drivers/at86rf2xx/at86rf2xx.c

+    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.

benpicco · 2019-09-09T11:27:17Z

drivers/at86rf2xx/at86rf2xx_getset.c

+     * 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

benpicco · 2019-09-09T11:32:19Z

drivers/at86rf2xx/at86rf2xx_getset.c

+    rxsens = (rxsens - RSSI_BASE_VAL)/3;
+    if (rxsens >14){
+        rxsens =14;
+    }else if(rxsens <0){


< MIN_RX_SENSITIVITY ?

benpicco · 2019-09-09T11:32:36Z

drivers/at86rf2xx/at86rf2xx_getset.c

+    /* 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 ?

benpicco · 2019-09-09T11:34:34Z

drivers/at86rf2xx/at86rf2xx_getset.c

+    }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.

benpicco · 2019-09-09T11:44:56Z

drivers/at86rf2xx/at86rf2xx_getset.c

     * 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.

benpicco · 2019-09-09T11:45:48Z

drivers/at86rf2xx/at86rf2xx_internal.c

+#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.

benpicco · 2019-09-09T11:47:26Z

drivers/at86rf2xx/at86rf2xx_internal.c

+#if ENABLE_HEX_DUMP_RX
+    puts("RECEIVED:");
+    od_hex_dump(data, len, OD_WIDTH_DEFAULT);
+#endif


Remove the added debug code.

benpicco · 2019-09-09T11:50:14Z

drivers/at86rf2xx/at86rf2xx_netdev.c

+    /* 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?

benpicco · 2019-09-09T11:56:56Z

drivers/at86rf2xx/at86rfr2_interrupts.c

+#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

#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.

benpicco · 2019-10-09T22:48:20Z

I now have a board with this radio #12411
It's much cheaper than the official dev boards too.

benpicco · 2019-10-28T08:32:46Z

Superseded by #12537

Josar mentioned this pull request May 23, 2018

Jiminy Support for hwrng and at86rfr2 transceiver #8742

Closed

PeterKietzmann assigned jnohlgard May 29, 2018

PeterKietzmann requested review from jnohlgard and kYc0o May 29, 2018 16:28

bergzand mentioned this pull request Jul 3, 2018

tests/monocypher: disable TEST_ON_CI_WHITELIST #9486

Closed

kYc0o requested changes Jul 5, 2018

View reviewed changes

Josar mentioned this pull request Jul 6, 2018

at86rf2xx: release framebuffer on recv with (len > 0) && (buf == NULL) #9509

Merged

This was referenced Jul 17, 2018

at86rf2xx: uncrustify and changed parenthesis order of NETOPT_CSMA_RETRIES check #9508

Merged

at86rf2xx: correct rssi #9510

Merged

Josar force-pushed the pr/rfr2 branch from aae25fe to a073584 Compare October 29, 2018 15:17

Josar commented Oct 29, 2018

View reviewed changes

RIOT-OS deleted a comment Oct 30, 2018

drivers/../at86rf2xx.c: reorder code

78164fe

Josar force-pushed the pr/rfr2 branch from bd14dfc to 05082f3 Compare December 18, 2018 12:23

drivers/at86rf2xx: atmega256rfr2 support

671a4b3

Josar force-pushed the pr/rfr2 branch from 05082f3 to 671a4b3 Compare December 18, 2018 13:27

stale bot added the State: stale State: The issue / PR has no activity for >185 days label Aug 10, 2019

benpicco mentioned this pull request Sep 9, 2019

boards: Remove support for the Jiminy-Mega256RFR2 #12182

Merged

benpicco reviewed Sep 9, 2019

View reviewed changes

stale bot removed the State: stale State: The issue / PR has no activity for >185 days label Sep 9, 2019

benpicco mentioned this pull request Oct 9, 2019

boards/microduino-corerf: Initial support #12411

Merged

benpicco mentioned this pull request Oct 21, 2019

drivers/at86rf2xx: add support for ATmegaRF MCUs #12537

Merged

benpicco closed this Oct 28, 2019

benpicco mentioned this pull request Jul 23, 2020

drivers/at86rf2xx: support frame retry reporting on AT86RFR2 #14596

Merged


		/* clear interrupt flags, AT86RF233 Manual p.33*/
		at86rf2xx_reg_read(dev, AT86RF2XX_REG__IRQ_STATUS);

Conversation

Josar commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Josar commented May 29, 2018

Uh oh!

PeterKietzmann commented May 29, 2018

Uh oh!

kYc0o commented May 29, 2018

Uh oh!

Josar commented Jun 15, 2018

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Josar commented Jul 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kYc0o commented Oct 29, 2018

Uh oh!

Josar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Josar commented Oct 29, 2018

Uh oh!

stale bot commented Aug 10, 2019

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco commented Oct 9, 2019

Uh oh!

benpicco commented Oct 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Josar commented May 23, 2018 •

edited

Loading

Josar commented Jul 6, 2018 •

edited

Loading

Josar left a comment •

edited

Loading