Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Starting the Bluetooth Low Energy design doc #838

Merged
merged 1 commit into from Jul 18, 2018
Merged

Starting the Bluetooth Low Energy design doc #838

merged 1 commit into from Jul 18, 2018

Conversation

alevy
Copy link
Member

@alevy alevy commented Mar 23, 2018

Pull Request Overview

This is a work in progress. I'm posting this now to start soliciting feedback, particularly from contributors who are currently implementing various parts of the stack.

This adds a design document for the Bluetooth Low Energy subsystem. It includes high level design for the system call interface for advertising, scanning and connection-oriented communication, as well as a HIL for on-chip Bluetooth Low Energy radio hardware.

Testing Strategy

This is just a design doc, so there is nothing to test

TODO or Help Wanted

@niklasad1 @cpluss @Antil00p @lindskogen: Your feedback is requested?

  • I didn't really write up any layering, but we should probably do that. Any thoughts on how to slice up the stack? The only part I did put in is that basically processes will be responsible for everything above the Bluetooth HCI protocol. That includes at least GAP and advertising payload semantics. I'm not sure if it includes l2cap or just ATT/GATT (haven't looked closely at L2CAP yet).

  • Do we need the HIL to provide higher level operations, for example in order for the system call driver to meet timing constraints for scan responses? What about when we implement connection-oriented communication?

Of course we'll still be able to iterate on this interface even after merging this PR, but would be good to think about what we'll actually need out of the radio-specific implementations.

Documentation Updated

  • Kernel: Updated the relevant files in /docs, or no updates are required.
  • Userland: Added/updated the application README, if needed.

Formatting

  • Ran make formatall.

Rendered

@alevy alevy added help wanted blocked Waiting on something, like a different PR or a dependency. documentation kernel P-Significant This is a substancial change that requires review from all core developers. labels Mar 23, 2018
@niklasad1 niklasad1 added the Work in Progress PR that is still being updated, feedback is likely helpful. label Mar 24, 2018
@niklasad1
Copy link
Member

@alevy great

I would like to add two additional lower level layers:

  • pub trait LinkLayer
  • pub trait PhysicalLayer

I would like to see the timing critical things encapsulated in these two layers but I'm not developing anything at the moment so it's up the other guys the report what is reasonable/plausible or not

@cpluss
Copy link
Contributor

cpluss commented Mar 27, 2018

Not all radios are connected in such a way which makes it possible to send all types of packets in the same way (eg. the cc26xx radio have a different way to send each type of packet, see p.1658 in the cc2650 reference manual).

I'm thinking something along the lines of

enum BlePacketType {
     Transmit,
     AdvertiseNonConnectableDirected,
     ...
}
struct BlePacket<'a> {
    data: &'a [u8],
    type: BlePacketType,
    ... // additional data
}

...

    /// Transmits a packet over the radio
    ///
    /// Returns ReturnCode::EBUSY if the radio is currently transmiting or
    /// recieving, otherwise ReturnCode::Success.
    fn transmit_packet(
        &self,
        packet: BlePacket,
     ) -> ReturnCode;

To be able to determine the type in the hardware implementation instead of decoding the packet payload. This would also allow utilisation of any packet-specific features for certain hardware :)

@niklasad1
Copy link
Member

niklasad1 commented Mar 27, 2018

@cpluss I get your concern but the information is still in the data_payload which is specified by the BLE spec and thus chip independent. The format looks as the following for ADV_IND

+----------+--------+-----------+
|  Header  |  AdvA  |  AdvData  |
+----------+--------+-----------+

So, in my opinion if you need the Advertising PDU Type you have to parse the 4 LSB in the buffer because this is the format that data should have over the air

But for more clarity using structs over raw slices is a good idea for the different PDUs but that depends how it will be used from userland vs kernel-mode. There is an open RFC regarding constructing the entire Advertising PDU from userland for example #716

It could like something like:

