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.

Sign up for GitHub

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

Jump to bottom

net: add eui48 type including IPv6 IID conversion funcs #10567

Merged
merged 4 commits into from
Dec 6, 2018
Merged

net: add eui48 type including IPv6 IID conversion funcs #10567

merged 4 commits into from
Dec 6, 2018

Conversation

haukepetersen
Copy link
Contributor

Contribution description

While working on the IID handling for GNRC and on implementing GNRC over NimBLE, I found that the IID handling is identical for BLE and Ethernet (and also ESP_NOW). So IMO it makes sense to factor the IID generation and reversal into its own header, as it should be the same for all IEEE EUI-48 based device addresses.

Further calling some ethernet_get_iid() function for switch NETDEV_TYPE_BLE looks pretty strange :-)

Testing procedure

Make sure ping6 between native nodes (they use Ethernet) still works as expected.

Issues/PRs references

follow up on #10513

@haukepetersen haukepetersen added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation labels Dec 6, 2018
@haukepetersen haukepetersen requested a review from miri64 December 6, 2018 18:08
miri64
miri64 reviewed Dec 6, 2018
View reviewed changes
Copy link
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

LGTM. Untested ACK.

@miri64 miri64 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 6, 2018
miri64
miri64 reviewed Dec 6, 2018
View reviewed changes
sys/include/net/eui48.h Outdated
*
* @see <a href="https://tools.ietf.org/html/rfc2464#section-4">
* E.g.: RFC 2464, section 4
* </a>
Copy link
Member

Choose a reason for hiding this comment

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

Rather use markdown style? It's more compact and more readable when reading the doc unparsed.

 * @see [RFC 2464, section 4](https://tools.ietf.org/html/rfc2464#section-4)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

definitely, was just copy pasted, changed.

sys/include/net/eui48.h Outdated
*
* @see <a href="https://tools.ietf.org/html/rfc2464#section-4">
* E.g.: RFC 2464, section 4
* </a>
Copy link
Member

Choose a reason for hiding this comment

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

BTW, the reasoning for the flipping of the U/L bit can be found in RFC 4291, section 2.5.1, for those interested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this also as reference

@miri64 miri64 removed the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Dec 6, 2018
@miri64
Copy link
Member

miri64 commented Dec 6, 2018

@haukepetersen I think you forgot to push ;-)

@haukepetersen
Copy link
Contributor Author

nope, simply found some other minors to fix:

  • changed doxygen group name to the same style as the eui64 (added the word 'identifier')
  • made the changes you suggested

all squashed right in.

@miri64 miri64 added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Dec 6, 2018
@miri64
Copy link
Member

miri64 commented Dec 6, 2018

ACK. Tested with native by

a) checking the IID of the auto-configured IPv6 address
b) trying out the back-translation by merging in a WIP branch of mine that implements RFC 7973 and compiling gnrc_networking for native and pinging between two instances.

@miri64 miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Dec 6, 2018
@miri64 miri64 added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 6, 2018
@haukepetersen
Copy link
Contributor Author

all green -> lets go.

@haukepetersen haukepetersen merged commit e8535f2 into RIOT-OS:master Dec 6, 2018
@haukepetersen haukepetersen deleted the add_eui48 branch December 6, 2018 21:38
@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
@miri64 miri64 mentioned this pull request Jan 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miri64 miri64 miri64 approved these changes

Assignees
No one assigned
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 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: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation
Projects
None yet
Milestone
Release 2019.01
Development

Successfully merging this pull request may close these issues.
3 participants
@haukepetersen @miri64 @aabadie