Skip to content

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

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
kfessel:master
Jan 30, 2020
Merged

cpu/stm32/eth: Use luid_get_eui48 to generate local, non group EUI#13232
miri64 merged 2 commits intoRIOT-OS:masterfrom
kfessel:master

Conversation

@kfessel
Copy link
Copy Markdown
Contributor

@kfessel kfessel commented Jan 29, 2020 •
edited
Loading

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 benpicco, maribu 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
Copy Markdown
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
@kfessel
cpu/stm32/eth: Use luid_get_eui48 to generate local, non group EUI
7483826 
luid_get may generate non compliant EUI (MAC-address) luid_get_eui48
fixes that.
@kfessel
Copy link
Copy Markdown
Contributor Author

kfessel commented Jan 29, 2020

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

benpicco
benpicco reviewed Jan 29, 2020
View reviewed changes
@miri64
Copy link
Copy Markdown
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 :-)

@kfessel
cpu/stm32/eth: luid_get_eui48 match type information
b4b9ffe 
changed type of hwaddr to eui48
moved hwaddr declaration  where it is needed
benpicco
benpicco approved these changes Jan 29, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

benpicco commented Jan 30, 2020 •
edited
Loading

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
Copy Markdown
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
@kfessel kfessel mentioned this pull request Feb 3, 2020
Closed
@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

Reviewers

@benpicco benpicco benpicco approved these changes

@maribu maribu Awaiting requested review from maribu

@miri64 miri64 Awaiting requested review from miri64

Assignees

No one assigned

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

Milestone

Release 2020.04

Development

Successfully merging this pull request may close these issues.

4 participants

@kfessel @miri64 @benpicco @leandrolanzieri