// this is not valid Rust syntax
struct Header {
 adv_type: 4 bits
 rfu: 2 bits,
 tx_add: 1 bit,
 rx_add: 1 bit,
 length: 6 bits,
 rfu: 2 bits,
}

struct ConnectUndirected {
  header: Header,
  adv_a: [u8; 6],
  adv_data: [u8; 32],
}

Finally, advertising == transmitting

@cpluss
Copy link
Contributor

cpluss commented Mar 28, 2018

@niklasad1 imo it's really ugly to extract parts of the payload from a byte array (like this); it'd be nice to be able to decompose it through some shared structure.

The way you interact with the BLE module on the TI cc26xx family MCUs enforces you to decompose the payload, even if it was constructed in userland. All interactions occur through shared memory using pre-defined command structures (differ between each command) - and advertisements has one command structure per kind (ConnectedUndirected etc).

I don't have that much knowledge about how BLE actually works, in my mind I've got the impression that transmissions and advertisements are two different things - since they run on different channels?

@niklasad1
Copy link
Member

Aha, are you are referring to Data Channel PDU when saying Transmitting?

In general the different PDUs are transmitting (Advertising, Scanning and Data Channel).

Sorry didn't mean to be rude it's a very complicated protocol I can't claim that I understand it fully!

@niklasad1
Copy link
Member

RE:

I don't see how your proposed interface would mitigate these ugly extract parts but de-serialize the buffer into a struct as I proposed could maybe do it?

However, to use these different structs the AdvType has to be provided from userspace as an additional parameter otherwise we have to parse the buffer in capsule which is probably not a good idea. I think that along with different structures is the best we can do. Then the question is how pass the different structs for example via Trait objects or something similar!

But _still_, we can't make any assumptions how these are configured in different chips. Ideally, it would also be nice to avoid to copy the actual advertisement buffer into static mut buffers and just configure the DMA to point directly to struct/buffer allocated by each application (process)

@cpluss
Copy link
Contributor

cpluss commented Mar 28, 2018

@niklasad1 using structs was what I meant in the beginning, sorry for being unclear :) The structs I provided was meant to reflect that, and was not meant as an actual suggestion of how we should do it - but rather an example of how (ie. using structs instead of a plain byte array in the HIL). Your proposed solution is way better than my example!

Ideally I'd like the HIL to provide a flexibility in such a way that it's possible to handle all PDU types separately, as well as the embedded data in them (device addr, etc). I have no idea what the best way to achieve this would be.

And yes, I'm referring to the Data Channel PDU when I say transmitting :)

@niklasad1
Copy link
Member

niklasad1 commented Mar 31, 2018

@cpluss 👍

I been thinking a bit on this for a bit and came with a suggestion that the BLE Capsule in collaboration with userland constructs a final advertising buffer depending on what AdvertisingPdu that we can just convert that into a struct

  • AdvertisingConnectUndirected (ADV_IND)
  • AdvertisingConnectDirected (ADV_DIRECT_IND)
  • AdvertisingNonConnectUndirected (ADV_NONCONN_IND)
  • AdvertisingScanUndirected (ADV_SCAN_IND)

And then pass it via the HIL to chips, I don't know what the kind of checks are needed here maybe check AdvertisingPdu in the buffer and buffer length or just trust that the userland and capsule is correct?!

I have illustrated with some code but only listed in one of the four AdvertisingPdu's but the idea is to convert a slice into to a ÀdvertisingPdu via the From trait. Then this should go on the stack and then chips have to copy it into something else with static lifetime.

A slice has the size of two words and the header 2 bytes so this should require:
2 * word_size (4 I guess) + 2 bytes + additional padding

This should be okay I think

#[macro_use]
extern crate bitfield;

#[allow(unused)]
#[repr(u8)]
enum BLEAdvertisementType {
    ConnectUndirected = 0x00,
    ConnectDirected = 0x01,
    NonConnectUndirected = 0x02,
    ScanUndirected = 0x06,
}

/// BLUETOOTH SPECIFICATION Version 4.2 [Vol 6, Part B]
/// 2.3 Advertising Channel PDU
bitfield!{
    #[derive(Copy, Clone)]
    pub struct Header(u16);
    impl Debug;
    /// pdu 4 bits
    pub pdu,        set_pdu:    3, 0;
    /// rfu 2 bits
    pub _rfu1,      _:          5, 4;
    /// txAdd 1 bit
    pub tx_add,     set_tx_add: 6, 6;
    /// rxAdd 1 bit
    pub rx_add,     set_rx_add: 7, 7;
    /// length 6 bits
    pub length,     set_length: 13, 8;
    /// rfu 2 bits
    pub _rfu2,      _:          15, 14;
}

#[derive(Debug)]
pub struct AdvertisingConnectUndirected {
    header: Header,
    adv_a: &'static [u8],
    adv_data: &'static [u8],
}

impl From<&'static mut [u8]> for AdvertisingConnectUndirected {
    fn from(buf: &'static mut [u8]) -> Self {
        assert!(buf.len() == 39);
        let header = Header((buf[0]) as u16 | (buf[1] as u16) << 8);

        AdvertisingConnectUndirected {
            header: header,
            adv_a: &buf[2..],
            adv_data: &buf[8..],
        }
    }
}

static mut BUF: [u8; 39] = [2, 23, 1, 2, 3, 4, 5, 6, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0];

fn main() {
    unsafe {
        let packet = AdvertisingConnectUndirected::from(BUF.as_mut());
        println!("{:?}", packet);
    }
}

Thoughts?

&self,
buf: &'static mut [u8],
disable: bool,
len: usize) -> ReturnCode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove len I think buf.len() is sufficent here!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be wrong, but I think len is required if it's a statically sized buffer (eg. [64; 0] would yield buf.len() = 64)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this is a slice not an array

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, I just assumed that slices were arrays

///
/// Returns ReturnCode::EBUSY if the radio is currently transmiting or
/// recieving, otherwise ReturnCode::Success.
fn receive_packet(&self, buf: &'static mut [u8]) -> ReturnCode;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is good with buf here but it requires changes to userspace if we want to advertise and scan concurrently. Also with this I think we can remove the static buffers in chips then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also how do you propose to the replace TakeCell in the BLE Capsule?

///
/// Returns ReturnCode::EBUSY if the radio is currently transmiting or
/// recieving, otherwise ReturnCode::Success.
fn set_channel(&self, channel: RadioChannel) -> ReturnCode;
Copy link
Member

@niklasad1 niklasad1 Mar 31, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give an example how this intended to work?

I would rather see the following sequence:

  1. set_busy()
  2. start_advertise(buf, channel37).
  3. continue_advertise(channel38)
  4. continue_advertise(channel39)
  5. set_not_busy()

This would passing/copying a advertisement_buffer everytime we should receive or transmit advertisements when in practice we only need to change channel!

But, if a "final buffer" is passed that the DMA only need to be pointed at then it is fine to me to skip continue_advertise

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply advertise(buf) which would perform advertisements on all three channels?

Copy link
Member

@niklasad1 niklasad1 Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For efficiency reasons, no reason to copy the buffer when we have mutual exclusion if we are switching channel from 37 -> 38 or 38 -> 39 but could probably solved be internally in the method otherwise. But, currently it's like you proposed :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the sequence could be more verbose in that case :)

  1. set_busy(true)
  2. set_buffer(buf)
  3. advertise_on(37)
  4. advertise_on(38)
  5. advertise_on(39)
  6. set_busy(false)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, that is way cleaner

* Connect request

Scan response are sent automatically if a scan response payload is configured.
Scan rerequest and connection requests are handled by other parts of the sytsem
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/rerequest/request

@cpluss
Copy link
Contributor

cpluss commented Apr 4, 2018

@niklasad1 that looks nice :) How would you be able to determine what struct to decode it into, reading the PDU type with a switch-case?

