[riot-notifications] [RIOT-OS/RIOT] drivers/cc110x: add hook cc110x_eui_get() (#16286)

Marian Buschsieweke notifications at github.com
Wed Apr 7 21:15:14 CEST 2021

@maribu commented on this pull request.

see inline

> @@ -38,6 +40,21 @@ extern const netdev_driver_t cc110x_driver;
 void cc110x_on_gdo(void *dev);
+ * @brief   Retrieve a unique layer-2 address for a cc110x instance
+ *
+ * @note    This function has __attribute__((weak)) so you can override this, e.g.
+ *          to construct an address. By default, this function always returns false.
+ *          If this function returns false, @ref luid_get is used.
+ *
+ * @param[in]   dev     The device descriptor of the transceiver
+ * @param[out]  eui     Destination to write the address to
+ *
+ * @return  true, if an address has been written to @p eui, else false
+ */
+bool cc110x_eui_get(const netdev_t *dev, void *eui);


>      while (dev->addr == CC110X_L2ADDR_AUTO) {
-        luid_get(&dev->addr, 1);
+        _cc110x_eui_get(netdev, &dev->addr);

I have the feeling that this loop could cause issues, if the user provided `cc110x_eui_get()` would provide `CC110X_L2ADDR_AUTO` as EUI. I think just having an API contract that this address must not be returned and an `assert()` here to enforce this would solve the issue.

How about making `cc110x_eui_get()` return void and calling it here without the `while`? The `CC110X_L2ADDR_AUTO` mechanism was only a crude way to provide an L2 address via compile time flags. Since you are providing a proper mechanism in this PR, IMO we can just drop this macro and the `l2addr` member of `cc110x_params_t`. Also the documentation in `drivers/include/cc110x.h` lines 85 to 94 needs update - IMO just reference the `cc110x_eui_get` function there.

The default `weak` `cc110x_eui_get()` should loop until the returned byte differs from `CC1XXX_BCAST_ADDR` to match the API contract. (Actually, the loop is not needed, as the luid_get would do a pretty poor job in returning the same ID twice in two subsequent calls. But a `do`...`while` loop there should be more ROM efficient and is bullet proof, just in case a poor `luid_get()` gets used or the `init()` gets interrupted by someone doing excessive calls to `luid_get()`.)

Btw.: It might make sense to place the default `weak` implementation in `drivers/cc1xxx_common` and call the provider `cc1xxx_eui_get()`. Just in case a [cc1020 driver](https://github.com/RIOT-OS/RIOT/issues/658) magically gets upstream, this would be useful by both drivers.

You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20210407/c8836804/attachment-0001.htm>

More information about the notifications mailing list