drivers/radios Apply register to rssi dBm conversion to all radios by bergzand · Pull Request #6895 · RIOT-OS/RIOT

bergzand · 2017-04-12T08:16:28Z

Implementation idea of #6887. Should be easy to convert to absolute positive values if preferred.

Hyungsin · 2017-08-28T16:55:59Z

@bergzand, is it possible for you to change the implementation according to @gebart'comment in #6887 ?
Using int16_t for rssi seems to be good. Any concern about this change?

bergzand · 2017-08-29T16:12:05Z

@Hyungsin I've got no problem with that, but I'd like some kind of GO from somebody higher up in the hierarchy before I'm going to change and check most of the radio drivers

Hyungsin · 2017-08-29T17:31:55Z

@thomaseichinger, any feedback?

thomaseichinger · 2017-08-30T14:34:33Z

drivers/at86rf2xx/at86rf2xx_netdev.c

-        radio_info->rssi = at86rf2xx_reg_read(dev, AT86RF2XX_REG__PHY_ED_LEVEL);
+        rssi = at86rf2xx_reg_read(dev, AT86RF2XX_REG__PHY_ED_LEVEL);
 #endif
+        radio_info->rssi = -RSSI_BASE_VAL - rssi;


@bergzand Please help me out, what is the reason for changing the algebraic sign of this calculation? Especially as you are changing .rssi to int8_t ...

I forgot to change .rssi back to the original uint8_t. The sign here is flipped to have the value from the datasheet directly as a define. The sign is flipped to correct the algabraic formula from sec 8.5.3 from the datasheet of the at86rf233 to get a positive value.

thomaseichinger · 2017-08-30T14:35:51Z

drivers/cc2420/include/cc2420_registers.h

+ * @{
+ */
+#define CC2420_CRCCOR_CRC_MASK      (0x80)
+#define CC2420_CRCCOR_COR_MASK     (0x7F)


missing /** @} */

thomaseichinger · 2017-08-30T14:43:28Z

drivers/cc2420/cc2420.c

        /* fetch and check if CRC_OK bit (MSB) is set */
        cc2420_fifo_read(dev, &crc_corr, 1);
-        if (!(crc_corr & 0x80)) {
+        if (!(crc_corr & CC2420_CRCCOR_CRC_MASK)) {


Think this and related changes should go in a separate commit.

Good point.

bergzand · 2017-08-30T15:05:08Z

@thomaseichinger Thank you for your feedback. I initially build the implementation here to have rssi as a int8_t, but changed it to uint8_t after some discussion in #6887. I think I only changed the implementation to report rssi as the absolute value, but forgot to change the variable back to uint8_t.

bergzand · 2017-08-30T15:06:41Z

The main discussion point now is whether to use uint8_t to express RSSI as the absolute value or as proposed by @gebart use int16_t for RSSI.

thomaseichinger · 2017-08-30T15:39:05Z

@bergzand Thank you for shedding some light on it. Personally I think @gebart's suggestion using int16_t provides us with more flexibility.

bergzand · 2017-08-30T16:23:50Z

Great, then I make sure to adjust and clean up this PR. Is it okay to keep it one big PR for all radio drivers or do you prefer separate PR's for each radio?

thomaseichinger · 2017-08-30T17:43:29Z

I think one PR is fine but maybe split changes in one commit per device.

bergzand · 2017-08-30T20:31:29Z

@thomaseichinger I rebased it a bit, it is now a set of commits, one for the .rssi and one per radio. .rssi is now int16_t and I corrected all issues from your review. The kw2xrf is now also adjusted for signed dBm.

thomaseichinger · 2017-08-30T22:25:34Z

Awesome! Finally, could you add a mention in the header comment that rssi has to be in dBm?

bergzand · 2017-08-31T08:27:20Z

Of course.
Added "in dBm" to the comments of the two structs.

bergzand · 2017-11-17T12:08:12Z

@gvz If you have time, could you please test this PR with the cc2420? I don't have access to that radio, so my code is a best guess on the information from the datasheet.

gvz · 2017-11-21T12:56:53Z

@bergzand Just tried on the cc2420 and the rssi is 65470, I guess something went wrong. I will try to have a look.

bergzand · 2017-11-21T13:30:29Z

@gvz That looks like an interpretation error, int16_t as uint16_t. 65470 is -66 when interpreted as signed, and that looks like a valid number for rssi.

gvz · 2017-11-21T13:49:39Z

Ok I found it: its in: gnrc_netif_hdr_print.c the fix is:
- printf("rssi: %u ", (unsigned) hdr->rssi);
+ printf("rssi: %d ", (signed) hdr->rssi);

bergzand · 2017-11-21T15:40:11Z

Got it, thanks, will put a fixup asap here.

bergzand · 2017-11-22T09:29:41Z

@gvz It should print negative numbers now :)

smlng

tested with samr21-xpro and pba-d-01-kw2x, looks good. Though the drivers derive RSSI and LQI value differently.

smlng · 2017-11-23T09:00:45Z

please squash

bergzand · 2017-11-27T20:50:43Z

Squashed

tests: fix tests/gnrc_netif for changes in #6895

miri64 assigned thomaseichinger, haukepetersen and PeterKietzmann Apr 13, 2017

aabadie modified the milestones: Release 2017.07, Release 2017.10 Jun 26, 2017

bergzand mentioned this pull request Aug 6, 2017

[RFC] net/netstats L1/L2 per neighbor transfer statistics #6873

Closed

4 tasks

bergzand mentioned this pull request Aug 26, 2017

[OpenThread] report RSSI for openthread #7518

Merged

thomaseichinger reviewed Aug 30, 2017

View reviewed changes

bergzand force-pushed the drivers/radio-rssi-conv branch from 318200b to 49b89ce Compare August 30, 2017 15:30

bergzand force-pushed the drivers/radio-rssi-conv branch 2 times, most recently from 28a0279 to e4a5939 Compare August 30, 2017 20:13

bergzand force-pushed the drivers/radio-rssi-conv branch from a6af6f4 to 4970660 Compare August 31, 2017 08:21

thomaseichinger added Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 31, 2017

smlng modified the milestones: Release 2017.10, Release 2018.01 Nov 16, 2017

smlng approved these changes Nov 23, 2017

View reviewed changes

smlng added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Nov 23, 2017

bergzand added 6 commits November 27, 2017 21:49

netdev: Represent RSSI as int16_t

fa64e93

mrf24j40: Add RSSI dBm conversion

4bd5fd6

at86rf2xx: Add RSSI dBm conversion

11e1477

cc2420: Add RSSI dBm conversion

047b817

cc110x: Add RSSI dBm conversion

7dce20a

kw2xrf: dBm conversion to int16_t

2d491e0

bergzand force-pushed the drivers/radio-rssi-conv branch from 444f67c to 2d491e0 Compare November 27, 2017 20:50

miri64 merged commit a63bb6d into RIOT-OS:master Nov 27, 2017

bergzand deleted the drivers/radio-rssi-conv branch November 27, 2017 22:52

miri64 added a commit to miri64/RIOT that referenced this pull request Dec 13, 2017

tests: fix tests/gnrc_netif for changes in RIOT-OS#6895

37bffda

miri64 mentioned this pull request Dec 13, 2017

tests: fix tests/gnrc_netif for changes in #6895 #8255

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Dec 13, 2017

tests: fix tests/gnrc_netif for changes in RIOT-OS#6895

eb08a50

bergzand added a commit that referenced this pull request Dec 13, 2017

Merge pull request #8255 from miri64/tests/fix/regression-fa64e933

d1aa688

tests: fix tests/gnrc_netif for changes in #6895

shr70 pushed a commit to Josar/RIOT that referenced this pull request Dec 18, 2017

tests: fix tests/gnrc_netif for changes in RIOT-OS#6895

cf0f8a9

panail pushed a commit to panail/RIOT that referenced this pull request Oct 29, 2018

tests: fix tests/gnrc_netif for changes in RIOT-OS#6895

1c14242

Conversation

bergzand commented Apr 12, 2017

Uh oh!

Hyungsin commented Aug 28, 2017

Uh oh!

bergzand commented Aug 29, 2017

Uh oh!

Hyungsin commented Aug 29, 2017

Uh oh!

thomaseichinger Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

bergzand Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

thomaseichinger Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

thomaseichinger Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

bergzand Aug 30, 2017

Choose a reason for hiding this comment

Uh oh!

bergzand commented Aug 30, 2017

Uh oh!

bergzand commented Aug 30, 2017

Uh oh!

thomaseichinger commented Aug 30, 2017

Uh oh!

bergzand commented Aug 30, 2017

Uh oh!

thomaseichinger commented Aug 30, 2017

Uh oh!

bergzand commented Aug 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomaseichinger commented Aug 30, 2017

Uh oh!

bergzand commented Aug 31, 2017

Uh oh!

bergzand commented Nov 17, 2017

Uh oh!

gvz commented Nov 21, 2017

Uh oh!

bergzand commented Nov 21, 2017

Uh oh!

gvz commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bergzand commented Nov 21, 2017

Uh oh!

bergzand commented Nov 22, 2017

Uh oh!

smlng left a comment

Choose a reason for hiding this comment

Uh oh!

smlng commented Nov 23, 2017

Uh oh!

bergzand commented Nov 27, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

bergzand commented Aug 30, 2017 •

edited

Loading

gvz commented Nov 21, 2017 •

edited

Loading