net/l2util: add eui_short/48/64_get() by benpicco · Pull Request #12641 · RIOT-OS/RIOT

benpicco · 2019-11-04T17:21:28Z

Contribution description

Based on the discussion in #12592 the consensus seemed that there should be an additional layer on top of luid to dispatch L2 addresses.

Maybe it could be as simple as that?

If your board provides a EUI64, with this all you have to do is implement

size_t board_get_eui64(netdev_t *netdev, eui64_t *addr) {
    …
}

and then figure out a way to dispatch the address to the right netdev.
However, for the most common case where there is only one interface, you can simple ignore netdev.

Testing procedure

Nothing to test yet.

Issues/PRs references

depends on #12592
addresses #12616

sys/net/link_layer/l2util/l2util.c

benpicco · 2019-11-07T17:47:57Z

Rebased & renamed the functions to better fit with the other functions in eui48.h/eui64.h

sys/include/net/eui48.h

sys/include/net/eui64.h

API users should only call the wrapper functions.

miri64

I am not happy about the naming and placement of these functions. IMHO they belong more to l2util than the type definitions for euiXX.

sys/include/net/eui48.h

miri64 · 2019-11-09T19:23:10Z

sys/include/net/eui48.h

+ * @param[out] addr     The dedicated address for the netdev
+ *
+ */
+size_t board_get_eui48(netdev_t *netdev, eui48_t *addr);


eui48_get_from_board()?

miri64 · 2019-11-09T19:23:18Z

sys/include/net/eui64.h

+ * @param[out] addr     The generated short address
+ *
+ */
+static inline void eui_short_get(netdev_t *netdev, uint16_t *addr)


Why is this in eui64.h. Neither is the short address of IEEE 802.15.4 an EUI, nor has it anything to do with an EUI-64 (except that they are both used in IEEE 802.15.4 as addresses). This function and board_get_eui_short don't belong here. You've put the implementation for board_get_…() already in l2util (which given that l2util might not be compiled in in any case is somewhat of a bargain IMHO, but let's discuss this elsewhere), so why not put it there as l2util_generate_short_addr()?

Then it should also be l2util_generate_euiXX() for consistency.

Yes, i wrote the other comments before I realized, that the other functions are also misplaced.

…i*()

benpicco · 2019-11-10T01:48:13Z

I moved them back to l2util once again.

sys/include/net/l2util.h

…rt/48/64()

maribu · 2019-11-19T22:50:24Z

Let's define what we expect as use cases by likelyhood. I'd say there are three scenarios:

There are no EUIs provided at all, LUID has to be used
There is a single EUI chip on the board
There are multiple EUI chips on the board

I believe that this is also the order of likelihood. I think that this PR handles case 1. and 3. very well, but case 2. could be handled better: If only a single EUI driver is loaded, this could provide the custom EUI function as weak symbol, which returns the provided EUI on the first call and an error on all subsequent. This would require no board support (except for adding e.g. USEMODUE += at24mac). If multiple EUI chips are present, the board would need to provide the function as strong symbol that matches the driver name and index to the correct EUI provider.

benpicco · 2019-11-20T19:57:33Z

As for 2., I could implement

size_t board_get_eui48(const char *driver, unsigned idx, eui48_t *addr) {
    (void) driver;
    /* Maybe at24mac_get_euiXX() should just return the size */
    if (at24mac_get_eui48(idx, addr)) {
        return 0;
    }
    return sizeof(*addr);
}

in #12746.
But If the board wants to also implement board_get_euiXX() to handle the driver name, we would have to introduce some define.

benpicco · 2019-12-10T12:21:26Z

I think we should just drop that 'automatic' MAC chip <-> transceiver pairing idea and leave it to the board to implement board_get_euiXX().

If at some point we come up with a good solution we can still convert it to that, but let's start with the simple implementation first.

miri64

Thanks for bringing this back to my attention. I have a question though...

miri64 · 2020-01-29T20:03:48Z

sys/include/net/l2util.h

+ *
+ * @warning Don't call this function directly, use @ref l2util_generate_short_addr() instead.
+ *
+ * @param[in] driver    The driver name of the netdev


Please relight my memory: What is the driver name of the netdev?

The idea was that a EUI would be assigned to a certain device/driver.
E.g. both samr21-xpro and same54-xpro contain a EUI-64 resp. EUI-48.
On samr21-xpro that should only be used for the first device of the at86rf2xx driver…

It's not pretty but so far we haven't come up with a better solution to assign IDs to netdevs.

It's not pretty but so far we haven't come up with a better solution to assign IDs to netdevs.

Mhhhh its not just not pretty, it can be very costly. Both the string will cost memory, and string operations aren't the most efficient either. :-/

Would it make sense to use NETDEV_TYPE_ here?

If the netdev allocations would be in an xfa this would be simpler :-/.

For this we would have the migration benefit, that this could be done driver by driver.

Why not provide a board-unique ID with params btw?

we have to touch every board

we waste RAM and ROM for information that is already implicitly available

* we waste RAM and ROM for information that is already implicitly available

I thought it is not. If it is why use the (more RAM heavy) name?

* we have to touch every board

If we don't use the current approach, I believe we don't have. Also: only every board that provides a netdev params.

I thought it is not. If it is why use the (more RAM heavy) name?

Well we just have to touch every netdev driver (to do the index -> params mapping inside the driver instead of auto_init). I just want to be sure that's the right way to go before I shave that Yak 😉

I just want to be sure that's the right way to go before I shave that Yak wink

See #12641 (comment). As far as I understand board.c might not exist anymore in the near future :-/.

miri64 · 2020-01-30T12:56:44Z

Apparently, there is some rework to board.c waiting to be proposed. @kaspar030 could you maybe have a look, at the core idea of this PR is still relevant with your rework in mind.

…eui_short/48/64()

The board comes with an EU-64 stored inside EDBG. It is also printed on a label on the board.

benpicco · 2020-01-30T15:12:19Z

I've converted at86rf2xx as an example and implemented board_get_eui64() for samr21-xpro.
The HWaddr should now match the one printed backside of the board.

miri64 · 2020-06-25T13:17:32Z

@benpicco can you give this a rebase and status update on the WIP state of this PR?

benpicco · 2020-06-25T13:58:01Z

Requires #13746 for a proper solution, but I don't have time to shave that yak now.
If we want to solve the common 'board provides EUI, interface should use EUI' case and we don't care about the order for the multi-EUI / multi-Interface case, we can just implement board_get_eui…() as initially proposed here.

benpicco · 2020-07-24T13:35:36Z

superseded by #13746

benpicco requested review from maribu and miri64 November 4, 2019 17:21

benpicco mentioned this pull request Nov 4, 2019

sys/luid: add luid_get_eui48() / luid_get_eui64() #12592

Merged

maribu reviewed Nov 4, 2019

View reviewed changes

sys/net/link_layer/l2util/l2util.c Outdated Show resolved Hide resolved

miri64 reviewed Nov 4, 2019

View reviewed changes

sys/net/link_layer/l2util/l2util.c Outdated Show resolved Hide resolved

net/l2util: add l2util_get_eui_short/48/64()

523d5cf

benpicco force-pushed the eui-get branch from 411374c to 523d5cf Compare November 7, 2019 17:45

benpicco changed the title ~~net/l2util: add l2util_get_eui_short/48/64()~~ net/l2util: add eui_short/48/64_get() Nov 7, 2019

miri64 requested changes Nov 7, 2019

View reviewed changes

sys/include/net/eui48.h Outdated Show resolved Hide resolved

sys/include/net/eui64.h Outdated Show resolved Hide resolved

fixup! net/l2util: add l2util_get_eui_short/48/64()

3366c83

benpicco removed the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Nov 8, 2019

benpicco mentioned this pull request Nov 8, 2019

USBUS CDC ECM: host and MCU mac addresses should differ #12676

Merged

benpicco added 2 commits November 8, 2019 15:56

sys/luid: add warning to use eui*_get() instead of luid_get_eui*()

70c9cf2

API users should only call the wrapper functions.

fixup! fixup! net/l2util: add l2util_get_eui_short/48/64()

94a3bed

miri64 requested changes Nov 9, 2019

View reviewed changes

benpicco added 2 commits November 10, 2019 02:43

fixup! fixup! fixup! net/l2util: add l2util_get_eui_short/48/64()

18ce9b4

fixup! sys/luid: add warning to use eui*_get() instead of luid_get_eu…

70b87a4

…i*()

miri64 reviewed Nov 10, 2019

View reviewed changes

sys/include/net/l2util.h Outdated Show resolved Hide resolved

fixup! fixup! fixup! fixup! net/l2util: add l2util_get_eui_short/48/64()

9ed80a2

benpicco mentioned this pull request Nov 14, 2019

drivers/at86rf2xx: don't copy params to RAM #12712

Closed

fixup! fixup! fixup! fixup! fixup! net/l2util: add l2util_get_eui_sho…

bcc1433

…rt/48/64()

benpicco force-pushed the eui-get branch from 572dd0d to bcc1433 Compare November 16, 2019 00:26

benpicco mentioned this pull request Nov 19, 2019

drivers: add driver for the AT24MAC unique ID chip #12746

Merged

benpicco requested a review from jia200x November 29, 2019 09:00

miri64 reviewed Jan 29, 2020

View reviewed changes

benpicco added 3 commits January 30, 2020 16:07

fixup! fixup! fixup! fixup! fixup! fixup! net/l2util: add l2util_get_…

78ce6c1

…eui_short/48/64()

drivers/at86rf2xx: convert to use l2util_generate_eui64()

ef45452

boards/samr21-xpro: provide board_get_eui64()

fdc9e4a

The board comes with an EU-64 stored inside EDBG. It is also printed on a label on the board.

benpicco mentioned this pull request Feb 13, 2020

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

Closed

benpicco mentioned this pull request Mar 3, 2020

Order of auto_init functions #13541

Open

benpicco added the State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet label Mar 3, 2020

This was referenced Mar 26, 2020

gnrc_netif: implementation of dynamic GNRC_NETIF_NUMOF approach #12994

Merged

netdev: add netdev_register() to keep track of netdev IDs #13746

Merged

benpicco mentioned this pull request May 25, 2020

drivers/edbg_eui: add driver to get MAC address from Atmel EDBG #14134

Merged

miri64 added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 25, 2020

benpicco closed this Jul 24, 2020

benpicco deleted the eui-get branch July 24, 2020 14:17

fjmolinas mentioned this pull request Aug 25, 2020

net/link_layer: implement EUI provider #14634

Merged

Conversation

benpicco commented Nov 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

Uh oh!

Uh oh!

benpicco commented Nov 7, 2019

Uh oh!

Uh oh!

Uh oh!

miri64 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benpicco commented Nov 10, 2019

Uh oh!

Uh oh!

maribu commented Nov 19, 2019

Uh oh!

benpicco commented Nov 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Dec 10, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miri64 commented Jan 30, 2020

Uh oh!

benpicco commented Jan 30, 2020

Uh oh!

miri64 commented Jun 25, 2020

Uh oh!

benpicco commented Jun 25, 2020

Uh oh!

benpicco commented Jul 24, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

benpicco commented Nov 4, 2019 •

edited

Loading

benpicco commented Nov 20, 2019 •

edited

Loading

benpicco commented Dec 10, 2019 •

edited

Loading