drivers: nrf: Added config option for OUI. #27283

kkasperczyk-no · 2020-07-31T09:57:50Z

Support for defining vendor specific OUI (Organizationally
Unique Identifier) was added.

Signed-off-by: Kamil Kasperczyk kamil.kasperczyk@nordicsemi.no

jukkar · 2020-07-31T11:15:32Z

drivers/ieee802154/Kconfig.nrf5

+	default 16043574
+	help
+	  Custom vendor OUI for OpenThread,
+	  which makes 24 oldest bits of MAC address


What does oldest mean here, do you mean lowest instead?

Sorry I think I literally copied meaning from Polish language and I didn't realize that it's incomprehensive. I meant 24 most-significant bits and i fixed it.

markus-becker-tridonic-com · 2020-08-03T06:19:52Z

On other radio chips this was solved through IEEE802154_NET_IF_NO_AUTO_START (#25629) and calls to net_if_set_link_addr(iface, mac, 8, NET_LINK_IEEE802154). If this is accepted, it should be reworked to allow to do the same for other radio chips as well.

@rlubos @nandojve

kkasperczyk-no · 2020-08-03T08:28:58Z

On other radio chips this was solved through IEEE802154_NET_IF_NO_AUTO_START (#25629) and calls to net_if_set_link_addr(iface, mac, 8, NET_LINK_IEEE802154). If this is accepted, it should be reworked to allow to do the same for other radio chips as well.

@rlubos @nandojve

Could you specify what changes exactly do you expect? I'm not sure if I understand correctly, but I have the impression that my change has quite different purpose, that the one mentioned by you. You suggested to call net_if_set_link_addr() method to set EUI64, what actually is done. My change lets user to define custom, vendor (e.g. Nordics) specific OUI address and to provide own method for generating EUI64, what might be useful in some cases. To sum up it doesn't change the way and the place of setting EUI64, but only the way of generating it in some specific situations.

nandojve · 2020-08-03T14:28:10Z

On other radio chips this was solved through IEEE802154_NET_IF_NO_AUTO_START (#25629) and calls to net_if_set_link_addr(iface, mac, 8, NET_LINK_IEEE802154). If this is accepted, it should be reworked to allow to do the same for other radio chips as well.
@rlubos @nandojve

Could you specify what changes exactly do you expect? I'm not sure if I understand correctly, but I have the impression that my change has quite different purpose, that the one mentioned by you. You suggested to call net_if_set_link_addr() method to set EUI64, what actually is done. My change lets user to define custom, vendor (e.g. Nordics) specific OUI address and to provide own method for generating EUI64, what might be useful in some cases. To sum up it doesn't change the way and the place of setting EUI64, but only the way of generating it in some specific situations.

I think what @markus-becker-tridonic-com said is, we developed a standard way to proper set EUI64 for all devices and situations. Maybe we forget to cover something.

Could you tell us what are the useful situations because it may be relevant to all drivers?

kderda · 2020-08-04T14:04:47Z

@nandojve @markus-becker-tridonic-com As far as I understand the #25629 lets us configure the EUI before the interface is started but if I understand correctly it still has to be then started "manually" somewhere in the code so the user (in our understanding - product/device developer) has to modify existing code or add it to e.g. main thread. I believe that this PR has two "pros" compared to this such solution:

It's more explicit. After you enable the IEEE802154_NRF5_EUI64_CUSTOM_SOURCE you are forced to provide the function implementation at linking stage. To my mind is also more consistent with Zephyr configuration style - you manipulate the configuration of drivers, interfaces, protocols etc. before building the software and you might even have empty "main thread" because everything is set up in the background. Here, while you have to provide a function that fetches an EUI (e.g. from NVM) somewhere in the sources - it is called by the driver instead of some other context.
Example use case: This lets to provide already compiled versions of libraries for product developers (e.g. already certified) with weak implementation of this function which can be easily replaced by the user at build time.

All in all I belive that these changes are not conflicting and CONFIG_IEEE802154_NET_IF_NO_AUTO_START might still be useful for developers.

nandojve · 2020-08-04T15:25:55Z

@nandojve @markus-becker-tridonic-com As far as I understand the #25629 lets us configure the EUI before the interface is started but if I understand correctly it still has to be then started "manually" somewhere in the code so the user (in our understanding - product/device developer) has to modify existing code or add it to e.g. main thread. I believe that this PR has two "pros" compared to this such solution:

It's more explicit. After you enable the IEEE802154_NRF5_EUI64_CUSTOM_SOURCE you are forced to provide the function implementation at linking stage. To my mind is also more consistent with Zephyr configuration style - you manipulate the configuration of drivers, interfaces, protocols etc. before building the software and you might even have empty "main thread" because everything is set up in the background. Here, while you have to provide a function that fetches an EUI (e.g. from NVM) somewhere in the sources - it is called by the driver instead of some other context.

Example use case: This lets to provide already compiled versions of libraries for product developers (e.g. already certified) with weak implementation of this function which can be easily replaced by the user at build time.

All in all I belive that these changes are not conflicting and CONFIG_IEEE802154_NET_IF_NO_AUTO_START might still be useful for developers.

Ok, I understand you point, we did a generic weak method proposal at #24096, that could be extended to all 15.4 drivers and was not accepted that time. Now, we are revisiting exactly same thing here and this is what @markus-becker-tridonic-com raised. The "manually" was introduced as a solution to sync 15.4 initialization because driver can not proper set EUI without a weak method.

My point is, we need follow same approach between all drivers to be consistent. Today, 15.4 drivers can only set EUI from a external source using management interface because weak methods are not allowed.

I saw two paths here: 1- keep EUI from external source only by management interface; 2- introduce EUI from external source using a generic weak function as an second option. This second option must be an standard for, at least, 15.4 drivers and should be available at API header because others will need it too for the same reasons.

I'll add DNM till we define the right path here, since we have more than 2 green lights.

jukkar

My point is, we need follow same approach between all drivers to be consistent. Today, 15.4 drivers can only set EUI from a external source using management interface because weak methods are not allowed.

I agree, it would be better if the drivers are working in the same way. What this way is, we can debate about that. Withdrawing my +1

kkasperczyk-no · 2020-08-05T09:02:33Z

@nandojve @markus-becker-tridonic-com @konradderda @jukkar I have a proposition then. I will remove this EUI64 custom source changes for now, as you find it controversial and I would like to push only OUI change, which is directed to specific (nrf) radio driver. We will also create corresponding issue with the proposition of problem solution and we will tag you to discuss it soon. Do you accept such approach?

nandojve · 2020-08-05T11:49:45Z

drivers/ieee802154/Kconfig.nrf5

@@ -42,4 +42,12 @@ config IEEE802154_NRF5_EXT_IRQ_MGMT
 	  the system. One example of external radio IRQ provider could be
 	  a radio arbiter used in dynamic multiprotocol applications.

+config IEEE802154_NRF5_VENDOR_OUI


Is this be valid for other drivers? If yes, it could be added as IEEE802154_VENDOR_OUI at Kconfig instead Kconfig.nrf5.

I can move it, as you suggest, to general drivers Kconfig and not assign default value to it. Thanks to this it will be possible to check in specific driver files if value was set by someone and that value should be used or it wasn't and for example some default, driver defined value can be set. Do you find it being useful for other drivers? Main use case is to have some constant value, specific for given driver and use it in most cases, but also to not close the doors for some vendors and users, which might want to set own value. I think that such approach doesn't give much benefit, as with generalization of this config, each driver will have to define own default value anyway. Finding the best default value for all drivers is not possible, as this value defines specific vendor and it can't be general. Please consider also that setting OUI at the configuration level might be valid for some other drivers, but not necessarily for all, as some chips have their addresses set in the factory and it's not possible to change them. I understand your statement that all drivers should work in the same way, so please verify if described solution would be better in your opinion.

If there is no objection, I do prefer define IEEE802154_VENDOR_OUI at Kconfig. As far I understand, you can silent define your own default OUI at Kconfig.nrf5, for instance, since symbol now in 15.4 scope. User always have the possibility to override Kconfig symbols.

I'm not sure if checking !defined(IEEE802154_VENDOR_OUI) is the best approach. Because if in the future we define a default value the logic will be broken. Maybe ignore vendor OUI if value is Zero could be more reliable. Remember that you can override OUI at any place. Because of that I think is better to define a default value.

nandojve · 2020-08-05T12:00:22Z

drivers/ieee802154/Kconfig.nrf5

@@ -42,4 +42,12 @@ config IEEE802154_NRF5_EXT_IRQ_MGMT
 	  the system. One example of external radio IRQ provider could be
 	  a radio arbiter used in dynamic multiprotocol applications.

+config IEEE802154_NRF5_VENDOR_OUI
+	int "Vendor Organizationally Unique Identifier"
+	default 16043574


Then, this could be Zephyr Project OUI, if we had one, otherwise networking folks can suggest better value. I'm thinking to use same approach from USB_DEVICE_VID, for instance.

I answered on this in above comment

nandojve · 2020-08-05T12:13:22Z

@nandojve @markus-becker-tridonic-com @konradderda @jukkar I have a proposition then. I will remove this EUI64 custom source changes for now, as you find it controversial and I would like to push only OUI change, which is directed to specific (nrf) radio driver. We will also create corresponding issue with the proposition of problem solution and we will tag you to discuss it soon. Do you accept such approach?

Sure, please, make it and let's fix this.

nandojve · 2020-08-06T13:14:56Z

drivers/ieee802154/ieee802154_nrf5.c

+#if CONFIG_IEEE802154_VENDOR_OUI
+#define IEEE802154_NRF5_VENDOR_OUI CONFIG_IEEE802154_VENDOR_OUI
+#else
+#define IEEE802154_NRF5_VENDOR_OUI (uint32_t)0xF4CE36
+#endif


Not a block

You can define at Kconfig.nrf the default value, since its 15.4 global scope
config IEEE802154_VENDOR_OUI
default 0xF4CE36

When you want to change it, add below at app.conf file, for instance.
CONFIG_IEEE802154_VENDOR_OUI=XXXX

I believe that it's ok to keep this value in the driver's sources since it's a constant. With the solution proposed above (IEEE802154_VENDOR_OUI is a boolean) we can easily determine if this option is used or not.

LuDuda · 2020-08-06T13:29:28Z

drivers/ieee802154/Kconfig

+	int "Vendor Organizationally Unique Identifier"
+	default 0
+	help
+	  Custom vendor OUI for OpenThread,


Can we stop referring to OpenThread? It's generic 802.15.4 driver configuration.

Sure I forget to change it moving from somewhere else. Good point, done.

kderda · 2020-08-06T14:24:03Z

drivers/ieee802154/Kconfig

@@ -39,6 +39,13 @@ config IEEE802154_RDEV
 	help
 	  PHY is a ranging-capable device (RDEV)

+config IEEE802154_VENDOR_OUI
+	int "Vendor Organizationally Unique Identifier"
+	default 0


We cannot use any default value here. I would rather change this to boolean and add another one like IEEE802154_VENDOR_OUI_VALUE without default value to ensure that user will have to fill this option when IEEE802154_VENDOR_OUI is enabled.

kderda · 2020-08-06T14:36:32Z

drivers/ieee802154/ieee802154_nrf5.c

+#if CONFIG_IEEE802154_VENDOR_OUI
+#define IEEE802154_NRF5_VENDOR_OUI CONFIG_IEEE802154_VENDOR_OUI
+#else
+#define IEEE802154_NRF5_VENDOR_OUI (uint32_t)0xF4CE36
+#endif


I believe that it's ok to keep this value in the driver's sources since it's a constant. With the solution proposed above (IEEE802154_VENDOR_OUI is a boolean) we can easily determine if this option is used or not.

kkasperczyk-no · 2020-08-07T08:52:48Z

To sum up, as there was a lot of discussions about OUI approach:

IEEE802154_VENDOR_OUI is located in global 15.4 Kconfig to be useful for all drivers
IEEE802154_VENDOR_OUI can't have any default value, as there is not a reserved value defined in RFC, which could be used for it (especially 0 value is used by some company).
Overwriting option in Kconfig.nrf file wasn't done to not define option storing constant value and also to not break naming convention in Kconfig files.
IEEE802154_VENDOR_OUI_ENABLE option is used to enable feature and when set - it forces user to define own OUI value, as there is no default.

Support for defining vendor specific OUI (Organizationally Unique Identifier) was added. Signed-off-by: Kamil Kasperczyk <kamil.kasperczyk@nordicsemi.no>

kkasperczyk-no requested review from jukkar and tbursztyka as code owners July 31, 2020 09:57

tbursztyka approved these changes Jul 31, 2020

View reviewed changes

LuDuda approved these changes Jul 31, 2020

View reviewed changes

jukkar reviewed Jul 31, 2020

View reviewed changes

kkasperczyk-no force-pushed the oui_pr branch from b63cf68 to e237e1a Compare July 31, 2020 11:50

jukkar approved these changes Jul 31, 2020

View reviewed changes

carlescufi requested a review from rlubos August 3, 2020 14:21

nandojve added the DNM This PR should not be merged (Do Not Merge) label Aug 4, 2020

nandojve self-requested a review August 4, 2020 15:27

jukkar added area: IEEE 802.15.4 area: Networking area: OpenThread labels Aug 5, 2020

jukkar requested changes Aug 5, 2020

View reviewed changes

kkasperczyk-no force-pushed the oui_pr branch 2 times, most recently from 8106e5c to 26a073e Compare August 5, 2020 09:10

kkasperczyk-no changed the title ~~drivers: nrf: Added config options for EUI64 and OUI.~~ drivers: nrf: Added config option for OUI. Aug 5, 2020

nandojve requested changes Aug 5, 2020

View reviewed changes

nandojve removed the DNM This PR should not be merged (Do Not Merge) label Aug 5, 2020

kkasperczyk-no force-pushed the oui_pr branch from 26a073e to ac45631 Compare August 6, 2020 11:30

kkasperczyk-no requested review from jukkar and nandojve August 6, 2020 11:31

nandojve reviewed Aug 6, 2020

View reviewed changes

nandojve approved these changes Aug 6, 2020

View reviewed changes

LuDuda reviewed Aug 6, 2020

View reviewed changes

kkasperczyk-no force-pushed the oui_pr branch from ac45631 to e0f3c30 Compare August 6, 2020 14:18

kderda suggested changes Aug 6, 2020

View reviewed changes

kkasperczyk-no force-pushed the oui_pr branch from e0f3c30 to 52316ed Compare August 7, 2020 08:19

drivers: nrf: Added config option for OUI.

110b9c2

Support for defining vendor specific OUI (Organizationally Unique Identifier) was added. Signed-off-by: Kamil Kasperczyk <kamil.kasperczyk@nordicsemi.no>

kkasperczyk-no force-pushed the oui_pr branch from 52316ed to 110b9c2 Compare August 7, 2020 09:02

jukkar approved these changes Aug 7, 2020

View reviewed changes

kderda approved these changes Aug 7, 2020

View reviewed changes

LuDuda approved these changes Aug 9, 2020

View reviewed changes

jukkar merged commit e4d20ca into zephyrproject-rtos:master Aug 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: nrf: Added config option for OUI. #27283

drivers: nrf: Added config option for OUI. #27283

kkasperczyk-no commented Jul 31, 2020 •

edited

Loading

jukkar Jul 31, 2020

kkasperczyk-no Jul 31, 2020

markus-becker-tridonic-com commented Aug 3, 2020

kkasperczyk-no commented Aug 3, 2020

nandojve commented Aug 3, 2020 •

edited

Loading

kderda commented Aug 4, 2020

nandojve commented Aug 4, 2020

jukkar left a comment

kkasperczyk-no commented Aug 5, 2020

nandojve Aug 5, 2020

kkasperczyk-no Aug 5, 2020

nandojve Aug 5, 2020

nandojve Aug 5, 2020

kkasperczyk-no Aug 5, 2020

nandojve commented Aug 5, 2020

nandojve Aug 6, 2020

kderda Aug 6, 2020

LuDuda Aug 6, 2020

kkasperczyk-no Aug 6, 2020

kderda Aug 6, 2020

kderda Aug 6, 2020

kkasperczyk-no commented Aug 7, 2020

drivers: nrf: Added config option for OUI. #27283

drivers: nrf: Added config option for OUI. #27283

Conversation

kkasperczyk-no commented Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markus-becker-tridonic-com commented Aug 3, 2020

kkasperczyk-no commented Aug 3, 2020

nandojve commented Aug 3, 2020 • edited Loading

kderda commented Aug 4, 2020

nandojve commented Aug 4, 2020

jukkar left a comment

Choose a reason for hiding this comment

kkasperczyk-no commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nandojve commented Aug 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kkasperczyk-no commented Aug 7, 2020

kkasperczyk-no commented Jul 31, 2020 •

edited

Loading

nandojve commented Aug 3, 2020 •

edited

Loading