net: add Skald, a BLE advertising stack by haukepetersen · Pull Request #8884 · RIOT-OS/RIOT

haukepetersen · 2018-04-05T09:58:16Z

Contribution description

This PR adds Skald to RIOT, a BLE advertising stack written from scratch. Skald is focused on the BLE broadcaster role only, but for this he is extremely small, allowing for very memory efficient implementation of all kind of BLE beacons (e.g. a iBeacon takes ~5k ROM with everything).

Currently, Skald supports Apple's iBeacons, Google's Eddystone beacons, and sending out custom GAP payloads.

This PR also contains a netdev tailoring for using BLE radios, as well as a BLE radio driver implementation for the nrf5x family of devices.

Note: currently only the nrf52dk is supported (and tested), other platforms will be added step-by-step

Testing

flash the included iBeacon or the Eddystone example on a nrf52dk
use any iOS or Android app that can scan for BLE devices to verify the beacon, e.g. Nordic's nRF Connect App (for Android or iOS). Alternatively you can use a Bluetooth 4.0 capable Linux and use Bluez hcitools... Or something else...

Issues/PRs references

Skald is a spin-off of my current BLE implementation efforts, a full BLE stack will follow soon...

miri64

First iteration of review.

miri64 · 2018-04-05T11:06:41Z

cpu/nrf5x_common/include/nrfble.h

+ *
+ * @note        This driver is not thread safe (as of now)
+ *
+


Newline ;-)

miri64 · 2018-04-05T11:08:08Z

cpu/nrf5x_common/include/nrfble.h

+ * @note        This driver is not thread safe (as of now)
+ *
+
+ * @todo        add support for netdev_ble_pkt_ext_t


Given that this will show up in the documentation, maybe some more detailed elaboration on what this means would be great.

its outdated, removed.

miri64 · 2018-04-05T11:09:29Z

cpu/nrf5x_common/include/nrfble.h

+/**
+ * @brief   Reference to the netdev driver interface
+ */
+extern const netdev_driver_t nrfble_netdev;


Does this actually need to be exposed? This is set by nrfble_setup() inside _nrfble_dev, and used nowhere else. Also, the name is confusing. How about nrfble_driver (even when you internalize it)?

will remove.

miri64 · 2018-04-05T11:10:03Z

cpu/nrf5x_common/include/nrfble.h

+/**
+ * @brief   Export the netdev device descriptor
+ */
+extern netdev_t nrfble_dev;


This doesn't have a corresponding internal definition. Also is this required to be public? nrfble_setup() returns a pointer to _nrfble_dev (which I assume to be the fragment this used to expose earlier (?))

miri64 · 2018-04-05T11:14:41Z

cpu/nrf5x_common/radio/nrfble/nrfble.c

@@ -0,0 +1,357 @@
+/*
+ * Copyright (C) 2017 Freie Universität Berlin


nope, 2017-2018 :-)

miri64 · 2018-04-05T12:06:39Z

sys/include/net/skald/ibeacon.h

+ *
+ * # About
+ * This Skald module supports the creation and advertisement of BLE iBeacons as
+ * defined by Apple (see https://developer.apple.com/ibeacon/).


Same proposal here:

This Skald module supports the creation and advertisement of BLE [iBeacons](https://developer.apple.com/ibeacon/) as defined by Apple.

Actually I like the visibly printed link better, so I will leave it.

the NACK was superfluous, github interface didn't show my first comment....

miri64 · 2018-04-05T12:08:58Z

sys/net/skald/skald.c

+    /* advertise on the next adv channel - or skip this event if the radio is
+     * busy */
+    if (_radio->context != NULL) {
+


Is there something missing here? Otherwise: the if is redundant.

debugging leftover, removed.

miri64 · 2018-04-05T12:13:04Z

sys/net/skald/skald.c

+{
+    assert(buf);
+
+    luid_get(buf, BLE_ADDR_LEN);


AFAIK, BLE addresses are MAC addresses as defined in IEEE 802. Please at least set the universal/local bit to local and the individual/group bit to individual after calling this:

buf[0] &= 0x01; buf[0] |= 0x02;

miri64 · 2018-04-05T12:24:25Z

sys/include/net/skald.h

+ *
+ * @param[out] buf      the generated address is written to this buffer
+ */
+void skald_generate_random_addr(uint8_t *buf);


random might not be a good term here, since the result of get_luid() is not random, but pretty predeterminable.

but the bluetooth standard calls this type of address random.

miri64 · 2018-04-05T12:29:40Z

sys/net/skald/skald_eddystone.c

+    pre_t pre;
+    uint8_t tx_pwr;
+    uint8_t scheme;
+    uint8_t url;


The URL is only one byte long? oO

Nope, but this field is only used for its address.

Maybe use url[] instead then to make this clearer. (additional benefit, you don't need the address operator & to get the pointer ;-)).

miri64

Some more doc stuff I noticed when browsing through the rendered doc.

miri64 · 2018-04-05T12:55:25Z

cpu/nrf5x_common/include/nrfble.h

+ */
+
+/**
+ * @defgroup    drivers_nrf5x_nrfble nRF BLE radio driver


For other nRF drivers "NRF" (all upper case) was used was used in the group name. It took me a moment to find it ;-)

miri64 · 2018-04-05T13:03:31Z

drivers/include/net/netdev/ble.h

+        uint32_t u32;                       /**< compact access */
+    } aa;                                   /**< access address */
+    uint32_t crc;                           /**< CRC: 3 LSB for CRC, most
+                                             *   significant bit for RX state*/


Maybe use a top comment for that and while doing so, you can make a graphic (but don't have to):

/** * @brief CRC and RX state * * 3 LSB for CRC, most significant bit for RX state (R, marks if the CRC is * correct): * * 3 2 1 0 * 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 9 8 7 6 5 4 3 2 1 0 * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ * |R| reserved | CRC | * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ */

miri64 · 2018-04-05T13:05:46Z

sys/include/net/ble.h

+ */
+
+/**
+ * @defgroup    net_ble General BLE defines


I think "Bluetooth Low Energy (BLE)" is easier to find in the list of modules. At least I wouldn't search under "G" for the Bluetooth definitions ;-)

I don't use the word 'bluetooth' anywhere on purpose, as I am not clear on the copyright/usage rights on that word and I have the dimm memory that there was trouble for others in the past using that word without the ok of the bluetooth SIG

miri64 · 2018-04-05T13:06:20Z

sys/include/net/ble.h

+#endif
+
+/**
+ * @name    BT version constants


Mainly, because just BT in a heading looks weird IMHO:

miri64 · 2018-04-05T13:07:40Z

sys/include/net/eddystone.h

+ */
+
+/**
+ * @defgroup    net_eddystone Eddystone Constants


Drop the "Constants" for the group name.

miri64 · 2018-04-05T13:09:26Z

sys/include/net/eddystone.h

+ * @name    URL scheme prefix values
+ * @{
+ */
+#define EDDYSTONE_URL_HTTP_WWW          (0x00)      /**< http://www.URL */


use ` around these URLs here and below:

/**< `http://www.URL` */

Doxygen interprets these as valid URLs (which leads to nowhere of course) with the current usage.

miri64 · 2018-04-05T13:11:36Z

sys/include/net/skald.h

+ * @brief   Generate a random public address
+ *
+ * @todo    Make sure resulting address does not conflict with and assigned
+ *          vendor prefix (however we will do that..)


That is guaranteed, when you set the universal/local bit to local ;-).

miri64 · 2018-04-05T13:12:08Z

sys/include/net/skald/eddystone.h

+ */
+
+/**
+ * @defgroup    net_skald_ Skald meets Eddy


net_scald_eddystone?

miri64 · 2018-04-05T13:12:20Z

sys/include/net/skald/eddystone.h

+ */
+
+/**
+ * @defgroup    net_skald_ Skald meets Eddy


"Skald meets Eddystone"?

(but maybe I missed the joke here ;-))

miri64 · 2018-04-05T14:11:26Z

Noticed during testing: README (can be an adjusted copy of what you wrote in OP IMHO) for the examples is missing.

aabadie · 2018-04-05T14:49:00Z

General comment (sorry): I am very happy with this BLE initiative but I don't like the skald name. It think that it doesn't means anything regarding ble. Why not simply using ble_advertising ? The name is longer but way more explicit.

Actually, we have the same problem with emcute, IMHO it should have been called mqtt-sn to be explicit.

If those 2 projects (skald and emcute) were external libraries imported as packages, it would have been perfectly fine but since they are designed as implementation of standard protocols within RIOT (and with respect to highly constrained devices), they should use the standard names.

haukepetersen · 2018-04-05T15:21:58Z

I completely disagree regarding the name(s)! Naming an implementation after the protocol it implements is IMHO not a good idea - especially in the field of contrained devices where there is a tendency to have multiple implementations with different scopes!

And who says that Skald (or also emcute) is the default/standard implementation in RIOT for BLE advertising (or mqtt-sn, respectively)? They are simply the only ones we have currently...

And in the end I am the author, and I insist on this name :-)

aabadie · 2018-04-05T15:37:12Z

Naming an implementation after the protocol it implements is IMHO not a good idea - especially in the field of contrained devices where there is a tendency to have multiple implementations with different scopes!

Then they should be provided as external libraries. Implementation contributed into RIOT should use standard names and try to implement in a standard (whatever this means).

They are simply the only ones we have currently...

Indeed, but the problem is that they hide the underlying features instead of advertising them. I think I already said to someone asking if RIOT has support for MQTT that the example application was called emcute. Reply: "Ah ok, I missed it"

miri64 · 2018-04-05T15:44:04Z

Tested on nrf52dk with an android app as partner. Works.

miri64 · 2018-04-05T15:44:44Z

Indeed, but the problem is that they hide the underlying features instead of advertising them. I think I already said to someone asking if RIOT has support for MQTT that the example application was called emcute. Reply: "Ah ok, I missed it"

I think this is more a matter of documentation than of naming.

haukepetersen · 2018-04-05T15:47:16Z

But why is GNRC called GNRC instead of ip_stack?

haukepetersen · 2018-04-05T15:51:24Z

@miri64 are you sufficiently happy with the doc now? I know its not perfect (yet), but considering this being a completely new feature (with a high probability of significant structural changes), I tend to rather let this module settle a bit before detailing out the doc to a 'perfect' state...

haukepetersen · 2018-04-05T15:51:45Z

I think this is more a matter of documentation than of naming.

+1

miri64

One minor thing left that I think can be improved code-wise (see last comment in this review). Otherwise, I think you can squash.

miri64 · 2018-04-05T17:35:51Z

drivers/include/net/netdev/ble.h

+    uint8_t flags;                          /**< header flags */
+    uint8_t len;                            /**< actual length of PDU */
+    uint8_t pdu[NETDEV_BLE_PDU_MAXLEN];     /**< protocol data unit (PDU) */
+} netdev_ble_pkt_t;


Fine then let's keep it here for now. But when we finalize this API, I think it would still be better to move it somewhere else.

miri64 · 2018-04-05T17:36:09Z

drivers/include/net/netdev/ble.h

+    uint32_t crc;                           /**< CRC: 3 LSB for CRC, most
+                                             *   significant bit for RX state*/
+    uint8_t chan;                           /**< channel to use/used */
+} netdev_ble_ctx_t;


miri64 · 2018-04-05T17:36:34Z

sys/include/net/ble.h

+#endif
+
+/**
+ * @name    BT version constants


miri64 · 2018-04-05T17:37:16Z

sys/include/net/skald.h

+ *
+ * @param[out] buf      the generated address is written to this buffer
+ */
+void skald_generate_random_addr(uint8_t *buf);


miri64 · 2018-04-05T17:41:44Z

sys/net/skald/skald_eddystone.c

+    pre_t pre;
+    uint8_t tx_pwr;
+    uint8_t scheme;
+    uint8_t url;


Maybe use url[] instead then to make this clearer. (additional benefit, you don't need the address operator & to get the pointer ;-)).

aabadie · 2018-04-05T21:03:13Z

I think this is more a matter of documentation than of naming.

Just give the application a meaningful name, e.g. mqtt-sn, ble-eddystone, ble-ibeacon and everything will be fine.

Personally (and I'm sure I'm not the only one), I will be very difficult to convince with the name skald. This name inspires nothing related to ble, even uble would have been better. I also understand that changing the name you chose for your implementation will probably be even more difficult.

haukepetersen · 2018-04-06T08:12:24Z

even uble would have been better.

Very subjective...

I guess we agree to not agree on the naming thing, but as the original author I think I have the right to chose a name to my liking :-)

haukepetersen · 2018-04-06T08:32:03Z

@miri64 for some reason I don't seem to be allowed to comment on your last review comments...

Maybe use url[] instead then to make this clearer. (additional benefit, you don't need the address operator & to get the pointer ;-)).

fixed.

miri64

Please squash.

haukepetersen · 2018-04-06T08:48:59Z

my pleasure :-)

miri64

ACK

miri64 · 2018-04-06T09:17:15Z

Ah, the netopt module needs a new string in its array btw ;-).

Skald is a very small and simple, TX-only BLE stack that supports sending advertisements only. It is useful for building all kinds of BLE beacons with very minimal memory footprints.

haukepetersen · 2018-04-06T09:19:19Z

Ah, the netopt module needs a new string in its array btw ;-).

fixed and squashed right in

miri64 · 2018-04-06T09:26:50Z

And go.

haukepetersen · 2018-04-06T09:31:44Z

Thanks for the quick review! This makes is much easier to continue the actual BLE work :-)

Documentation of the option types was clarified in RIOT-OS#8655. I only noticed after merging RIOT-OS#8884, that `NETOPT_BLE_CTX` was not documented using that new style. So I deliver the change myself to make it quicker.

miri64 · 2018-04-06T09:37:01Z

sys/include/net/netopt.h

     */
    NETOPT_TX_RETRIES_NEEDED,

+    NETOPT_BLE_CTX,             /**< set radio context (channel, CRC, AA) */


Arghs, this doesn't uphold the new doc style for netopts introduced in #8655. Fix provided in #8892.

emmanuelsearch · 2018-04-06T14:59:31Z

awesome ;)

Documentation of the option types was clarified in RIOT-OS#8655. I only noticed after merging RIOT-OS#8884, that `NETOPT_BLE_CTX` was not documented using that new style. So I deliver the change myself to make it quicker.

haukepetersen assigned miri64 and kaspar030 Apr 5, 2018

miri64 requested changes Apr 5, 2018

View reviewed changes

miri64 reviewed Apr 5, 2018

View reviewed changes

miri64 reviewed Apr 6, 2018

View reviewed changes

miri64 approved these changes Apr 6, 2018

View reviewed changes

haukepetersen added 5 commits April 6, 2018 11:18

drivers/netdev: added BLE adaption for netdev

4e715e8

cpu/nrf5x: added nrfble radio driver

a92b577

boards/nrf52xxxdk: added BLE radio features

2c01034

sys/net: added generic BLE defines

3b6c8f2

sys/net: added generic Eddystone defines

7749990

haukepetersen added 5 commits April 6, 2018 11:18

sys/net: added Skald

49bd85d

Skald is a very small and simple, TX-only BLE stack that supports sending advertisements only. It is useful for building all kinds of BLE beacons with very minimal memory footprints.

sys/auto_init: added initialization for Skald

6efb06f

examples: added Skald iBeacon example

e4176f2

examples: added Skald Eddystone example

fcc6f3a

boards/nrf52xxxdk: add Skald dependencies

ceb7640

haukepetersen force-pushed the add_skald branch from 068b5ec to ceb7640 Compare April 6, 2018 09:19

miri64 merged commit a19cb84 into RIOT-OS:master Apr 6, 2018

miri64 deleted the add_skald branch April 6, 2018 09:26

miri64 restored the add_skald branch April 6, 2018 09:27

haukepetersen deleted the add_skald branch April 6, 2018 09:31

miri64 mentioned this pull request Apr 6, 2018

netopt: move NETOPT_BLE_CTX to new netopt doc style #8892

Merged

miri64 reviewed Apr 6, 2018

View reviewed changes

miri64 mentioned this pull request Apr 6, 2018

examples: rename MQTT-SN example for clarity #8893

Merged

miri64 added a commit to miri64/RIOT that referenced this pull request Apr 6, 2018

fixup! Merge pull request RIOT-OS#8884 from haukepetersen/add_skald

8b3824c

haukepetersen mentioned this pull request Apr 9, 2018

examples/skald: clarifying names of examples #8900

Closed

		@@ -0,0 +1,357 @@
		/*
		* Copyright (C) 2017 Freie Universität Berlin

Conversation

haukepetersen commented Apr 5, 2018

Contribution description

Testing

Issues/PRs references

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 Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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 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!

miri64 Apr 5, 2018 •

edited

Loading