I think some form of validation of the data would be required since they are composed by untrusted(?) code in the userland

@niklasad1
Copy link
Member

niklasad1 commented Apr 4, 2018

@cpluss Basically checking that the Advertising Type is valid and the buffer has the correct length (39 or 12) to prevent any buffer overruns etc. But, if bogus data passed from userspace it will just send invalid BLE packets.

But, the dummy serialization will be fooled! But, this is okay for me at least!

Thoughts?

OT:

  • ADV_IND, ADV_NONCONN_IND and ADV_NONCONN_IND - 39 bytes
  • ADV_DIRECT_IND - 12 bytes

@niklasad1
Copy link
Member

niklasad1 commented Apr 15, 2018

BTW, I implemented a PoC for this you can checkout https://github.com/helena-project/tock/tree/ble/dummy-serialization

Some notes:

  • The advertisement buffer is completely managed by user-space and the kernel only generates a random advertisement address
  • It is hard-coded to ADV_NON_CONN_IND in libtock atm but should be trivial to add the remaining structs
  • A dummy trait Advertising to enable generics in the Advertising HIL
  • TryFrom to convert a buffer to a struct

Still very very WIP

Thoughts?

@alevy alevy added this to Non-critical in Tock Rolling Release Apr 23, 2018
@alevy alevy moved this from Non-critical to 1.2 Goals in Tock Rolling Release Apr 25, 2018
@alevy
Copy link
Member Author

alevy commented Apr 26, 2018

Stepping back a bit, just at a high level, how is this breakdown of responsibilities?

The goal is to make layers swappable---so i can replace lower layers for different hardware setups and intermediate layers if I want to, e.g. implement a non-BLE protocol on top of the BLE phy, different kinds of system call drivers, or different virtualization layers for l2cap/advertising/etc.

Note that it's perfectly fine for a single implementation/module to implement multiple layers (e.g. it might be relatively easy for the NRF52 radio driver to just implement both the physical and link-layer trait).

PHY: Hardware-specific, on-chip radio driver (e.g. NRF52)

Expected to be able to transmit arbitrary data over all BLE channels and implement very low-latency timing operations (if necessary, I think that's still tbd?), such as maybe enforcing interframe spacing.

It is agnostic to what is sent over those channels: it doesn't care if you send data that looks like advertisements over the data channels or if you send non-BLE data at all. This could just as well be used to experiment with different protocols using the same PHY.

Configuration operations like changing the channel are expected to be synchronous (i.e. writing to a register rather than issuing a command over SPI/shared memory) and fast. Only actual transmission of data would be asynchronous.

Link-layer: Hardware agnostic, could be a peripheral chip (e.g. CC26xx)

This layer differentiates between advertising PDUs, data channel PDUs and subtypes for each (e.g. ADV_IND vs ADV_NONCONN). It would also manage advertising and connection events (but not the timing between advertising events or channel hopping and intervals for data channel connections). Any interactions that need to happen within an event (respond within T_IFS basically) would be implemented here: issue-ing or responding to scan requests, establishing connections, transfering multiple frames in a connection event, etc.

The link-layer, as a result, doesn't do any virtualization and, in fact, the point of it not managing the advertising interval or connection interval and channel hopping is to allow various uses: virtualizing and advertising, implementing a BLE connection master (which needs to multiplex channel hopping and timing for different connections), etc.

"Application" layer: (Usually) Hardware agnostic, implements connections, advertising intervals, virtualization, etc...

One example of this is basically what the proposed advertising system call driver would be, but would also include an implementation of L2CAP channels, a BLE master, etc. This is also where, potentially, a kernel-only implementation (e.g. for a console over BLE or application updates) would be implemented.

In some cases, e.g. for the nrf51 soft-device with serialization that's on Hail and imix, or a peripheral that exposes the bluetooth HCI protocol over UART, an implementation of this layer may not even use the link-layer/phy-layer HILs.

@bradjc
Copy link
Contributor

bradjc commented Jun 11, 2018

Well...as the name says it is a start. Should we merge this?

ppannuto
ppannuto previously approved these changes Jun 27, 2018
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this document is a pretty good start. It doesn't look like there are other immediate comments / changes needed, so I'm inclined to merge this in the next day or two rather than leave it languishing in PR forever.

Folks who are more involved in the BLE stuff than me, holler if you think we should keep holding off.

@ppannuto ppannuto added the last-call Final review period for a pull request. label Jun 27, 2018
/// receiving, otherwise ReturnCode::Success.
fn receive_packet(&self, buf: &'static mut [u8]) -> ReturnCode;

// FIXME: explain this
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alevy can you add some explaination of this?

fn abort_tx(&self) -> Option<&'static mut [u8]>;

// FIXME: explain this
fn abort_rx(&self) -> Option<&'static mut [u8]>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alevy can you add some explaination of this?

niklasad1
niklasad1 previously approved these changes Jun 28, 2018
@niklasad1
Copy link
Member

ping @alevy !

@bradjc bradjc moved this from 1.3 Goals to Critical in Tock Rolling Release Jul 9, 2018
@ppannuto
Copy link
Member

ppannuto commented Jul 18, 2018

bors delegate+

@alevy you normally can't r+ your own PRs, delegate lets you

@bors
Copy link
Contributor

bors bot commented Jul 18, 2018

✌️ alevy can now approve this pull request

@ppannuto ppannuto removed the blocked Waiting on something, like a different PR or a dependency. label Jul 18, 2018
@alevy
Copy link
Member Author

alevy commented Jul 18, 2018

bors r+

bors bot added a commit that referenced this pull request Jul 18, 2018
838: Starting the Bluetooth Low Energy design doc r=alevy a=alevy

### Pull Request Overview

_This is a work in progress. I'm posting this now to start soliciting feedback, particularly from contributors who are currently implementing various parts of the stack._

This adds a design document for the Bluetooth Low Energy subsystem. It includes high level design for the system call interface for advertising, scanning and connection-oriented communication, as well as a HIL for on-chip Bluetooth Low Energy radio hardware.

### Testing Strategy

This is just a design doc, so there is nothing to test

### TODO or Help Wanted

@niklasad1 @cpluss @Antil00p @lindskogen: Your feedback is requested?

  * I didn't really write up any layering, but we should probably do that. Any thoughts on how to slice up the stack? The only part I _did_ put in is that basically processes will be responsible for everything above the Bluetooth HCI protocol. That includes at least GAP and advertising payload semantics. I'm not sure if it includes l2cap or just ATT/GATT (haven't looked closely at L2CAP yet).

  * Do we need the HIL to provide higher level operations, for example in order for the system call driver to meet timing constraints for scan responses? What about when we implement connection-oriented communication?

Of course we'll still be able to iterate on this interface even after merging this PR, but would be good to think about what we'll actually need out of the radio-specific implementations.

### Documentation Updated

- [x] Kernel: Updated the relevant files in `/docs`, or no updates are required.
- [x] Userland: Added/updated the application README, if needed.

### Formatting

- [x] Ran `make formatall`.

-----

[Rendered](https://github.com/helena-project/tock/blob/ble-design-doc/doc/BluetoothLEStack.md)


Co-authored-by: Amit Levy <aalevy@gmail.com>
@bors
Copy link
Contributor

bors bot commented Jul 18, 2018

Build succeeded

@bors bors bot merged commit 955b8bf into master Jul 18, 2018
Tock Rolling Release automation moved this from Critical to Done Jul 18, 2018
@alevy alevy deleted the ble-design-doc branch July 18, 2018 00:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation help wanted kernel last-call Final review period for a pull request. P-Significant This is a substancial change that requires review from all core developers. Work in Progress PR that is still being updated, feedback is likely helpful.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants