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

Improve characteristic interactions #151

Merged
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
9469199
Add gatt_traits code from nrf-softdevice repo
petekubiak Oct 28, 2024
3bd5d74
Add GattServerInterface trait
petekubiak Oct 28, 2024
f1904d0
[WIP] Begin implementing characteristic functions on service
petekubiak Oct 28, 2024
96b5e05
Tidy up for draft
petekubiak Oct 28, 2024
ff36d52
Revert "Add GattServerInterface trait"
petekubiak Oct 30, 2024
d891a68
[WIP] Link characteristic type to handle with PhantomData
petekubiak Oct 30, 2024
6eacfe1
Fix issue with adding device name RO characteristic
petekubiak Oct 31, 2024
91860ef
[WIP] Begin updating examples
petekubiak Nov 1, 2024
d324467
Store a copy of GAP data to ensure required lifetime
petekubiak Nov 4, 2024
b6641fa
Address missing docs errors
petekubiak Nov 4, 2024
ce7a768
[WIP] Add read and write callbacks for attributes
petekubiak Nov 4, 2024
4835725
Remove false optional appearance from characteristic UUID argument
petekubiak Nov 6, 2024
7472d41
[WIP] Add on_read argument to gatt_service macro
petekubiak Nov 7, 2024
b551c7c
Add on_read and on_write arguments to characteristic attributes
petekubiak Nov 7, 2024
fa55978
Change callbacks to take reference to connection rather than ownership
petekubiak Nov 7, 2024
cb8915a
Implement calling callbacks on read and write events
petekubiak Nov 7, 2024
54dc667
Add usage of callbacks to examples and tests
petekubiak Nov 7, 2024
6959861
Add write property to example app characteristic to demonstrate write…
petekubiak Nov 7, 2024
2934ea8
Set ESP32 logging back to info level and allow trace from application
petekubiak Nov 7, 2024
4a9c320
Remove unused code
petekubiak Nov 7, 2024
edd3a8a
Merge remote-tracking branch 'origin/main' into consolidate-character…
petekubiak Nov 7, 2024
1c58d4b
Add debug to write callback in gatt_derive example
petekubiak Nov 7, 2024
ae68984
Replace cloned value with constant in test
petekubiak Nov 7, 2024
000d1e1
Bump version numbers to signify breaking changes
petekubiak Nov 7, 2024
9f7bfc4
Update AttErrorCode to match BT spec v6.0 ATT_ERROR_RSP PDU
petekubiak Nov 8, 2024
e6bca74
Reinstate docstrings for AttErrorCode variants
petekubiak Nov 8, 2024
016e68f
Change write callback to allow accept/reject control over write
petekubiak Nov 8, 2024
9577581
Align conn/connection naming
petekubiak Nov 8, 2024
39ed273
Add comments for unsafe blocks in gatt_traits
petekubiak Nov 8, 2024
c924901
Give from_gatt a Result return type to remove embedded unwraps
petekubiak Nov 8, 2024
23b13fb
Revert "Bump version numbers to signify breaking changes"
petekubiak Nov 8, 2024
1faee8d
Remove optional arguments from API and implement setting callbacks in…
petekubiak Nov 8, 2024
5fe0463
Add examples of callbacks to service macro docstring
petekubiak Nov 8, 2024
a6d1738
Move initialisation of device name memory to GapConfig
petekubiak Nov 8, 2024
49d1991
Merge branch 'main' into consolidate-characteristic-interactions
petekubiak Nov 11, 2024
ccbab50
Fix gatt_derive test for expected rejection of first write request
petekubiak Nov 11, 2024
00f2d0d
Merge branch 'consolidate-characteristic-interactions' of github.com:…
petekubiak Nov 11, 2024
2e2984c
Fix inverted logic in testing of write callback rejection mechanism
petekubiak Nov 11, 2024
3ec80b5
Fix the other inverted logic in testing of write callback rejection m…
petekubiak Nov 11, 2024
394cba9
Return an error from AttributeTable::set() rather than asserting data…
petekubiak Nov 11, 2024
1d49296
Remove panics from gatt_traits and update docstrings
petekubiak Nov 11, 2024
9ff71f1
Handle from_gatt error in attribute.rs rather than unwrapping
petekubiak Nov 11, 2024
adb1c9b
Change notify function to take characteristic reference rather than o…
petekubiak Nov 12, 2024
3603109
Update example to match modified notify function signature
petekubiak Nov 12, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/apps/src/ble_bas_central.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ where
let service = services.first().unwrap().clone();

info!("Looking for value handle");
let c = client
let c: Characteristic<u8> = client
.characteristic_by_uuid(&service, &Uuid::new_short(0x2a19))
.await
.unwrap();
Expand Down
33 changes: 23 additions & 10 deletions examples/apps/src/ble_bas_peripheral.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,18 @@ struct Server {
// Battery service
#[gatt_service(uuid = "180f")]
struct BatteryService {
#[characteristic(uuid = "2a19", read, notify)]
#[characteristic(uuid = "2a19", read, write, notify, on_read = battery_level_on_read, on_write = battery_level_on_write)]
level: u8,
}

fn battery_level_on_read(_connection: &Connection) {
info!("[gatt] Read event on battery level characteristic");
}

fn battery_level_on_write(_connection: &Connection, data: &[u8]) {
info!("[gatt] Write event on battery level characteristic: {:?}", data);
}

pub async fn run<C>(controller: C)
where
C: Controller,
Expand All @@ -46,7 +54,8 @@ where
name: "TrouBLE",
appearance: &appearance::GENERIC_POWER,
}),
);
)
.unwrap();

info!("Starting advertising and GATT service");
let _ = join3(
Expand All @@ -61,16 +70,20 @@ async fn ble_task<C: Controller>(mut runner: Runner<'_, C>) -> Result<(), BleHos
runner.run().await
}

async fn gatt_task<C: Controller>(server: &Server<'_,'_, C>) {
async fn gatt_task<C: Controller>(server: &Server<'_, '_, C>) {
loop {
match server.next().await {
Ok(GattEvent::Write { handle, connection: _ }) => {
let _ = server.get(handle, |value| {
info!("[gatt] Write event on {:?}. Value written: {:?}", handle, value);
});
Ok(GattEvent::Write {
value_handle,
connection: _,
}) => {
info!("[gatt] Write event on {:?}", value_handle);
}
Ok(GattEvent::Read { handle, connection: _ }) => {
info!("[gatt] Read event on {:?}", handle);
Ok(GattEvent::Read {
value_handle,
connection: _,
}) => {
info!("[gatt] Read event on {:?}", value_handle);
}
Err(e) => {
error!("[gatt] Error processing GATT events: {:?}", e);
Expand Down Expand Up @@ -111,7 +124,7 @@ async fn advertise_task<C: Controller>(
Timer::after(Duration::from_secs(2)).await;
tick = tick.wrapping_add(1);
info!("[adv] notifying connection of tick {}", tick);
let _ = server.notify(server.battery_service.level, &conn, &[tick]).await;
let _ = server.notify(server.battery_service.level, &conn, &tick).await;
jamessizeland marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
6 changes: 3 additions & 3 deletions examples/esp32/.cargo/config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@ runner = "espflash flash --monitor"


[env]
ESP_LOGLEVEL="trace"
ESP_LOGTARGETS="trouble_host"
ESP_LOGLEVEL = "info"

[build]
rustflags = [
# Required to obtain backtraces (e.g. when using the "esp-backtrace" crate.)
# NOTE: May negatively impact performance of produced code
"-C", "force-frame-pointers",
"-C",
"force-frame-pointers",
]

target = "riscv32imc-unknown-none-elf"
Expand Down
2 changes: 1 addition & 1 deletion host-macro/Cargo.toml
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "trouble-host-macros"
version = "0.1.0"
version = "0.2.0"
petekubiak marked this conversation as resolved.
Show resolved Hide resolved
edition = "2021"
license = "Apache-2.0 or MIT"
description = "An async Rust BLE host - Derive macros crate"
Expand Down
67 changes: 47 additions & 20 deletions host-macro/src/characteristic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ use darling::Error;
use darling::FromMeta;
use proc_macro2::Span;
use syn::parse::Result;
use syn::spanned::Spanned as _;
use syn::spanned::Spanned;
use syn::Field;
use syn::Ident;
use syn::LitStr;

#[derive(Debug)]
Expand Down Expand Up @@ -47,10 +48,10 @@ pub(crate) struct DescriptorArgs {
}

/// Characteristic attribute arguments
#[derive(Debug, FromMeta, Default)]
#[derive(Debug, FromMeta)]
pub(crate) struct CharacteristicArgs {
/// The UUID of the characteristic.
pub uuid: Option<Uuid>,
pub uuid: Uuid,
/// If true, the characteristic can be read.
#[darling(default)]
pub read: bool,
Expand All @@ -66,39 +67,57 @@ pub(crate) struct CharacteristicArgs {
/// If true, the characteristic can send indications.
#[darling(default)]
pub indicate: bool,
/// Optional callback to be triggered on a read event
#[darling(default)]
pub on_read: Option<Ident>,
/// Optional callback to be triggered on a write event
#[darling(default)]
pub on_write: Option<Ident>,
/// The initial value of the characteristic.
/// This is optional and can be used to set the initial value of the characteristic.
#[darling(default)]
pub value: Option<syn::Expr>,
pub _default_value: Option<syn::Expr>,
// /// Descriptors for the characteristic.
// /// Descriptors are optional and can be used to add additional metadata to the characteristic.
#[darling(default, multiple)]
pub _descriptor: Vec<DescriptorArgs>,
pub _descriptors: Vec<DescriptorArgs>,
}

impl CharacteristicArgs {
/// Parse the arguments of a characteristic attribute
pub fn parse(attribute: &syn::Attribute) -> Result<Self> {
let mut args = CharacteristicArgs::default();
let mut uuid: Option<Uuid> = None;
let mut read = false;
let mut write = false;
let mut write_without_response = false;
let mut notify = false;
let mut indicate = false;
let mut on_read = None;
let mut on_write = None;
let mut _default_value: Option<syn::Expr> = None;
let descriptors: Vec<DescriptorArgs> = Vec::new();
attribute.parse_nested_meta(|meta| {
match meta.path.get_ident().ok_or(Error::custom("no ident"))?.to_string().as_str() {
"uuid" => {
let value = meta
.value()
.map_err(|_| Error::custom("uuid must be followed by '= [data]'. i.e. uuid = '0x2A37'".to_string()))?;
let uuid_string: LitStr = value.parse()?;
args.uuid = Some(Uuid::from_string(uuid_string.value().as_str())?);
uuid = Some(Uuid::from_string(uuid_string.value().as_str())?);
},
"read" => args.read = true,
"write" => args.write = true,
"write_without_response" => args.write_without_response = true,
"notify" => args.notify = true,
"indicate" => args.indicate = true,
"read" => read = true,
"write" => write = true,
"write_without_response" => write_without_response = true,
"notify" => notify = true,
"indicate" => indicate = true,
"on_read" => on_read = Some(meta.value()?.parse()?),
"on_write" => on_write = Some(meta.value()?.parse()?),
"value" => {
let value = meta
.value()
.map_err(|_| Error::custom("value must be followed by '= [data]'. i.e. value = 'hello'".to_string()))?;
args.value = Some(value.parse()?);
return Err(Error::custom("Default value is currently unsupported").with_span(&meta.path.span()).into())
// let value = meta
// .value()
// .map_err(|_| Error::custom("value must be followed by '= [data]'. i.e. value = 'hello'".to_string()))?;
// default_value = Some(value.parse()?);
},
other => return Err(
meta.error(
Expand All @@ -108,9 +127,17 @@ impl CharacteristicArgs {
};
Ok(())
})?;
if args.uuid.is_none() {
return Err(Error::custom("Characteristic must have a UUID").into());
}
Ok(args)
Ok(Self {
uuid: uuid.ok_or(Error::custom("Characteristic must have a UUID"))?,
read,
write,
write_without_response,
notify,
indicate,
on_read,
on_write,
_default_value,
_descriptors: descriptors,
})
}
}
19 changes: 5 additions & 14 deletions host-macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ use ctxt::Ctxt;
use proc_macro::TokenStream;
use server::{ServerArgs, ServerBuilder};
use service::{ServiceArgs, ServiceBuilder};
use syn::parse_macro_input;

/// Gatt Service attribute macro.
///
Expand Down Expand Up @@ -73,21 +74,11 @@ pub fn gatt_server(args: TokenStream, item: TokenStream) -> TokenStream {
/// ```
#[proc_macro_attribute]
pub fn gatt_service(args: TokenStream, item: TokenStream) -> TokenStream {
let service_uuid = {
// Get arguments from the gatt_service macro attribute (i.e. uuid)
let service_attributes: ServiceArgs = {
let mut attributes = ServiceArgs::default();
let arg_parser = syn::meta::parser(|meta| attributes.parse(meta));

syn::parse_macro_input!(args with arg_parser);
attributes
};
service_attributes.uuid
}
.expect("uuid is required for gatt_service");
// Get arguments from the gatt_service macro attribute
let service_arguments = parse_macro_input!(args as ServiceArgs);
jamessizeland marked this conversation as resolved.
Show resolved Hide resolved

// Parse the contents of the struct
let mut service_props = syn::parse_macro_input!(item as syn::ItemStruct);
let mut service_props = parse_macro_input!(item as syn::ItemStruct);

let ctxt = Ctxt::new(); // error handling context, must be initialized after parse_macro_input calls.

Expand Down Expand Up @@ -119,7 +110,7 @@ pub fn gatt_service(args: TokenStream, item: TokenStream) -> TokenStream {
}

// Build the service struct
let result = ServiceBuilder::new(service_props, service_uuid)
let result = ServiceBuilder::new(service_props, service_arguments)
.process_characteristics_and_fields(fields, characteristics)
.build();

Expand Down
56 changes: 45 additions & 11 deletions host-macro/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,12 @@ impl ServerBuilder {
});
}

const GAP_UUID: u16 = 0x1800;
const GATT_UUID: u16 = 0x1801;

const DEVICE_NAME_UUID: u16 = 0x2a00;
const APPEARANCE_UUID: u16 = 0x2a01;

quote! {
#visibility struct #name<'reference, 'values, C: Controller>
{
Expand All @@ -119,40 +125,68 @@ impl ServerBuilder {
/// Create a new Gatt Server instance.
///
/// This function will add a Generic GAP Service with the given name.
#visibility fn new_default(stack: Stack<'reference, C>, name: &'values str) -> Self {
/// The maximum length which the name can be is 22 bytes (limited by the size of the advertising packet).
/// If a name longer than this is passed, Err() is returned.
#visibility fn new_default(stack: Stack<'reference, C>, name: &'values str) -> Result<Self, &'static str> {
let mut table: AttributeTable<'_, #mutex_type, #attribute_data_size> = AttributeTable::new();

GapConfig::default(name).build(&mut table);
static DEVICE_NAME: static_cell::StaticCell<HeaplessString<{DEVICE_NAME_MAX_LENGTH}>> = static_cell::StaticCell::new();
Copy link
Collaborator

Choose a reason for hiding this comment

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

this could be incorporated into a method in the GapConfig?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The reason I didn't do that was because if the user attempted to create a second GapConfig then it would panic. Admittedly this implementation would panic if the user attempted to create a second instance of the type decorated with #[gatt_server], but that would happen anyway due to the characteristic data stores.
If a user is only ever going to create one GapConfig then moving this code to a GapConfig method removes some duplicate code, which I'm always for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Yeah, being able to create multiple instances of a #[gatt_server] will likely come up in some applications. I'd suggest just putting an owned heapless::String directly in the GapConfig so you don't have to worry about lifetimes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah interesting - we were discussing this earlier and thought that creating multiple #[gatt_servers] in the same application wouldn't be required.

In that case I will need to revert a6d1738 as it will not allow for a second GapConfig::Peripheral to be made (whereas previously you could make as many GapConfigs as you desired but could only create one instance of a particular #[gatt_server] (but declaring a second #[gatt_server] and instantiating that would work fine).

I notice you said "putting an owned heapless::String directly into the GapConfig" rather than using a statically allocated heapless::String - the reason I haven't done that is that the GapConfig only exists for a short period of time, so the user would need to hang on to the GapConfig for the lifetime of the #[gatt_server].

I originally tried passing ownership of the GapConfig to the #[gatt_server], but this ends up in a chicken-and-egg situation where a reference to the String in the GapConfig is required for the attribute, but the GapConfig needs to be moved into the #[gatt_server] to complete its initialisation, thus invalidating the reference. Moving the GapConfig into the #[gatt_server] first and then adding the attributes has the same problem, as the #[gatt_server] is returned from the function at the end of initialisation, again causing a move and invalidating the reference.

I encountered this issue a few times during the course of this work - is there a pattern to get around it without using statics?

let device_name = DEVICE_NAME.init(HeaplessString::new());
if device_name.push_str(name).is_err() {
return Err("Name is too long. Device name must be <= 22 bytes");
};
let mut svc = table.add_service(Service::new(#GAP_UUID), None);
svc.add_characteristic_ro(#DEVICE_NAME_UUID, device_name, None);
svc.add_characteristic_ro(#APPEARANCE_UUID, &appearance::GENERIC_UNKNOWN, None);
svc.build();

table.add_service(Service::new(#GATT_UUID), None);

#code_service_init

Self {
Ok(Self {
server: GattServer::new(stack, table),
#code_server_populate
}
})
}

/// Create a new Gatt Server instance.
///
/// This function will add a GAP Service.
#visibility fn new_with_config(stack: Stack<'reference, C>, gap: GapConfig<'values>) -> Self {
/// The maximum length which the device name can be is 22 bytes (limited by the size of the advertising packet).
/// If a name longer than this is passed, Err() is returned.
#visibility fn new_with_config(stack: Stack<'reference, C>, gap: GapConfig<'values>) -> Result<Self, &'static str> {
let mut table: AttributeTable<'_, #mutex_type, #attribute_data_size> = AttributeTable::new();

gap.build(&mut table);
static DEVICE_NAME: static_cell::StaticCell<HeaplessString<{DEVICE_NAME_MAX_LENGTH}>> = static_cell::StaticCell::new();
let device_name = DEVICE_NAME.init(HeaplessString::new());
let (name, appearance) = match gap {
GapConfig::Peripheral(config) => (config.name, config.appearance),
GapConfig::Central(config) => (config.name, config.appearance),
};
if device_name.push_str(name).is_err() {
return Err("Name is too long. Device name must be <= 22 bytes");
};
let mut svc = table.add_service(Service::new(#GAP_UUID), None);
svc.add_characteristic_ro(#DEVICE_NAME_UUID, device_name, None);
svc.add_characteristic_ro(#APPEARANCE_UUID, appearance, None);
svc.build();

table.add_service(Service::new(#GATT_UUID), None);

#code_service_init

Self {
Ok(Self {
server: GattServer::new(stack, table),
#code_server_populate
}
})
}

#visibility fn get<F: FnMut(&[u8]) -> T, T>(&self, handle: Characteristic, f: F) -> Result<T, Error> {
self.server.server().table().get(handle, f)
#visibility fn get<T: trouble_host::types::gatt_traits::GattValue>(&self, handle: &Characteristic<T>) -> Result<T, Error> {
self.server.server().table().get(handle)
}

#visibility fn set(&self, handle: Characteristic, input: &[u8]) -> Result<(), Error> {
#visibility fn set<T: trouble_host::types::gatt_traits::GattValue>(&self, handle: &Characteristic<T>, input: &T) -> Result<(), Error> {
self.server.server().table().set(handle, input)
}
}
Expand Down
Loading
Loading