[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);
+
+

```suggestion
```

>      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:
https://github.com/RIOT-OS/RIOT/pull/16286#pullrequestreview-630432703
-------------- 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