search

embassy-rs / nrf-softdevice

Apache License 2.0
256 stars 76 forks source link

Add Proc Macro for Generating Adv/Scan Data #193

Closed AdinAck closed 6 months ago

AdinAck commented 10 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 = generate_adv_data! {
    flags: (LE_Only, GeneralDiscovery),
    services: Complete16(HealthThermometer),
    short_name: "HelloRust"
};

// expands to: &[2u8, 1u8, 2u8, 3u8, 3u8, 9u8, 24u8, 10u8, 9u8, 72u8, 101u8, 108u8, 108u8, 111u8, 82u8, 117u8, 115u8, 116u8];

let scan_data = generate_adv_data! {
    services: Complete16(HealthThermometer)
};

// expands to: &[3u8, 3u8, 9u8, 24u8];

With this PR, these code snippets are equivalent at runtime, as the array is generated by the proc macro at compile time.

Changes

Cargo

There are two additional dependencies: strum and strum-macros.

nrf-softdevice-macro

In lib.rs there is one new proc macro: generate_adv_data, along with the associated structures made for its implementation.

examples

All applicable examples have been updated to use the new macro instead of the raw array.

Features

Future

Examples

Multiple 16bit Services

const ADV_DATA: &[u8] = generate_adv_data! {
    flags: (LE_Only, GeneralDiscovery),
    services: Complete16(HealthThermometer, GamingAudio),
    short_name: "HelloRust"
};

Arbitrary 128bit Service

const ADV_DATA: &[u8] = generate_adv_data! {
    flags: (LE_Only, GeneralDiscovery),
    services: Complete128(Custom("e2b095d1-1b08-4e64-a942-842a7fa806bf")),
    short_name: "HelloRst" // max length with legacy (31 bytes)
};

const SCAN_DATA: &[u8] = generate_adv_data! {
    full_name: "Hello, Rust!" // but we can make up for it here
};

Notes

I tried my best to decouple the structures as best as I could, there is a little bit of coupling left but I'd say it's a fine start

alexmoon commented 10 months ago

Does there really need to be separate macros for adv_data and scan_data? It's the same format for both. Also, I don't think the macro should do length validation. Legacy advertising and scan packets are limited to 31 bytes, but extended advertising packets can be up to 254 bytes long.

AdinAck commented 10 months ago

Does there really need to be separate macros for adv_data and scan_data?

The only difference is the error message when it exceeds the byte limit, also i'm no expert in the differences between them so I just wanted to have it set up for future implementation differences, I can remove generate_scan_data if you like.

...I don't think the macro should do length validation. Legacy advertising and scan packets are limited to 31 bytes, but extended advertising packets can be up to 254 bytes long.

Ok cool I didn't know this, I just read somewhere that it was max 31 bytes, and noticed with my current Softdevice config that there would be a runtime error if either exceeded 31 bytes, but I don't want to defer this check to runtime, maybe somehow the user could specify legacy or extended. For now though, I will comment out the length validation.

alexmoon commented 10 months ago

A few other notes:

AdinAck commented 10 months ago

I think it's important to respect the user's order of fields for the advertising block. Generally speaking it shouldn't matter (other than flags coming first if present), but there may be edge cases.

Good idea I'll get on that.

It's possible to have a mix of 16-bit and 128-bit services in an advertising block. You might even mix a complete 16-bit service field with an incomplete 128-bit service field or vice-versa. I'm not sure if that is supported here?

No it's not, an easy addition though.

Trying to keep up with the list of valid 16-bit UUIDs is probably a bit of a losing battle. I'd recommend an escape hatch to let users provide a custom 16-bit UUID.

Haha yes that's what the Custom service is for. Although I didn't test it with 16bit services I'll make sure that works.

In addition to 16-bit and 128-bit services, there are also 32-bit service UUIDs (I believe these are generally assigned to companies by the Bluetooth SIG).

Yes I list this as a future capability.

In fact, there are a number of data types other than flags, name, or services which can be used in advertising (you can see the current list in Table 1.1 of the Core Specification Supplement). I'm not sure it makes sense to try to support the whole list, but I think you need some sort of escape hatch to let users say "data type N with bytes [1, 2, 3]".

I also list this as something to be implemented in the future.

Some data types (such as manufacturer data) can be (and may need to be) repeated within an advertising block.

My knowledge of how BLE works is in it's infancy, once I understand this I will probably implement it.

Much like how you said the gatt_server proc macro is incomplete and the builder API should be used for more complex systems, this proc macro is also incomplete but the idea is it's better than nothing and is good enough that it fits into every example.

alexmoon commented 10 months ago

In addition to 16-bit and 128-bit services, there are also 32-bit service UUIDs (I believe these are generally assigned to companies by the Bluetooth SIG).

Yes I list this as a future capability.

Oops, sorry. Poor reading comprehension on my part. 😅

Some data types (such as manufacturer data) can be (and may need to be) repeated within an advertising block.

My knowledge of how BLE works is in it's infancy, once I understand this I will probably implement it.

So the manufacturer data is sort of a catch-all that lets people stick arbitrary data in advertising packets. For example, the big OS vendors (Google, Microsoft, maybe Apple?) have special manufacturer data that can let your device pair faster or more easily with their OS. So if you want to support both Google Fast Pair and Windows Swift Pair you'd need manufacturing data fields from both vendors.

Much like how you said the gatt_server proc macro is incomplete and the builder API should be used for more complex systems, this proc macro is also incomplete but the idea is it's better than nothing and is good enough that it fits into every example.

Yep completely agreed. Especially with things like manufacturer data you're never going to have a complete version anyway. As long as there's a way to provide an arbitrary key/value pair I think what you've already got is plenty.

AdinAck commented 10 months ago

That's super interesting, I had no idea different OSs had such a capability.

I'll add the following sometime soon:

dossalab commented 9 months ago

This is definitely a missing piece in the current API. Cannot wait to see it merged!

andelf commented 7 months ago

ch58x-hal met the same problem, as it's also a BLE MCU. I am concerned that excessive abstraction may lead to a significant deviation in the programming model compared to the official C version. Consequently, all online and official references and sample projects would become obsolete.

AdinAck commented 6 months ago

Replaced by #214