embassy-rs / nrf-softdevice

Apache License 2.0
258 stars 79 forks source link

API to build advertising data #25

Open kuon opened 4 years ago

kuon commented 4 years ago

Right now, to build data packets, we have to do something like:

    #[rustfmt::skip]
    let adv_data = &[
        0x02, 0x01, raw::BLE_GAP_ADV_FLAGS_LE_ONLY_GENERAL_DISC_MODE as u8,
        0x03, 0x03, 0x09, 0x18,
        0x0a, 0x09, b'H', b'e', b'l', b'l', b'o', b'R', b'T', b'I', b'C',
    ];
    #[rustfmt::skip]
    let scan_data = &[
        0x03, 0x03, 0x09, 0x18,
    ];

We need an API to build those array without having to fiddle with bytes.

I'd like to help, but I could not find the reference documentation for those data packet.

I guessed that adv_data is an array of length(1byte), type(1byte), data(length - 1 bytes). But I am not very familiar with BLE and found dozen of different documentation with no real reference for this.

kuon commented 4 years ago

I'm trying to gather as much reference as possible:

kuon commented 3 years ago

I'd like to tackle this issue, but I need to know a few things:

Dirbaio commented 3 years ago

A good starting point would be what the Nordic SDK supports:

https://github.com/akiles/nrf5_sdk/blob/master/components/ble/common/ble_advdata.h#L146 https://github.com/akiles/nrf5_sdk/blob/master/components/ble/common/ble_advdata.c#L509

Also, Rubble already has support for this in Rust

https://jonas-schievink.github.io/rubble/rubble/link/ad_structure/enum.AdStructure.html

For builder pattern vs struct: I'd prefer a plain struct that you then encode to bytes (like rubble and the Nordic sdk). There are not that many fields to justify a builder IMO. Also using a builder opens interesting questions, for example: what if you set the UUID list 2 times? Does it use the last one? Do they get concatenated? If it's a regular Rust struct you don't have these issues.

About GATT services: they're a separate concern from advertising data. In advertising data you supply a list of UUIDs that may or may not match the list of GATT services you support after the connection is established.

kuon commented 3 years ago

Yeah I guess a builder isn't justified. Especially if we don't use builders elsewhere (like for config). We have to be consistent.

I think this library is low level enough to use struct/enum directly.

I will use nordic SDK as starting point and use a rubble like implementation.

kuon commented 3 years ago

I started working on it, and I came up with that:

pub enum DeviceName<'a> {
    Short(&'a str),
    Long(&'a str),
}

// maybe implement something like rubble name()
pub struct CompanyId(u16);

pub struct ManufacturerData<'a> {
    company_identifier: CompanyId,
    data: &'a [u8],
}

pub struct RawData<'a> (u8, &'a [u8]);

pub struct ServiceData<'a> (u16, &'a [u8]);

pub struct Flags(u8);

pub enum Uuid {
    U16(u16),
    U32(u32),
    U128([u8;4]),
}

pub struct AdvertData<'a> {
    flags: Flags,
    name: Option<&'a DeviceName<'a>>,
    service: Option<&'a [ServiceData<'a>]>,
    manufacturer: Option<&'a ManufacturerData<'a>>,
    uuids_more_available: Option<&'a [Uuid]>,
    uuids_complete: Option<&'a [Uuid]>,
    uuids_solicited: Option<&'a [Uuid]>,
    raw: Option<&'a [RawData<'a>]>
}

pub struct ScanData<'a> {
    name: Option<&'a DeviceName<'a>>,
    service: Option<&'a [ServiceData<'a>]>,
    manufacturer: Option<&'a ManufacturerData<'a>>,
    uuids_more_available: Option<&'a [Uuid]>,
    uuids_complete: Option<&'a [Uuid]>,
    uuids_solicited: Option<&'a [Uuid]>,
    raw: Option<&'a [RawData<'a>]>
}

Rubble uses a list of enums, but the problem with the list of enum is that we can specify the same thing multiple times, so the API is not fully safe. Also, scan response data cannot include flags, that's why I duplicated the structure. But we could also imagine passing a is_scan_data bool on the to_bytes method and specify that to get either advert or scan data from the struct.

I am also wondering if we should include something like rubble bitflags for flags.

The above structure is the closest I could come to represent the underlying data the best, they are a bit verbose, but with Default it should be ok. They are also unfinished, I need to include appearance and a few other things.

Also, rubble has a list of appearances in the gatt, which can be included in the advert data, do you think we should include a list like that?

Finally, in the nordic SDK, to include appearance (or name), you only pass a bool and the SDK gets the appearance (or name) from gap data, but I think it's better to have a complete data struct and not rely on existing gap data.

kuon commented 3 years ago

I'd like to tackle this issue, @Dirbaio what do you think?

derekdreery commented 3 years ago

@Dirbaio what are you thoughts on overhead of helpers to construct packets? I'm not sure that they would end up as zero-cost abstractions. If not, they could be compile-time macros. Or when you're able to create &'static [u8] in const fns that would be the best solution.

Dirbaio commented 3 years ago

Doing it at runtime is probably fine, that's what the C nrf5-sdk does. Also it makes it possible to make stuff like device names dynamic which is nice.

Getting it to work wth today's const fn limitations will be quite hard though, I'd just focus on runtime building.

It can be "translate to/from struct" like the one above, or a "builder" where you add data and it writes it to a buffer as you go. I have the suspicion the second will be better for code size.