drivers/at86rf2xx: Updated address API by maribu · Pull Request #12606 · RIOT-OS/RIOT

maribu · 2019-10-29T22:22:30Z

Contribution description

Changed the address getter and setter functions to avoid byte order confusion. In addition, passing the value via pointer reduces the ROM size a bit on ARM and quite significantly on AVR.

BOARD=microduino-corerf make -C examples/gnrc_minimal
   text    data     bss     dec     hex filename
  45590     426    3354   49370    c0da Current master
  45400     426    3354   49180    c01c This PR

USEMODULE=at86rf233 BOARD=nucleo-f446re make -C examples/gnrc_networking
   text    data     bss     dec     hex filename
  89328     196   18756  108280   1a6f8
  89264     196   18756  108216   1a6b8 This PR

Testing procedure

E.g. ping6 on a board equipped with an at86rf2xx transceiver should still work.

Issues/PRs references

Split out of #12600, as this turned out surprisingly complex. (Or I turned out surprisingly incapable of getting this right...)

benpicco · 2019-10-29T22:30:20Z

Looks like you got it reversed again.

master

2019-10-29 23:28:22,929 # Iface  7  HWaddr: 79:1A  Channel: 26  Page: 0  NID: 0x23
2019-10-29 23:28:22,934 #           Long HWaddr: 8F:05:5D:30:5F:8F:F9:1A 
2019-10-29 23:28:22,940 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2019-10-29 23:28:22,946 #           AUTOACK  ACK_REQ  CSMA  L2-PDU:102 MTU:1280  HL:64  RTR  
2019-10-29 23:28:22,948 #           6LO  IPHC  
2019-10-29 23:28:22,951 #           Source address length: 8
2019-10-29 23:28:22,954 #           Link type: wireless
2019-10-29 23:28:22,960 #           inet6 addr: fe80::8d05:5d30:5f8f:f91a  scope: link  VAL
2019-10-29 23:28:22,963 #           inet6 group: ff02::2
2019-10-29 23:28:22,965 #           inet6 group: ff02::1
2019-10-29 23:28:22,969 #           inet6 group: ff02::1:ff8f:f91a
2019-10-29 23:28:22,972 #           inet6 group: ff02::1a

this PR

2019-10-29 23:28:53,297 # Iface  7  HWaddr: 1A:F9  Channel: 26  Page: 0  NID: 0x23
2019-10-29 23:28:53,301 #           Long HWaddr: 1A:F9:8F:5F:30:5D:05:8F 
2019-10-29 23:28:53,308 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2019-10-29 23:28:53,314 #           AUTOACK  ACK_REQ  CSMA  L2-PDU:102 MTU:1280  HL:64  RTR  
2019-10-29 23:28:53,316 #           6LO  IPHC  
2019-10-29 23:28:53,319 #           Source address length: 8
2019-10-29 23:28:53,322 #           Link type: wireless
2019-10-29 23:28:53,327 #           inet6 addr: fe80::18f9:8f5f:305d:58f  scope: link  VAL
2019-10-29 23:28:53,330 #           inet6 group: ff02::2
2019-10-29 23:28:53,333 #           inet6 group: ff02::1
2019-10-29 23:28:53,336 #           inet6 group: ff02::1:ff5d:58f
2019-10-29 23:28:53,339 #           inet6 group: ff02::1a

drivers/at86rf2xx/at86rf2xx_getset.c

maribu · 2019-10-30T07:17:21Z

Looks like you got it reversed again.

Apparently, it was wrong in master. The most significant byte of long address of your board in master is 0x8F == 0b10001111. But the least significant bit of the most significant byte should be cleared and the second least significant bit of that byte should be set. With this PR the most signifact byte now is 0x1a == 0b00011010, which correctly indicates non-globally unique and non-multicast.

So making the API more fool proof in regard to the byte order fixed that bug :-)

maribu · 2019-10-30T08:07:24Z

Oh, the short address is now generated from the most significant bytes, not the least significant bytes. Let me change that back to the original behavior.

maribu · 2019-10-30T08:27:12Z

OK. I tested with examples/gnrc_networking (trimmed down to only contain a shell IPv6/6LoWPAN and ICMP) with two Microduino-CoreRF:

2019-10-30 09:23:57,698 # ifconfig
2019-10-30 09:23:57,708 # Iface  5  HWaddr: 3D:84  Channel: 26  Page: 0  NID: 0x23
2019-10-30 09:23:57,716 #           Long HWaddr: 3E:84:22:3C:A0:27:3D:84 
2019-10-30 09:23:57,729 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2019-10-30 09:23:57,741 #           AUTOACK  ACK_REQ  CSMA  L2-PDU:102 MTU:1280  HL:64  6LO  
2019-10-30 09:23:57,744 #           IPHC  
2019-10-30 09:23:57,750 #           Source address length: 8
2019-10-30 09:23:57,755 #           Link type: wireless
2019-10-30 09:23:57,766 #           inet6 addr: fe80::3c84:223c:a027:3d84  scope: link  VAL
2019-10-30 09:23:57,772 #           inet6 group: ff02::1
2019-10-30 09:23:57,774 #           
> ping6 fe80::3c84:223c:a027:3d85
2019-10-30 09:24:04,422 #  ping6 fe80::3c84:223c:a027:3d85
2019-10-30 09:24:04,449 # 12 bytes from fe80::3c84:223c:a027:3d85: icmp_seq=0 ttl=64 rssi=-48 dBm time=11.600 ms
2019-10-30 09:24:05,449 # 12 bytes from fe80::3c84:223c:a027:3d85: icmp_seq=1 ttl=64 rssi=-48 dBm time=12.240 ms
2019-10-30 09:24:06,451 # 12 bytes from fe80::3c84:223c:a027:3d85: icmp_seq=2 ttl=64 rssi=-48 dBm time=13.196 ms
2019-10-30 09:24:06,451 # 
2019-10-30 09:24:06,459 # --- fe80::3c84:223c:a027:3d85 PING statistics ---
2019-10-30 09:24:06,469 # 3 packets transmitted, 3 packets received, 0% packet loss
2019-10-30 09:24:06,477 # round-trip min/avg/max = 11.600/12.345/13.196 ms
>

(I changed the long address of the second board from 3E:84:22:3C:A0:27:3D:84 to 3E:84:22:3C:A0:27:3D:85 and updated the short address and IPv6 address accordingly. Those boards have no propper CPU ID, so they get the same address after boot.)

miri64

I talked to Hauke, who originally introduced the usage of uint64_t for the long address here, and he doesn't know anymore why he decided to do it this way. The approach with byte arrays seems far more sensible. So would ACK, however the asserts should be fixed according to the expected types for the given NETOPTs.

drivers/at86rf2xx/at86rf2xx_netdev.c

benpicco · 2019-10-31T17:48:46Z

The address is still reversed - is this intended?

before

2019-10-31 18:45:35,280 # Iface  7  HWaddr: 06:5A  Channel: 26  Page: 0  NID: 0x23
2019-10-31 18:45:35,290 #           Long HWaddr: 79:7D:36:2C:C9:59:06:5A 
2019-10-31 18:45:35,291 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2019-10-31 18:45:35,298 #           AUTOACK  ACK_REQ  CSMA  L2-PDU:102 MTU:1280  HL:64  RTR  
2019-10-31 18:45:35,300 #           6LO  IPHC  
2019-10-31 18:45:35,303 #           Source address length: 8
2019-10-31 18:45:35,306 #           Link type: wireless
2019-10-31 18:45:35,311 #           inet6 addr: fe80::7b7d:362c:c959:65a  scope: link  VAL
2019-10-31 18:45:35,314 #           inet6 group: ff02::2
2019-10-31 18:45:35,317 #           inet6 group: ff02::1
2019-10-31 18:45:35,321 #           inet6 group: ff02::1:ff59:65a
2019-10-31 18:45:35,323 #           inet6 group: ff02::1a

after

2019-10-31 18:46:42,579 # Iface  7  HWaddr: 7D:79  Channel: 26  Page: 0  NID: 0x23
2019-10-31 18:46:42,583 #           Long HWaddr: 5A:06:59:C9:2C:36:7D:79 
2019-10-31 18:46:42,592 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 
2019-10-31 18:46:42,599 #           AUTOACK  ACK_REQ  CSMA  L2-PDU:102 MTU:1280  HL:64  RTR  
2019-10-31 18:46:42,602 #           6LO  IPHC  
2019-10-31 18:46:42,605 #           Source address length: 8
2019-10-31 18:46:42,608 #           Link type: wireless
2019-10-31 18:46:42,615 #           inet6 addr: fe80::5806:59c9:2c36:7d79  scope: link  VAL
2019-10-31 18:46:42,618 #           inet6 group: ff02::2
2019-10-31 18:46:42,621 #           inet6 group: ff02::1
2019-10-31 18:46:42,625 #           inet6 group: ff02::1:ff36:7d79
2019-10-31 18:46:42,631 #           inet6 group: ff02::1a

maribu · 2019-10-31T18:27:01Z

The address is still reversed - is this intended?

Kind of. The code in master markes the first byte as non globally unique and non multicast and then calls ntohll(), effectively marking the least significant byte instead of (as intended) the most significant byte.

So I could keep the call to ntohll() (which doesn't really serve a purpose, as the numeric value of the address doesn't really matter). But in that case I would need to mark the last byte of the LUID instead of the first byte prior to the call of ntohll(), as this will become the MSB afterwards.

So the L2 address will change no matter what when we want to fix the issue of having 802.15.4 long addresses incorrectly marked as multicast or/and as globally unique. And without the call to ntohll() the code size will be a tad smaller, so I favoured that.

I could hover split this change out of the PR and keep the L2 address as in master for now, if you prefer.

benpicco · 2019-10-31T19:34:09Z

I'm not attached to the value or order of the L2 address - just wanted to make sure things are in order. :)

drivers/at86rf2xx/at86rf2xx_getset.c

benpicco

This is surely an improvement.
The address changes, but it will change again with the luid/Mac address dispatching changes.

maribu · 2019-11-14T18:38:15Z

May I squash?

drivers/at86rf2xx/at86rf2xx.c

My comments were addressed.

Changed the address getter and setter functions to avoid byte order confusion.

maribu · 2019-11-23T18:40:43Z

Squashed :-)

maribu added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: drivers Area: Device drivers labels Oct 29, 2019

maribu requested review from benpicco and miri64 October 29, 2019 22:22

miri64 requested changes Oct 29, 2019

View reviewed changes

drivers/at86rf2xx/at86rf2xx_getset.c Outdated Show resolved Hide resolved

drivers/at86rf2xx/at86rf2xx_getset.c Outdated Show resolved Hide resolved

drivers/at86rf2xx/at86rf2xx_getset.c Outdated Show resolved Hide resolved

drivers/at86rf2xx/at86rf2xx_getset.c Outdated Show resolved Hide resolved

miri64 previously requested changes Oct 31, 2019

View reviewed changes

drivers/at86rf2xx/at86rf2xx_netdev.c Outdated Show resolved Hide resolved

drivers/at86rf2xx/at86rf2xx_netdev.c Outdated Show resolved Hide resolved

benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 3, 2019

benpicco reviewed Nov 4, 2019

View reviewed changes

drivers/at86rf2xx/at86rf2xx_getset.c Show resolved Hide resolved

benpicco approved these changes Nov 5, 2019

View reviewed changes

benpicco requested a review from miri64 November 7, 2019 17:48

This was referenced Nov 14, 2019

drivers/at86rf2xx: Fix L2 addr genration/handling #12600

Merged

net/ieee802154: Use eui64_t and network_uint16_t in ieee802154_t for addresses #12713

Closed

benpicco reviewed Nov 21, 2019

View reviewed changes

drivers/at86rf2xx/at86rf2xx.c Outdated Show resolved Hide resolved

drivers/at86rf2xx: Updated address API

f0317c5

Changed the address getter and setter functions to avoid byte order confusion.

maribu force-pushed the at86rf2xx-addr-endianess branch from 15d05d1 to f0317c5 Compare November 23, 2019 18:40

benpicco merged commit ce1f383 into RIOT-OS:master Nov 24, 2019

fjmolinas added this to the Release 2020.01 milestone Dec 13, 2019

maribu deleted the at86rf2xx-addr-endianess branch February 6, 2020 20:47

aabadie mentioned this pull request Feb 13, 2020

iotlab-m3: long HW address is in reversed order #13358

Closed

Conversation

maribu commented Oct 29, 2019

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

benpicco commented Oct 29, 2019

master

this PR

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maribu commented Oct 30, 2019

Uh oh!

maribu commented Oct 30, 2019

Uh oh!

maribu commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benpicco commented Oct 31, 2019

before

after

Uh oh!

maribu commented Oct 31, 2019

Uh oh!

benpicco commented Oct 31, 2019

Uh oh!

Uh oh!

benpicco left a comment

Choose a reason for hiding this comment

Uh oh!

maribu commented Nov 14, 2019

Uh oh!

Uh oh!

maribu commented Nov 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

maribu commented Oct 30, 2019 •

edited

Loading