simple ble interface #623

niklasad1 · 2017-09-24T18:01:56Z

Pull Request Overview

This pull request simplifies the Bluetooth Low Energy API in user-space

Testing Strategy

This pull request was tested by running the test application tutorials/xx_ble_adv

A screenshot from Nordic Connect Android App can be found here:
Screenshot

TODO or Help Wanted

I want feedback on the BLE API and what should be different etc?
@brghena @alevy 😃

After feedback I will continue to add wrapper for Eddystone as @brghena suggested and update existing example apps since this will break those.

Documentation Updated

Kernel: The relevant files in /docs have been updated or no updates are required.
Userland: The application README has been added, updated, or no updates are required.

Formatting

make formatall has been run.

brghena

This looks pretty good. The test function is pretty simple and readable for all the functionality it provides. A couple of comments for you.

brghena · 2017-09-24T18:17:21Z

userland/examples/tutorials/xx_ble_adv/main.c

+  return 0;
+}
+
+const char* get_error(int err) {


Actually, this has already been written: https://github.com/helena-project/tock/blob/master/userland/libtock/tock.c#L178

brghena · 2017-09-24T18:18:14Z

userland/examples/tutorials/xx_ble_adv/main.c

+
+  // BLE DATA FIELD
+  uint8_t addr[] = {1, 2, 3, 4, 5, 6};
+  uint16_t advertising_itv_ms = 300;


I would spell out interval here advertising_interval_ms

brghena · 2017-09-24T18:25:18Z

userland/examples/tutorials/xx_ble_adv/main.c

+  err = ble_initialize(advertising_itv_ms,
+                       LE_LIMITED_DISCOVERABLE | BREDR_NOT_SUPPORTED,
+                       addr,
+                       sizeof(addr));


Technically, sizeof on buffers ~~allocated on the local stack~~ (EDIT: see below) does work successfully. However, I'd argue that it's a bad practice to do so. In the general case, sizeof does not work on buffers, and using them this way leads to mistakes (like someone making a buffer global instead of local and code suddenly breaking).

I'd suggest replacing them with explicit lengths.

Hmm, is this explanation really correct? I thought it has nothing to do whether there are stored on stack or in data segment since there are evaluated at compile-time? However, since arrays are automatically converted to pointers upon a function call you can not use sizeof then because you get the word size then. But send a link if I'm wrong this got me curious.

Doing some research shows you to be correct Niklas. sizeof works on any named buffer. So like you say, it fails when used on a buffer is passed through a function call, but works on any local buffer on the stack or global buffer in the same file.

I've always learned it to be a bad practice though, probably because beginning programmers do not notice the difference, and using sizeof on the other side of a function boundary is a common mistake. I could see the argument that it is appropriate here.

brghena · 2017-09-24T18:27:42Z

userland/libtock/ble.h

+ // initialize advertisement (should be used before invoken ble_start_advertising)
+ //
+ // advertising_itv_ms  - advertisement interval in milliseconds
+ // flags               - flags in the advertisement defined in GapFlags_t enum


Why expose flags to the user rather than just selecting the right ones? For instance, users will always have to specify BREDR_NOT_SUPPORTED.

I would suggest replacing this with a is_connectable bool and then have the code automatically select the flags.

Sure it is a better idea

brghena · 2017-09-24T18:32:04Z

userland/libtock/ble.h

-// start send BLE advertisement periodically according to the configured interval
-// if no interval is connected it will use a pre-configured value
+// data                 - array of bytes to write the received advertisment to
+// len                  - size in bytes of data


This is a maximum length, right? I'd rename it to max_len (or something like that).

Also, how does the actual received size get returned?

it won't currently if I remember correct, the capsule only writes the number of bytes it receives and notifies the user-space app.

brghena · 2017-09-24T18:32:24Z

userland/libtock/ble.h

-// if no interval is connected it will use a pre-configured value
+// data                 - array of bytes to write the received advertisment to
+// len                  - size in bytes of data
+// callback             - callback handler to call when an advertisement is received


What's the type signature of the callback handler?

static void callback(__attribute__((unused)) int unused0, __attribute__((unused)) int unused1, __attribute__((unused)) int unused2, __attribute__((unused)) void* ud);

It was like that before at least

I'd suggest documenting in that header file what the parameters in the callback actually mean. If I was going to write code that scanned for advertisements, I wouldn't know what the parameters I get in the callback actually mean without reading through the kernel code.

brghena · 2017-09-24T18:33:37Z

userland/libtock/ble.h

+ //
+ // device_name         - device named to be used in the advertisement
+ // len                 - size in bytes
+int ble_advertise_name(uint8_t *device_name, uint8_t size_b);


For this class of advertisement functions, what are the possible return values? What value is returned if the advertisement data is now too long to advertise?

turns out the "capsule" returned SUCCESS regardless good catch but now it returns ESIZE when the buffer is full.

brghena · 2017-09-24T18:35:01Z

userland/libtock/ble.c

+static int s_ble_cfg_gap_data (GapAdvertisementData_t header, uint8_t *data, uint8_t len);
+
+// local VARIABLES
+static uint8_t s_gap[GAP_DATA_MAX_SIZE];


What does the s stand for?

static, well I named all things that have file scope as s_xxx_yyy but I can remove it if it's annoying

brghena · 2017-09-24T18:40:21Z

userland/libtock/ble.c

+  if (data_len + 2 > GAP_DATA_MAX_SIZE) {
+    return TOCK_FAIL;
+  }else {
+    memset(s_gap, 0, GAP_DATA_MAX_SIZE);


Why not make s_gap as a local buffer on the stack in this function instead of a file variable?

niklasad1 · 2017-09-25T06:56:06Z

Thank you @brghena for excellent feedback.
I have cleaned it up, fixed some spelling errors and added some documentation (inline comments)

alevy · 2017-09-25T19:40:04Z

the ble_adv app seems not to be compiling for some reason.

niklasad1 · 2017-09-25T20:18:41Z

the ble_adv app seems not to be compiling for some reason.

Yes, it will not work because it uses the old API, the only application that work with this is:

https://github.com/niklasad1/tock/blob/simple_ble_interface/userland/examples/tutorials/xx_ble_adv/main.c

I will not modify anything until we have reached consensus 😃

alevy

This is really great! I think it's pretty close to being able to support pretty cleanly and simply the kinds of applications @brghena et al have been doing with the NRF5x anyway, which is pretty cool.

My main concern is thinking about how this interface works when there are multiple processes using it. In general, I think it's simple to just allow each process to configure a separate advertising packet, and interleave those, but the ble_initialize function throws a wrench in that.

alevy · 2017-09-26T13:13:27Z

userland/libtock/ble.h

+// passed back to user-space thus userspace must loop through the entire
+// buffer.
+//
+// TODO: add functionlity in kernel to pass back the size of the received


This seems important to do before merging.

alevy · 2017-09-26T13:17:48Z

userland/libtock/ble.c

+  int err;
+
+  // configure advertisement interval
+  // if the interval is less than 20 or bigger than 10240 to kernel


"...the kernel..."

alevy · 2017-09-26T13:18:21Z

userland/libtock/ble.c

+ *   INTERNAL BLE HELPER FUNCTION Prototypes
+ *
+ *   s_   - static (file scope)
+ ******************************************************************************/


It would be useful to have comments for what these functions do, even though they are internal helper functions.

alevy · 2017-09-26T13:24:55Z

userland/libtock/ble.c

+ *   USER-SPACE BLE API
+ ******************************************************************************/
+
+int ble_initialize(uint16_t advertising_itv_ms, bool discoverable, uint8_t *addr,


I think a good question is whether we want to allow users-pace to set the BLE address, or whether that should be in the kernel (or potentially in a separate driver that eventually is only accessible by privileged processes or something).

Consider the situation where we have two processes sending advertisements. We can interleave different advertising packets from each process with the same address no problem. We can also, in principle, send different advertising packets with different addresses from each process, but it becomes more complicated if they the device is connectable. Specifically, I'm not sure it will be as easy to have the device actually connect on two different addresses (though might be an interesting piece of engineering to attempt).

There are various ways we can imagine breaking ties, for example only actually responding to connection handshakes one address at a time, but it's worth considering what we actually want to do.

Regardless it's probably also worth allowing the process to not choose an address (perhaps by passing NULL and 0 to addr and addr_len, respectively), and then defaulting to either an address a different process has already set, or something reasonable the kernel decides on (e.g. an address based on the hardware MAC address or something).

I think the busy flag in the capsule should at least protect against "races" in changing advertising address for now.

However it may limit concurrency which also is a valid question to consider as well.

I didn't really think about it before, but it's probably a good idea for the kernel to set the BLE address of the device and for the application to have no input on it. However, this isn't how the nRF SDK works, so if we made that change, it would be a difference between serialized and native applications.

The exception would be if we really could virtualize everything about BLE, changing addresses with each advertisement and accepting connections to specific applications, but that sounds pretty tough.

(edit: I posted this somewhere else, but it really belongs here)

Ok, but it sounds like it is best to let kernel decide the address for now then?

Should be pretty easy to use the TRNG driver to generate 6 random bytes if you want that?

But should all functions be completely re-written because it is still possible have interleaving between configuring advertisement_interval, configure different data (device_name, uuids etc) in the packet.

But after _start the busy flag will be set and block the entire thing as it is right now

I think you @alevy and @brghena has to decide this because I don't know how to approach this.

EDIT: well, I guess each process have its own grant but that doesn't matter because both are writing to the buffer which the DMA are reading from.

I think for now having the kernel control the address is best.

From the perspective of this PR, just having passed into the driver as a configuration variable in the board setup seems fine. The point is it's not up to the process. There are several ways we can set the address in the kernel (e.g. there is also a 64-bit unique device identifier in the FICR controller for the NRF52; randomness could work too). But that's for a different PR, it's not about the interface.

As far as other functions in the userland interface are concerned, I don't think they are problematic in the same way. I think at the moment (i.e. given the current state of the driver in the kernel) interleaving won't work as we expect, but we know how to make it work. In particular, you'll just store the data for each process's configured advertising packet in a grant and, in the kernel, switch between which one you're sending to interleave them. So, for example, if I have two processes, one with a 50ms advertising interval, and another with a 100ms advertising interval, my actual on-air packets might look something like this:

Time Packet ==== ====== 0 ---> "process 1 packet" 25 ms ---> "process 2 packet" 50 ms ---> "process 1 packet" 75 ms ---> no_data 100 ms ---> "process 1 packet" 125 ms ---> "process 2 packet" 150 ms ---> "process 1 packet"

Again, it's fine if the kernel driver doesn't do that just yet, as long as the interface supports it.

As for scanning, I think basically all incoming scanned packets should be delivered to all processes that have subscribed to scans. In the future we could maybe add functions that allow processes to configure filters for which packets they actually want to receive (and I can imagine implementing this in a combination of hardware and software in the kernel).

This is definitely what I was thinking too Amit.

alevy · 2017-09-26T13:26:02Z

userland/libtock/ble.c

+  err = s_ble_cfg_flags(flags);
+  if (err < TOCK_SUCCESS) return err;
+
+  // if this is NULL advertisment address {0, 0, 0, 0, 0, 0}  will be used


Does {0, 0, 0, 0, 0, 0} have a particular meaning? If so, processes should be explicit rather than using NULL. If not, than maybe this should promise less---i.e. it will use some preconfigured address either from another process or from the kernel.

No, it means that kernel will use preconfigured adress which is better.

I will change.

alevy · 2017-09-26T13:27:51Z

userland/libtock/ble.h

+int ble_stop_advertising(void);
+
+// reset the entire advertisement data packet
+// excluding flags and advertisement address


Does this also implicitly stop advertising?

No, but it perhaps should?

alevy · 2017-10-03T15:00:51Z

@niklasad1 how do you feel about the comments? Do you think that all the feedback you need or are there other open questions about the interface?

niklasad1 · 2017-10-03T18:09:56Z

Yeah, I think it is only to remove the configure advertisement option.

But in there is more to do as follows:

Progess

Update existing example apps (this PR)
Add option in the kernel to pass back how many bytes have been read by the Radio in passive advertisement scanning (fix in different PR)
Refactor ble_advertising_driver to enable more concurrency and avoid race-conditions (fix in different PR)
Add eddystone wrappers (another PR)
Add randomization to advertisement address (another PR)

Thoughts?

alevy · 2017-10-03T20:49:28Z

I think that division of work for this PR vs future PRs is right.

niklasad1 · 2017-10-06T15:47:11Z

Is this good enough for merge?

I have removed [tutorials/xx_ble_adv] and changed the existing "bluetooth low energy example apps" to work with the new interface but I don't want to start with the subtasks until this is merged!

alevy · 2017-10-06T21:20:50Z

userland/libtock/simple_ble.h

+// TODO: add functionlity in kernel to pass back the size of the received
+// advertisement
+//
+int ble_start_passive_scan_wip(uint8_t *data, uint8_t len, subscribe_cb callback);


I don't think we need to postfix the name with _wip

alevy · 2017-10-06T21:22:05Z

There is a superfluous userland/examples/ble_advertising/.main.c.swp that needs to be deleted. Otherwise looks good to me

some fixes according to feedback remove option to configure address in advertisement migrate to Tock 1.0 format simple ble interface

niklasad1 added blocked Waiting on something, like a different PR or a dependency. help wanted labels Sep 24, 2017

niklasad1 requested review from alevy and brghena September 24, 2017 18:01

brghena reviewed Sep 24, 2017

View reviewed changes

alevy requested changes Sep 26, 2017

View reviewed changes

niklasad1 removed the blocked Waiting on something, like a different PR or a dependency. label Oct 3, 2017

alevy reviewed Oct 6, 2017

View reviewed changes

simple ble interface

ca02604

some fixes according to feedback remove option to configure address in advertisement migrate to Tock 1.0 format simple ble interface

niklasad1 removed the help wanted label Oct 7, 2017

niklasad1 mentioned this pull request Oct 14, 2017

Corrode nrf51 #649

Merged

3 tasks

alevy approved these changes Oct 16, 2017

View reviewed changes

alevy merged commit 0b823dc into tock:master Oct 16, 2017

alevy deleted the simple_ble_interface branch October 16, 2017 20:34

simple ble interface #623

simple ble interface #623

Conversation

niklasad1 commented Sep 24, 2017

Pull Request Overview

Testing Strategy

TODO or Help Wanted

Documentation Updated

Formatting

brghena left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brghena Sep 24, 2017 • edited

Choose a reason for hiding this comment

niklasad1 Sep 24, 2017 • edited

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

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

niklasad1 Sep 24, 2017 • edited

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

niklasad1 commented Sep 25, 2017 • edited

alevy commented Sep 25, 2017

niklasad1 commented Sep 25, 2017

alevy left a 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

niklasad1 Sep 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Sep 26, 2017 • edited

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

Choose a reason for hiding this comment

alevy commented Oct 3, 2017

niklasad1 commented Oct 3, 2017 • edited

Progess

alevy commented Oct 3, 2017

niklasad1 commented Oct 6, 2017

Choose a reason for hiding this comment

alevy commented Oct 6, 2017 • edited

brghena Sep 24, 2017 •

edited

niklasad1 Sep 24, 2017 •

edited

niklasad1 Sep 24, 2017 •

edited

niklasad1 commented Sep 25, 2017 •

edited

niklasad1 Sep 26, 2017 •

edited

niklasad1 Sep 26, 2017 •

edited

niklasad1 commented Oct 3, 2017 •

edited

alevy commented Oct 6, 2017 •

edited