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

cpu/stm32/eth: Use luid_get_eui48 to generate local, non group EUI #13232

Merged
merged 2 commits into from
Jan 30, 2020

Conversation

kfessel
Copy link
Contributor

@kfessel kfessel commented Jan 29, 2020

luid_get may generate non compliant EUI (MAC-address) luid_get_eui48
fixes that.

I had MAC been generated that was marked as group (multicast) so Wireshark complained, i looked into it found that there is allready a function in Riot to generate appropriate EUI, and ei changed and testet.

It worked (no more complain by wireshark) and the interface is still answering echo requests.

@kfessel kfessel requested review from maribu, benpicco and miri64 January 29, 2020 14:15
@benpicco benpicco added Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jan 29, 2020
@miri64 miri64 changed the title cpu/stm32_common: Use luid_get_eui48 to generate local, non group EUI cpu/stm32/periph_eth: Use luid_get_eui48 to generate local, non group EUI Jan 29, 2020
@miri64 miri64 changed the title cpu/stm32/periph_eth: Use luid_get_eui48 to generate local, non group EUI cpu/stm32/eth: Use luid_get_eui48 to generate local, non group EUI Jan 29, 2020
@miri64
Copy link
Member

miri64 commented Jan 29, 2020

Can you adapt the commit message according to the current title, please? I was very confused what a CPU is doing with a EUI-48 at first.

@miri64 miri64 added Area: drivers Area: Device drivers Area: network Area: Networking and removed Area: cpu Area: CPU/MCU ports labels Jan 29, 2020
luid_get may generate non compliant EUI (MAC-address) luid_get_eui48
fixes that.
@kfessel
Copy link
Contributor Author

kfessel commented Jan 29, 2020

Done
I wasn't sure about this commit title-format

@miri64
Copy link
Member

miri64 commented Jan 29, 2020

I wasn't sure about this commit title-format

As long as it's clear what component is changed its fine :-)

changed type of hwaddr to eui48
moved hwaddr declaration  where it is needed
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

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

Code looks good, can easily swapped should #12641 get merged.

@miri64
Copy link
Member

miri64 commented Jan 29, 2020

@benpicco did you test this?

@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 labels Jan 29, 2020
@benpicco
Copy link
Contributor

benpicco commented Jan 30, 2020

I think it should suffice if @kfessel can confirm that the MAC address is the right way round (eui48_t is Big Endian, stm32_eth_set_mac() looks Big Endian too.)

@kfessel
Copy link
Contributor Author

kfessel commented Jan 30, 2020

Every bit but the group bit, which was cleard, stayed the same, so this bit is at the right place. I ping fe80::28xx:xxff:fexx:xxxx instead of fe80::29xx:xxff:fexx:xxxx now. So it is working.

@miri64 miri64 merged commit 3d4977c into RIOT-OS:master Jan 30, 2020
@miri64 miri64 added the Reviewed: 3-testing The PR was tested according to the maintainer guidelines label Jan 30, 2020
@leandrolanzieri leandrolanzieri added this to the Release 2020.04 milestone Feb 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers 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 Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants