embassy-rs / nrf-softdevice

Apache License 2.0
269 stars 80 forks source link

De-Obfuscate Advertisement Data with Advertisement Builder #214

Closed AdinAck closed 11 months ago

AdinAck commented 11 months ago

Motivation

This is how adv/scan data is currently configured:

#[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'u', b's', b't',
];
#[rustfmt::skip]
let scan_data = &[
    0x03, 0x03, 0x09, 0x18,
];

You don't often get to deal with raw bytes in a top level interface!

Proposal

let adv_data = StandardAdvertisementData::new()
    .flags([Flag::GeneralDiscovery, Flag::LE_Only])
    .services(Complete16([BasicService::HealthThermometer]))
    .name(ShortName("HelloRust"));

let scan_data = StandardAdvertisementData::new().name(FullName("Hello, Rust!"));

let adv = peripheral::NonconnectableAdvertisement::ScannableUndirected {
    adv_data: unwrap!(adv_data.as_slice()),
    scan_data: unwrap!(scan_data.as_slice()),
};

With this PR, advertisement/scan data can be generated with a builder rather than raw bytes.

Changes

ble

examples

Some examples have been updated to use the new builder.

Features

Future

Examples

Multiple 16 bit Services

let adv_data = StandardAdvertisementData::new()
    .flags([Flag::GeneralDiscovery, Flag::LE_Only])
    .services(Complete16([BasicService::HealthThermometer, BasicService::GamingAudio]))
    .name(FullName("Full Name"));

Arbitrary 128bit service

// the uuid crate can be used
let test_service = CustomService(uuid!("67e55044-10b1-426f-9247-bb680e5fe0c8").as_bytes().clone());
// or raw bytes can be provided
let test_service = CustomService([0x67, 0xe5, 0x50, 0x44, 0x10, 0xb1, 0x42, 0x6f, 0x92, 0x47, 0xbb, 0x68, 0x0e, 0x5f, 0xe0, 0xc8]);

let adv_data = StandardAdvertisementData::new()
    .flags([Flag::GeneralDiscovery, Flag::LE_Only])
    .services(Complete128([test_service]))
    .name(ShortName("ShrtNm"));

Raw Bytes

let adv_data = StandardAdvertisementData::new()
    .flags([Flag::GeneralDiscovery, Flag::LE_Only])
    .raw(ADType::URI, &[0xde, 0xad, 0xbe, 0xef])
    .name(ShortName("ShrtNm"));

Adaptable Name

let adv_data = StandardAdvertisementData::new()
    .flags([Flag::GeneralDiscovery, Flag::LE_Only])
    .raw(ADType::URI, &[0xde, 0xad, 0xbe, 0xef])
    .adapt_name(FullName("Full Name"), ShortName("ShrtNm"));

Extended Data

let first_service = CustomService(uuid!("67e55044-10b1-426f-9247-bb680e5fe0c8").as_bytes().clone());
let second_service = CustomService(uuid!("c8381a2f-fe22-4d2c-8552-f46b31da7005").as_bytes().clone());

let adv_data = ExtendedAdvertisementData::new()
    .flags([Flag::GeneralDiscovery, Flag::LE_Only])
    .services(Complete16([BasicService::HealthThermometer, BasicService::GamingAudio]))
    .services(Complete128([first_service, second_service]))
    .name(FullName("Full Name"));

Notes

Some slight adjustments other than what I listed in the Future section may be needed, but I believe this pretty much covers all the bases and will be very useful.

AdinAck commented 11 months ago

I just realized I need to check if the built advertisement data exceeds the maximum allowed length... I'll implement that in a bit.

AdinAck commented 11 months ago

Done. Also realized this repo is not really set up for unit tests so I removed them.

alexmoon commented 11 months ago

This looks pretty good to me. I prefer it to the macro version, though it does have some runtime cost. One issue I see is that hard-coding ADV_LEN means it doesn't support extended advertising packets (which can be up to 254 bytes) and also means that you can't use a smaller buffer if your advertising packet is less than 31 bytes. I wonder if you could have the builder take in the buffer to use as a parameter or use a const generic for the packet length.

alexmoon commented 11 months ago

Another feature that would be nice to have is a name function that fills the remaining space in the buffer with as much of the name that fits. If the whole name fits it gives it type FullName, otherwise ShortName.

AdinAck commented 11 months ago

...though it does have some runtime cost. ...it doesn't support extended advertising packets (which can be up to 254 bytes)

Both of these are listed as future changes.

I wonder if you could have the builder take in the buffer to use as a parameter or use a const generic for the packet length.

Yeah I've been thinking about this... I can't think of a good way to utilize generics and still allow the order to be controlled as you have made it clear you want.

Passing in the buffer is sketchy since this is at runtime, how would the user decide the length each time they change the advertisement data? I decided a small burst of some memory usage is better than an ambiguity related panic.

AdinAck commented 11 months ago

Another feature that would be nice to have is a name function that fills the remaining space in the buffer with as much of the name that fits. If the whole name fits it gives it type FullName, otherwise ShortName.

Great idea, I'll get right on this.

alexmoon commented 11 months ago

I wonder if you could have the builder take in the buffer to use as a parameter or use a const generic for the packet length.

Yeah I've been thinking about this... I can't think of a good way to utilize generics and still allow the order to be controlled as you have made it clear you want.

Passing in the buffer is sketchy since this is at runtime, how would the user decide the length each time they change the advertisement data? I decided a small burst of some memory usage is better than an ambiguity related panic.

Const generic version could look like this (and just use N in place of ADV_LEN everywhere):

pub struct AdvertisementData<const N = 31> {
    buf: [u8; N],
    ptr: usize,
}

The buffer version would be something like (using self.buf.len() in place of ADV_LEN):

pub struct AdvertisementData<'a> {
    buf: &'a mut [u8],
    ptr: usize,
}

impl<'a> AdvertisementData<'a> {
    pub fn new(buf: &'a mut [u8]) -> Self {
        Self {
            buf,
            ptr: 0,
        }
    }

    ...
}
AdinAck commented 11 months ago

When you said const generics I thought you were referring to the builder supporting constant evaluation (so it could be a const). What I am working on now is something like AdvertisementData<Standard> or AdvertisementData<Extended>. Passing in a buffer would make it so that the user would have to compute all of the bytes themselves to know the length, defeating the purpose of the builder.