Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gnrc_netif: add general IID to/from l2addr conversion functions #10513

Merged
merged 4 commits into from
Dec 6, 2018

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Nov 29, 2018

Contribution description

They are required to make GNRC's 6Lo IPHC implementation ready for hardware addresses other than IEEE 802.15.4's.

Testing procedure

Pinging a link-local address with gnrc_networking should still work for

  • Ethernet
  • IEEE 802.15.4
  • Nordic BLE softdevice
  • cc110x
  • nrfmin
  • ESP NOW

Issues/PRs references

Depends on #10509 (merged), #10514 (invalid dependency), and #10515 (merged).

gnrc_netif/gnrc_sixlowpan_iphc BLE capability

@miri64 miri64 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: network Area: Networking Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. labels Nov 29, 2018
@miri64 miri64 changed the title Gnrc netif/enh/iid conversion gnrc_netif: add general IID to/from l2addr conversion functions Nov 29, 2018
@miri64 miri64 force-pushed the gnrc_netif/enh/iid-conversion branch from b20cdb4 to 80258ef Compare November 29, 2018 14:46
@miri64 miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Nov 29, 2018
@miri64
Copy link
Member Author

miri64 commented Nov 29, 2018

Rebased to current master and new dependencies.

miri64 added a commit to miri64/RIOT that referenced this pull request Nov 29, 2018
@miri64 miri64 force-pushed the gnrc_netif/enh/iid-conversion branch from c618ccd to b2a05a5 Compare November 30, 2018 08:35
@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed State: waiting for other PR State: The PR requires another PR to be merged first labels Nov 30, 2018
@miri64
Copy link
Member Author

miri64 commented Nov 30, 2018

Rebased to and adapted for current master (since the #10514 dependency actually requires a PR downstream from this PR - #10514 (comment)). Ready for review and testing.

Copy link
Member

@maribu maribu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me code-wise :-)

@maribu maribu added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Dec 4, 2018
@maribu
Copy link
Member

maribu commented Dec 4, 2018

I hope I can test it tomorrow with the cc110x (both current and future) and with an enc28j60.

@maribu
Copy link
Member

maribu commented Dec 5, 2018

To me it looks like the current issue Murdock uncovered seems to be unrelated.

@cladmi: As far as I know you are a linker magician. Care to have a look at the murdock build log?

/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x80220a of /tmp/dwq.0.8781428163408961/ce7b5c92c43c4dec218e57c9939ac8c2/build/tests_gnrc_sock_udp.elf section `.bss' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x80220c of /tmp/dwq.0.8781428163408961/ce7b5c92c43c4dec218e57c9939ac8c2/build/tests_gnrc_sock_udp.elf section `.noinit' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x80220a of /tmp/dwq.0.8781428163408961/ce7b5c92c43c4dec218e57c9939ac8c2/build/tests_gnrc_sock_udp.elf section `.bss' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x80220c of /tmp/dwq.0.8781428163408961/ce7b5c92c43c4dec218e57c9939ac8c2/build/tests_gnrc_sock_udp.elf section `.noinit' is not within region `data'
collect2: error: ld returned 1 exit status

This does not seem to be an overflow, as in case of an overflow I get a message like this:

/usr/lib/gcc/arm-none-eabi/6.3.1/../../../../arm-none-eabi/bin/ld: /home/maribu/Repos/software/RIOT/examples/gnrc_networking/bin/bluepill/gnrc_networking.elf section `.text' will not fit in region `rom'
/usr/lib/gcc/arm-none-eabi/6.3.1/../../../../arm-none-eabi/bin/ld: region `rom' overflowed by 17640 bytes

(I cannot recall how the message looks like in case of a RAM overflow, but I assume it will also use the phrase "overflowed by xy bytes".)

@haukepetersen
Copy link
Contributor

would you mind to rebase? That would simplify the review a bit... Thx

@miri64
Copy link
Member Author

miri64 commented Dec 5, 2018

Back on track: @maribu were you able to test by now?

@cladmi
Copy link
Contributor

cladmi commented Dec 5, 2018

To show the output for the arduino-mega2560:

On my ubuntu-bionic machine:

With this PR:

RIOT_CI_BUILD=1 BOARD=arduino-mega2560 make -C tests/gnrc_sock_udp/
make: Entering directory '/home/harter/work/git/RIOT/tests/gnrc_sock_udp'
Building application "tests_gnrc_sock_udp" for "arduino-mega2560" with MCU "atmega2560".

/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x80220a of /home/harter/work/git/RIOT/tests/gnrc_sock_udp/bin/arduino-mega2560/tests_gnrc_sock_udp.elf section `.bss' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x80220c of /home/harter/work/git/RIOT/tests/gnrc_sock_udp/bin/arduino-mega2560/tests_gnrc_sock_udp.elf section `.noinit' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x80220a of /home/harter/work/git/RIOT/tests/gnrc_sock_udp/bin/arduino-mega2560/tests_gnrc_sock_udp.elf section `.bss' is not within region `data'
/usr/lib/gcc/avr/5.4.0/../../../avr/bin/ld: address 0x80220c of /home/harter/work/git/RIOT/tests/gnrc_sock_udp/bin/arduino-mega2560/tests_gnrc_sock_udp.elf section `.noinit' is not within region `data'
collect2: error: ld returned 1 exit status
/home/harter/work/git/RIOT/tests/gnrc_sock_udp/../../Makefile.include:446: recipe for target '/home/harter/work/git/RIOT/tests/gnrc_sock_udp/bin/arduino-mega2560/tests_gnrc_sock_udp.elf' failed
make: *** [/home/harter/work/git/RIOT/tests/gnrc_sock_udp/bin/arduino-mega2560/tests_gnrc_sock_udp.elf] Error 1
make: Leaving directory '/home/harter/work/git/RIOT/tests/gnrc_sock_udp'

With a "bigger" RAM, 16K instead of 8K linking works properly:

RIOT_CI_BUILD=1 BOARD=arduino-mega2560 make -C tests/gnrc_sock_udp/ RAM_LEN=16K
make: Entering directory '/home/harter/work/git/RIOT/tests/gnrc_sock_udp'
Building application "tests_gnrc_sock_udp" for "arduino-mega2560" with MCU "atmega2560".

   text    data     bss     dec     hex filename
  46690    4362    3842   54894    d66e /home/harter/work/git/RIOT/tests/gnrc_sock_udp/bin/arduino-mega2560/tests_gnrc_sock_udp.elf
make: Leaving directory '/home/harter/work/git/RIOT/tests/gnrc_sock_udp'

So the board just needs to be put in BOARD_INSUFFICIENT_MEMORY.

@maribu
Copy link
Member

maribu commented Dec 5, 2018

Works fine with the CC1100/MSB-A2 using the current cc110x driver and works fine with the CC1101/MSB-IoT using PR #10340

@miri64
Copy link
Member Author

miri64 commented Dec 5, 2018

@gschorcht can you test with ESP32?

@gschorcht
Copy link
Contributor

Works fine with the ESP32

@miri64
Copy link
Member Author

miri64 commented Dec 5, 2018

Thanks for testing @maribu and @gschorcht!

@maribu
Copy link
Member

maribu commented Dec 6, 2018

I just found my enc28j60. I will test as soon as I have time

@miri64
Copy link
Member Author

miri64 commented Dec 6, 2018

I just found my enc28j60. I will test as soon as I have time

You don't have to if you don't have the time. native can also be used for testing Ethernet.

@gschorcht
Copy link
Contributor

@miri64 I forgot to mention, I tested ESP32 only with esp_now netdev driver.

I have also an ESP32 board with EMAC and LAN8270 ethernet interface. So, I tested it with esp_eth netdev driver too and it works fine.

@TimoRoth
Copy link
Contributor

TimoRoth commented Dec 6, 2018

What I'm wondering about ESP-NOW is that for examples its _send function expects the data to be a pointer to an ipv6_hdr_t:

https://github.com/RIOT-OS/RIOT/blob/master/cpu/esp32/esp-now/esp_now_netdev.c#L526

But with 6Lo, wouldn't something along those lines be needed, where the first byte of the data indicates what actually follows:

https://github.com/RIOT-OS/RIOT/blob/master/sys/net/network_layer/sixlowpan/sixlowpan_print.c#L23

Or is that unpacked for ESP-NOW at an earlier point, and it only gets to see properly fragmented actual IPv6 packages?

@miri64
Copy link
Member Author

miri64 commented Dec 6, 2018

@TimoRoth can we please discuss 6Lo over ESP-Now in #10531 rather than this unrelated issue?

@miri64
Copy link
Member Author

miri64 commented Dec 6, 2018

But to answer your question: yes ESP-now needs then to assume, that the first header is a 6Lo dispatch instead of a IPv6 header.

@miri64
Copy link
Member Author

miri64 commented Dec 6, 2018

(this whole file looks rather hacked I might add oO)

@haukepetersen
Copy link
Contributor

tested all remaining link layers successfully:

  • nrfmin using two nrf52dk boards no my desk -> ping works both ways
  • ieee802.15.4 using two iotlab-m3 boards in the iot-lab -> ping works both ways
  • ethernet using native -> ping works two ways
  • softdevice -> ping to/from linux does not work, but the same is true for master. But the IID generation (verified using ifconfig) seems to do the trick, so I consider this PR not doing any harm here...

@miri64 miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 6, 2018
Copy link
Contributor

@haukepetersen haukepetersen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

@miri64 miri64 force-pushed the gnrc_netif/enh/iid-conversion branch from 0535402 to bc1b490 Compare December 6, 2018 13:41
@miri64
Copy link
Member Author

miri64 commented Dec 6, 2018

Squashed

@miri64
Copy link
Member Author

miri64 commented Dec 6, 2018

Marked arduino-mega2560 in tests/gnrc_sock_udp for insufficient memory.

@miri64 miri64 force-pushed the gnrc_netif/enh/iid-conversion branch from c7d146d to f757376 Compare December 6, 2018 14:39
@haukepetersen
Copy link
Contributor

all green -> go!

@haukepetersen haukepetersen merged commit 92fcb4d into RIOT-OS:master Dec 6, 2018
@miri64 miri64 deleted the gnrc_netif/enh/iid-conversion branch December 6, 2018 14:57
@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants