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

Allow overriding get_mac() function in ieee802154 drivers #23193

Closed
markus-becker-tridonic-com opened this issue Mar 2, 2020 · 18 comments · Fixed by #24096
Closed

Allow overriding get_mac() function in ieee802154 drivers #23193

markus-becker-tridonic-com opened this issue Mar 2, 2020 · 18 comments · Fixed by #24096
Assignees
Labels
area: Networking Enhancement Changes/Updates/Additions to existing features
Milestone

Comments

@markus-becker-tridonic-com
Copy link
Contributor

Is your enhancement proposal related to a problem? Please describe.
Several IEEE802.15.4 drivers are using randomly generated MACs and the API does not offer to provide a manufacturer MAC/EUI64.

static inline u8_t *get_mac(struct device *dev)
{
}

Drivers that only use randomized MACs: rf2xx, mcr20a

uart_pipe, cc1200, cc2520 has the option to use randomized or static addresses.

cc13/cc26, nrf5 is using a MAC from the SoC register.

Describe the solution you'd like
It would be good to be able to override the get_mac function and allow providing the MAC from different sources.

Make get_mac a weak function?

@markus-becker-tridonic-com markus-becker-tridonic-com added the Enhancement Changes/Updates/Additions to existing features label Mar 2, 2020
@nandojve
Copy link
Member

nandojve commented Mar 2, 2020

@nandojve @tbursztyka @jukkar

@tbursztyka
Copy link
Collaborator

tbursztyka commented Mar 2, 2020

Forget about "get_mac()". In those drivers, it is just a utility function helped to set the IEEE extended address with necessary default value.

But there is already a way to set such IEEE extended address externaly, see include/net/ieee802154_mgmt.h header and more specifically NET_REQUEST_IEEE802154_SET_EXT_ADDR command code.

@tbursztyka
Copy link
Collaborator

see subsys/net/l2/ieee802154/ieee802154_shell.c for an actual example on how this interface is being used.

@markus-becker-tridonic-com
Copy link
Contributor Author

OK, found:

net_mgmt(NET_REQUEST_IEEE802154_SET_EXT_ADDR, iface,
                     addr, IEEE802154_EXT_ADDR_LENGTH))

Need to check it out and see whether that would work.

@nandojve
Copy link
Member

nandojve commented Mar 2, 2020

@tbursztyka Could be a good idea add local-mac-address on DT for tests purposes as alternative to random MAC?

    local-mac-address:
      type: uint8-array
      default: false
      description:
        Specifies the MAC address that was assigned to the network device

@markus-becker-tridonic-com
Copy link
Contributor Author

The problem with setting the MAC via net_mgmt(NET_REQUEST_IEEE802154_SET_EXT_ADDR, iface, addr, IEEE802154_EXT_ADDR_LENGTH)) or net_if_set_link_addr(iface, mac, 8, NET_LINK_IEEE802154); is that this is not possible before ieee802154_radio_api.iface_api.init() which internally calls ieee802154_init() and thus also openthread_init(), but needs to be done before. Currently, thus you can only modify each driver to hook your MAC in.

@rlubos
Copy link
Contributor

rlubos commented Mar 11, 2020

I'm a bit confused, do we talk about setting the 802.15.4 Extended Address or the factory assigned EUI-64? Yes, in core 802.15.4 they're the same, but OpenThread, for instance, differentiates them. Given the last comment from @markus-becker-tridonic-com I get an impression we talk about EUI-64, but just wanted to clarify.

In the case of the former, most (if not all) of the drivers use filter API function to set Extended Address, for instance:
https://github.com/zephyrproject-rtos/zephyr/blob/master/drivers/ieee802154/ieee802154_nrf5.c#L263

In case of the EUI-64, isn't it up to each driver to configure this? Just as @tbursztyka says, we can't rely on get_mac function, as it's internal for each driver and not all drivers have it.
Probably we could add a common Kconfig option to choose how MAC address should be obtained (random/static/hw-specific)? Still, each driver would need to respect these.

@markus-becker-tridonic-com
Copy link
Contributor Author

Sorry for the confusion. It was about the factory assigned EUI-64. Not all radio devices have this in silicon, e.g. RF233 requires another chip connected via I2C to provide the EUI-64 (AT24MAC402) or a custom method to provide your own EUI-64 from your own OUI range.

Probably we could add a common Kconfig option to choose how MAC address should be obtained (random/static/hw-specific)? Still, each driver would need to respect these.

This would be a first step, but even in the case of hardware specific there might be a need to be able to override the mechanism. A weak get_mac function, would make this possible.

@markus-becker-tridonic-com
Copy link
Contributor Author

How about this would be handled similarly to hwinfo_get_device_id()?

@nandojve
Copy link
Member

If I understand, maybe we can define a phandle on DT exclusive to get mac address for ifaces. That phandle represents a driver that can return MAC-48 and/or MAC-64. This must have preference over random MAC. In fact, a random MAC could be the default implementation (first driver) and default action to call. We can use DT local-mac-address Linux definition to define an alternative way for test purposes and for cases that phandle driver not exist yet. This alternative is a way that user don't need change code.

It seems that this way we can have an agnostic API, I'm not sure yet.

Could it be useful for other ifaces like ETH, WIFI etc?
Could it be called by upper layers instead by driver itself? I'm not sure if this question is an option.

I did some tests with local-mac-address and will share soon.

@markus-becker-tridonic-com
Copy link
Contributor Author

@nandojve looking forward to your proposal.

@nandojve
Copy link
Member

nandojve commented Apr 4, 2020

@markus-becker-tridonic-com The @24096 have small part of the solution that shows how to get local-mac-address from device tree. I'll start to work next week on the remaining part.

@nandojve
Copy link
Member

@markus-becker-tridonic-com I was thinking about keep this more simple now. I took you suggestion to implement method as weak. I only changed the method signature to avoid internal problems and refactored the internals. At this moment there are many things happen with DT and is better have everything stable. This way, you can have everything you want at Zephyr 2.3 release.

nandojve added a commit to nandojve/zephyr that referenced this issue Apr 24, 2020
Add a Kconfig option that allows user to set any necessary config
using management interface before interface be operational. A use
case is set the EUI-64 address from an external EEPROM by the
NET_REQUEST_IEEE802154_SET_EXT_ADDR command. After all configs
are done net_if_up() can be invoked to bring interface up.

Fixes zephyrproject-rtos#23193.

Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
jukkar pushed a commit that referenced this issue Apr 28, 2020
Add a Kconfig option that allows user to set any necessary config
using management interface before interface be operational. A use
case is set the EUI-64 address from an external EEPROM by the
NET_REQUEST_IEEE802154_SET_EXT_ADDR command. After all configs
are done net_if_up() can be invoked to bring interface up.

Fixes #23193.

Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
sandeepbrcm pushed a commit to Broadcom/zephyr that referenced this issue Apr 30, 2020
Add a Kconfig option that allows user to set any necessary config
using management interface before interface be operational. A use
case is set the EUI-64 address from an external EEPROM by the
NET_REQUEST_IEEE802154_SET_EXT_ADDR command. After all configs
are done net_if_up() can be invoked to bring interface up.

Fixes zephyrproject-rtos#23193.

Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
@markus-becker-tridonic-com
Copy link
Contributor Author

markus-becker-tridonic-com commented May 26, 2020

The solution from #23193 (comment) does not work with OpenThread because that is the 802.15.4 management interface.

This seems to work:

    uint8_t *pMac;

   // pMAC[0] = ...;
   // ...

#if defined(CONFIG_IEEE802154_RF2XX_NET_IF_NO_AUTO_START)
    /* setting MAC address ... */
    struct net_if *iface = net_if_get_first_by_type(&NET_L2_GET_NAME(OPENTHREAD));
    __ASSERT(iface != NULL, "OT iface NULL");
	net_if_set_link_addr(iface, pMac, 8, NET_LINK_IEEE802154);

    /* ... and then manually bringing up the interface */
    net_if_up(iface);
#endif

@markus-becker-tridonic-com
Copy link
Contributor Author

markus-becker-tridonic-com commented May 26, 2020

Would it be possible to make the option CONFIG_IEEE802154_RF2XX_NET_IF_NO_AUTO_START not RF2XX only, but CONFIG_IEEE802154_NET_IF_NO_AUTO_START since there are other radio drivers for which the MAC needs to be set as well? Additionally, we have seen start-up order problems when you use OpenThread with LittleFS as storage backend, where it gets hacky to have flash ready and filesystem mounted before OpenThread start-up.

@jukkar
Copy link
Member

jukkar commented May 26, 2020

CONFIG_IEEE802154_NET_IF_NO_AUTO_START since there are other radio drivers for which the MAC needs to be set as well

@markus-becker-tridonic-com That is a good idea. Can you send a PR for that?

@markus-becker-tridonic-com
Copy link
Contributor Author

@jukkar should that then be in ieee802154_init() in subsys/net/l2/openthread/openthread.c and subsys/net/l2/ieee802154/ieee802154.c?

@markus-becker-tridonic-com
Copy link
Contributor Author

@jukkar PR: #25629

@nandojve nandojve added this to the v2.3.0 milestone May 28, 2020
hakehuang pushed a commit to hakehuang/zephyr that referenced this issue Jun 20, 2020
Add a Kconfig option that allows user to set any necessary config
using management interface before interface be operational. A use
case is set the EUI-64 address from an external EEPROM by the
NET_REQUEST_IEEE802154_SET_EXT_ADDR command. After all configs
are done net_if_up() can be invoked to bring interface up.

Fixes zephyrproject-rtos#23193.

Signed-off-by: Gerson Fernando Budke <nandojve@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Networking Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants