From a2a8c94e7dbb18f9514fb59b68ce6d945ffe9e41 Mon Sep 17 00:00:00 2001 From: Jonathan Shi Date: Mon, 11 Jan 2021 11:40:29 -0800 Subject: [PATCH] Basic writeable connection attributes + attribute permissions (#171) * Start of writeable connection attribute user interface * WIP interface as of 08/21. * Add permission checks on type/group reads * Start writeable attribute demo (stuck on mutability of state) * Print on receiving write * Figure out how to store state * More state troubles (last time forgot to mutate state) * Use set_value to no avail * Identify lifetime issue * Integrate ownable attr value * Lazily generate attributes for LED demo * Uncomment features for boards besides 52832 * Clarify comment on read perm issue * Remove extra as_ref * Fix missing nrf52833 in write demo * Randomly generate UUID for led demo * Forgot to run rustfmt * Merge writeable demo with original demo * log instead of unwrap on failed writereq * log instead of unwrap on failed write cmd Co-authored-by: David Ross --- demos/nrf52-demo/README.md | 2 + demos/nrf52-demo/src/attrs.rs | 213 ++++++++++++++++++++++++++++++++++ demos/nrf52-demo/src/main.rs | 45 ++++--- rubble/src/att/mod.rs | 85 ++++++++++---- rubble/src/att/server.rs | 77 ++++++++---- rubble/src/gatt/mod.rs | 10 +- 6 files changed, 369 insertions(+), 63 deletions(-) create mode 100644 demos/nrf52-demo/src/attrs.rs diff --git a/demos/nrf52-demo/README.md b/demos/nrf52-demo/README.md index 05764600a..b4792e437 100644 --- a/demos/nrf52-demo/README.md +++ b/demos/nrf52-demo/README.md @@ -2,6 +2,8 @@ This is a demo application we're using when hacking on Rubble. It runs on any chip in the nRF52 family. +It exposes a dummy read-only battery service attribute, as well as a read/write attribute that +toggles an on-board LED (pin 17 by default). The demo allows establishing a connection and provides a GATT server. A *lot* of things are logged over the UART, so it's recommended to hook that up. diff --git a/demos/nrf52-demo/src/attrs.rs b/demos/nrf52-demo/src/attrs.rs new file mode 100644 index 000000000..5117ef0c8 --- /dev/null +++ b/demos/nrf52-demo/src/attrs.rs @@ -0,0 +1,213 @@ +//! Defines a custom `AttributeValueProvider`. + +#[cfg(feature = "52810")] +use nrf52810_hal as hal; +#[cfg(feature = "52832")] +use nrf52832_hal as hal; +#[cfg(feature = "52833")] +use nrf52833_hal as hal; +#[cfg(feature = "52840")] +use nrf52840_hal as hal; + +use hal::{ + gpio::{Output, Pin, PushPull}, + prelude::OutputPin, +}; +use rubble::{ + att::{AttUuid, Attribute, AttributeAccessPermissions, AttributeProvider, Handle, HandleRange}, + uuid::{Uuid128, Uuid16}, + Error, +}; + +pub struct DemoAttrs { + // Attributes exposed to clients that don't change. + // This includes the "primary service" and "characteristic" attributes. + // Some attributes are copied from the declaration of `BatteryServiceAttrs` in the gatt module. + static_attributes: [Attribute<&'static [u8]>; 6], + // State and resources to be modified/queried when packets are received. + // The `AttributeValueProvider` interface allows attributes to be generated lazily; those + // attributes should use these fields. + led_pin: Pin>, + led_buf: [u8; 1], +} + +const PRIMARY_SERVICE_UUID16: Uuid16 = Uuid16(0x2800); +const CHARACTERISTIC_UUID16: Uuid16 = Uuid16(0x2803); +const GENERIC_ATTRIBUTE_UUID16: Uuid16 = Uuid16(0x1801); +const BATTERY_LEVEL_UUID16: Uuid16 = Uuid16(0x2A19); + +// Randomly generated +// a86a62f0-5d26-4538-b364-5654961515c9 +const LED_UUID128: [u8; 16] = [ + 0xC9, 0x15, 0x15, 0x96, 0x54, 0x56, 0x64, 0xB3, 0x38, 0x45, 0x26, 0x5D, 0xF0, 0x62, 0x6A, 0xA8, +]; +// Replace bytes 12/13 (0x62F0) of the 128-bit UUID with 62F1 +const LED_STATE_CHAR_UUID128: [u8; 16] = [ + 0xC9, 0x15, 0x15, 0x96, 0x54, 0x56, 0x64, 0xB3, 0x38, 0x45, 0x26, 0x5D, 0xF1, 0x62, 0x6A, 0xA8, +]; + +const LED_CHAR_DECL_VALUE: [u8; 19] = [ + 0x02 | 0x08, // 0x02 = read, 0x08 = write with response + // 2 byte handle pointing to characteristic value + 0x03, + 0x00, + // 128-bit UUID of characteristic value (copied from above constant) + 0xC9, + 0x15, + 0x15, + 0x96, + 0x54, + 0x56, + 0x64, + 0xB3, + 0x38, + 0x45, + 0x26, + 0x5D, + 0xF1, + 0x62, + 0x6A, + 0xA8, +]; + +impl DemoAttrs { + pub fn new(mut led_pin: Pin>) -> Self { + // Turn off by default (active low) + led_pin.set_high().unwrap(); + Self { + static_attributes: [ + Attribute::new( + PRIMARY_SERVICE_UUID16.into(), + Handle::from_raw(0x0001), + &LED_UUID128, + ), + Attribute::new( + CHARACTERISTIC_UUID16.into(), + Handle::from_raw(0x0002), + &LED_CHAR_DECL_VALUE, + ), + // 0x0003 is skipped because it's lazily generated + // Dummy ending attribute + // This needs to come after our lazily generated data attribute because group_end() + // needs to return a reference + Attribute::new( + GENERIC_ATTRIBUTE_UUID16.into(), + Handle::from_raw(0x0004), + &[], + ), + // Below is copied from `gatt::BatteryServiceAttrs` + Attribute::new( + PRIMARY_SERVICE_UUID16.into(), + Handle::from_raw(0x0005), + &[0x0F, 0x18], // "Battery Service" = 0x180F + ), + Attribute::new( + CHARACTERISTIC_UUID16.into(), + Handle::from_raw(0x0006), + &[ + 0x02, // 1 byte properties: READ = 0x02 + 0x07, 0x00, // 2 bytes handle = 0x0007 + 0x19, 0x2A, // 2 bytes UUID = 0x2A19 (Battery Level) + ], + ), + // Characteristic value (Battery Level) + Attribute::new( + BATTERY_LEVEL_UUID16.into(), + Handle::from_raw(0x0007), + &[48u8], + ), + ], + led_pin, + led_buf: [0u8], + } + } +} + +impl DemoAttrs { + // Lazily produces an attribute to be read/written, representing the LED state. + fn led_data_attr(&self) -> Attribute<[u8; 1]> { + Attribute::new( + Uuid128::from_bytes(LED_STATE_CHAR_UUID128).into(), + Handle::from_raw(0x0003), + self.led_buf, + ) + } +} + +impl AttributeProvider for DemoAttrs { + /// Retrieves the permissions for attribute with the given handle. + fn attr_access_permissions(&self, handle: Handle) -> AttributeAccessPermissions { + match handle.as_u16() { + 0x0003 => AttributeAccessPermissions::ReadableAndWriteable, + _ => AttributeAccessPermissions::Readable, + } + } + + /// Attempts to write data to the attribute with the given handle. + /// If any of your attributes are writeable, this function must be implemented. + fn write_attr(&mut self, handle: Handle, data: &[u8]) -> Result<(), Error> { + match handle.as_u16() { + 0x0003 => { + if data.is_empty() { + return Err(Error::InvalidLength); + } + // If we receive a 1, activate the LED; otherwise deactivate it + // Assumes LED is active low + if data[0] == 1 { + self.led_pin.set_low().unwrap(); + } else { + self.led_pin.set_high().unwrap(); + } + // Copy written value into buffer to display back for reading + self.led_buf.copy_from_slice(data); + Ok(()) + } + _ => panic!("Attempted to write an unwriteable attribute"), + } + } + + fn is_grouping_attr(&self, uuid: AttUuid) -> bool { + uuid == PRIMARY_SERVICE_UUID16 || uuid == CHARACTERISTIC_UUID16 + } + + fn group_end(&self, handle: Handle) -> Option<&Attribute>> { + match handle.as_u16() { + // Handles for the LED primary service and characteristic + // The group end is a dummy attribute; strictly speaking it's not required + // but we can't use the lazily generated attribute because this funtion requires + // returning a reference + 0x0001 | 0x0002 => Some(&self.static_attributes[2]), + // Handles for Battery Service + 0x0005 | 0x0006 => Some(&self.static_attributes[5]), + _ => None, + } + } + + /// Applies a function to all attributes with handles within the specified range + fn for_attrs_in_range( + &mut self, + range: HandleRange, + mut f: impl FnMut(&Self, &Attribute>) -> Result<(), Error>, + ) -> Result<(), Error> { + // Handles start at 1, not 0, but we're not directly indexing + let start = range.start().as_u16(); + let end = range.end().as_u16(); + let range_u16 = start..=end; + // Can't just iterate from start to end because of the presence of lazy attributes + // Ranges are empty if start >= end + for attr in &self.static_attributes { + if range_u16.contains(&attr.handle.as_u16()) { + f(self, attr)?; + } + } + // Check lazy attributes + // Note that with this implementation, if a static attribute has handle greater than a + // lazy attribute, the order in which f() is applied is not preserved. + // This may matter for the purposes of short-circuiting an operation if it cannot be applied + // to a particular attribute + if range_u16.contains(&0x0003) { + f(self, &self.led_data_attr())?; + }; + Ok(()) + } +} diff --git a/demos/nrf52-demo/src/main.rs b/demos/nrf52-demo/src/main.rs index 257dfb6cd..d89bdd260 100644 --- a/demos/nrf52-demo/src/main.rs +++ b/demos/nrf52-demo/src/main.rs @@ -35,6 +35,7 @@ macro_rules! config { #[macro_use] mod config; +mod attrs; mod logger; // Import the right HAL/PAC crate, depending on the target chip @@ -48,24 +49,38 @@ use nrf52833_hal as hal; use nrf52840_hal as hal; use bbqueue::Consumer; -use core::fmt::Write; -use core::sync::atomic::{compiler_fence, Ordering}; -use hal::uarte::{Baudrate, Parity, Uarte}; -use hal::{gpio::Level, pac::UARTE0}; -use rubble::l2cap::{BleChannelMap, L2CAPState}; -use rubble::link::queue::{PacketQueue, SimpleQueue}; -use rubble::link::{ad_structure::AdStructure, LinkLayer, Responder, MIN_PDU_BUF}; -use rubble::time::{Duration, Timer}; -use rubble::{config::Config, gatt::BatteryServiceAttrs, security::NoSecurity}; -use rubble_nrf5x::radio::{BleRadio, PacketBuffer}; -use rubble_nrf5x::{timer::BleTimer, utils::get_device_address}; +use core::{ + fmt::Write, + sync::atomic::{compiler_fence, Ordering}, +}; +use hal::{ + gpio::Level, + pac::UARTE0, + uarte::{Baudrate, Parity, Uarte}, +}; +use rubble::{ + config::Config, + l2cap::{BleChannelMap, L2CAPState}, + link::{ + ad_structure::AdStructure, + queue::{PacketQueue, SimpleQueue}, + LinkLayer, Responder, MIN_PDU_BUF, + }, + security::NoSecurity, + time::{Duration, Timer}, +}; +use rubble_nrf5x::{ + radio::{BleRadio, PacketBuffer}, + timer::BleTimer, + utils::get_device_address, +}; pub enum AppConfig {} impl Config for AppConfig { type Timer = BleTimer; type Transmitter = BleRadio; - type ChannelMapper = BleChannelMap; + type ChannelMapper = BleChannelMap; type PacketQueue = &'static mut SimpleQueue; } @@ -121,10 +136,14 @@ const APP: () = { // Create the actual BLE stack objects let mut ble_ll = LinkLayer::::new(device_address, ble_timer); + // Assumes pin 17 corresponds to an LED. + // On the NRF52DK board, this is LED 1. let ble_r = Responder::new( tx, rx, - L2CAPState::new(BleChannelMap::with_attributes(BatteryServiceAttrs::new())), + L2CAPState::new(BleChannelMap::with_attributes(attrs::DemoAttrs::new( + p0.p0_17.into_push_pull_output(Level::High).degrade(), + ))), ); // Send advertisement and set up regular interrupt diff --git a/rubble/src/att/mod.rs b/rubble/src/att/mod.rs index 51cea7d1f..43acbf504 100644 --- a/rubble/src/att/mod.rs +++ b/rubble/src/att/mod.rs @@ -41,23 +41,6 @@ pub use self::handle::{Handle, HandleRange}; pub use self::server::{AttributeServer, AttributeServerTx}; pub use self::uuid::AttUuid; -/// An attribute value that can be represented as a byte slice. -pub trait AttrValue { - fn as_slice(&self) -> &[u8]; -} - -impl AttrValue for &[u8] { - fn as_slice(&self) -> &[u8] { - self - } -} - -impl AttrValue for () { - fn as_slice(&self) -> &[u8] { - &[] - } -} - /// An ATT server attribute pub struct Attribute where @@ -72,20 +55,20 @@ where pub value: T, } -impl Attribute { +impl> Attribute { /// Creates a new attribute. pub fn new(att_type: AttUuid, handle: Handle, value: T) -> Self { assert_ne!(handle, Handle::NULL); Attribute { att_type, handle, - value: value, + value, } } /// Retrieves the attribute's value as a slice. pub fn value(&self) -> &[u8] { - self.value.as_slice() + self.value.as_ref() } /// Overrides the previously set attribute's value. @@ -94,6 +77,35 @@ impl Attribute { } } +pub enum AttributeAccessPermissions { + Readable, + Writeable, + ReadableAndWriteable, +} + +impl AttributeAccessPermissions { + fn is_readable(&self) -> bool { + match self { + AttributeAccessPermissions::Readable + | AttributeAccessPermissions::ReadableAndWriteable => true, + AttributeAccessPermissions::Writeable => false, + } + } + fn is_writeable(&self) -> bool { + match self { + AttributeAccessPermissions::Writeable + | AttributeAccessPermissions::ReadableAndWriteable => true, + AttributeAccessPermissions::Readable => false, + } + } +} + +impl Default for AttributeAccessPermissions { + fn default() -> Self { + AttributeAccessPermissions::Readable + } +} + /// Trait for attribute sets that can be hosted by an `AttributeServer`. pub trait AttributeProvider { /// Calls a closure `f` with every attribute whose handle is inside `range`, ascending. @@ -107,7 +119,7 @@ pub trait AttributeProvider { fn for_attrs_in_range( &mut self, range: HandleRange, - f: impl FnMut(&Self, &Attribute) -> Result<(), Error>, + f: impl FnMut(&Self, &Attribute>) -> Result<(), Error>, ) -> Result<(), Error>; /// Returns whether `uuid` is a valid grouping attribute type that can be used in *Read By @@ -129,7 +141,32 @@ pub trait AttributeProvider { /// last attribute contained within that service. /// /// TODO: document what the BLE spec has to say about grouping for characteristics. - fn group_end(&self, handle: Handle) -> Option<&Attribute>; + fn group_end(&self, handle: Handle) -> Option<&Attribute>>; + + /// Retrieves the permissions for the given attribute. + /// + /// These are used purely for access control within rubble, and won't be + /// communicated with clients. They should be coordinated beforehand as part + /// of a larger protocol. + /// + /// Defaults to read-only. If this is overwritten and some attributes are made writeable, + /// `write_attribute` must be implemented as well. + fn attr_access_permissions(&self, _handle: Handle) -> AttributeAccessPermissions { + AttributeAccessPermissions::Readable + } + + /// Attempts to write data to the given attribute. + /// + /// This will only be called on handles for which + /// `attribute_access_permissions` returns + /// [`AttributeAccessPermissions::Writeable`] + /// or [`AttributeAccessPermissions::ReadableAndWriteable`]. + /// + /// By default, panics on all writes. This must be overwritten if + /// `attribute_access_permissions` is. + fn write_attr(&mut self, _handle: Handle, _data: &[u8]) -> Result<(), Error> { + unimplemented!("by default, no attributes should have write access permissions, and this should never be called"); + } } /// An empty attribute set. @@ -141,7 +178,7 @@ impl AttributeProvider for NoAttributes { fn for_attrs_in_range( &mut self, _range: HandleRange, - _f: impl FnMut(&Self, &Attribute) -> Result<(), Error>, + _f: impl FnMut(&Self, &Attribute>) -> Result<(), Error>, ) -> Result<(), Error> { Ok(()) } @@ -150,7 +187,7 @@ impl AttributeProvider for NoAttributes { false } - fn group_end(&self, _handle: Handle) -> Option<&Attribute> { + fn group_end(&self, _handle: Handle) -> Option<&Attribute>> { None } } diff --git a/rubble/src/att/server.rs b/rubble/src/att/server.rs index 6117a59d2..db28b9d9b 100644 --- a/rubble/src/att/server.rs +++ b/rubble/src/att/server.rs @@ -96,10 +96,14 @@ impl AttributeServer { let mut size = None; let att_mtu = self.att_mtu(); self.attrs - .for_attrs_in_range(range, |_provider, attr| { - if attr.att_type == *attribute_type { + .for_attrs_in_range(range, |provider, attr| { + // "Only attributes that can be read shall be returned in a + // Read By Type Response." + if attr.att_type == *attribute_type + && provider.attr_access_permissions(attr.handle).is_readable() + { let data = - ByTypeAttData::new(att_mtu, attr.handle, attr.value.as_slice()); + ByTypeAttData::new(att_mtu, attr.handle, attr.value.as_ref()); if size == Some(data.encoded_size()) || size.is_none() { // Can try to encode `data`. If we run out of space, end the list. data.to_bytes(writer)?; @@ -151,12 +155,14 @@ impl AttributeServer { let att_mtu = self.att_mtu(); self.attrs .for_attrs_in_range(range, |provider, attr| { - if attr.att_type == *group_type { + if attr.att_type == *group_type + && provider.attr_access_permissions(attr.handle).is_readable() + { let data = ByGroupAttData::new( att_mtu, attr.handle, provider.group_end(attr.handle).unwrap().handle, - attr.value.as_slice(), + attr.value.as_ref(), ); if size == Some(data.encoded_size()) || size.is_none() { // Can try to encode `data`. If we run out of space, end the list. @@ -197,10 +203,17 @@ impl AttributeServer { self.attrs.for_attrs_in_range( HandleRange::new(*handle, *handle), |_provider, attr| { - let value = if writer.space_left() < attr.value.as_slice().len() { - &attr.value.as_slice()[..writer.space_left()] + // FIXME return if attribute is not readable + // This code currently doesn't work because the callback should + // return rubble::Error rather than an AtError + // if !self.attrs.attr_access_permissions(*handle).is_readable() { + // return + // Err(AttError::new(ErrorCode::ReadNotPermitted, *handle)) + // } + let value = if writer.space_left() < attr.value.as_ref().len() { + &attr.value.as_ref()[..writer.space_left()] } else { - attr.value.as_slice() + attr.value.as_ref() }; writer.write_slice(value) }, @@ -213,17 +226,40 @@ impl AttributeServer { Ok(()) } - AttPdu::WriteReq { .. } => { - // FIXME: ATT Writes are not yet supported, but we pretend they work so that some - // applications that only need CCCD writes work (eg. BLE MIDI). - warn!("NYI: ATT Write Req"); - - responder - .send_with(|writer| -> Result<(), Error> { - writer.write_u8(Opcode::WriteRsp.into())?; - Ok(()) - }) - .unwrap(); + AttPdu::WriteReq { value, handle } => { + if self.attrs.attr_access_permissions(*handle).is_writeable() { + self.attrs + .write_attr(*handle, value.as_ref()) + .map_err(|err| { + // Convert rubble::Error to AttError + AttError::new( + match err { + Error::InvalidLength => ErrorCode::InvalidAttributeValueLength, + _ => ErrorCode::UnlikelyError, + }, + *handle, + ) + })?; + responder + .send_with(|writer| -> Result<(), Error> { + writer.write_u8(Opcode::WriteRsp.into())?; + Ok(()) + }) + .map_err(|err| error!("error while handling write request: {:?}", err)) + .ok(); + Ok(()) + } else { + Err(AttError::new(ErrorCode::WriteNotPermitted, *handle)) + } + } + AttPdu::WriteCommand { handle, value } => { + // WriteCommand shouldn't respond to the client even on failure + if self.attrs.attr_access_permissions(*handle).is_writeable() { + self.attrs + .write_attr(*handle, value.as_ref()) + .map_err(|err| error!("error while handling write command: {:?}", err)) + .ok(); + } Ok(()) } @@ -251,10 +287,9 @@ impl AttributeServer { | AttPdu::FindByTypeValueReq { .. } | AttPdu::ReadBlobReq { .. } | AttPdu::ReadMultipleReq { .. } - | AttPdu::WriteCommand { .. } - | AttPdu::SignedWriteCommand { .. } | AttPdu::PrepareWriteReq { .. } | AttPdu::ExecuteWriteReq { .. } + | AttPdu::SignedWriteCommand { .. } | AttPdu::HandleValueConfirmation { .. } => { if msg.opcode().is_command() { // According to the spec, unknown Command PDUs should be ignored diff --git a/rubble/src/gatt/mod.rs b/rubble/src/gatt/mod.rs index acd6e12f3..dc491fdcc 100644 --- a/rubble/src/gatt/mod.rs +++ b/rubble/src/gatt/mod.rs @@ -5,7 +5,7 @@ pub mod characteristic; -use crate::att::{AttUuid, AttrValue, Attribute, AttributeProvider, Handle, HandleRange}; +use crate::att::{AttUuid, Attribute, AttributeProvider, Handle, HandleRange}; use crate::uuid::{Uuid128, Uuid16}; use crate::Error; use core::cmp; @@ -48,7 +48,7 @@ impl AttributeProvider for BatteryServiceAttrs { fn for_attrs_in_range( &mut self, range: HandleRange, - mut f: impl FnMut(&Self, &Attribute) -> Result<(), Error>, + mut f: impl FnMut(&Self, &Attribute>) -> Result<(), Error>, ) -> Result<(), Error> { let count = self.attributes.len(); let start = usize::from(range.start().as_u16() - 1); // handles start at 1, not 0 @@ -71,7 +71,7 @@ impl AttributeProvider for BatteryServiceAttrs { uuid == Uuid16(0x2800) // FIXME not characteristics? } - fn group_end(&self, handle: Handle) -> Option<&Attribute> { + fn group_end(&self, handle: Handle) -> Option<&Attribute>> { match handle.as_u16() { 0x0001 => Some(&self.attributes[2]), 0x0002 => Some(&self.attributes[2]), @@ -158,7 +158,7 @@ impl AttributeProvider for MidiServiceAttrs { fn for_attrs_in_range( &mut self, range: HandleRange, - mut f: impl FnMut(&Self, &Attribute) -> Result<(), Error>, + mut f: impl FnMut(&Self, &Attribute>) -> Result<(), Error>, ) -> Result<(), Error> { let count = self.attributes.len(); let start = usize::from(range.start().as_u16() - 1); // handles start at 1, not 0 @@ -181,7 +181,7 @@ impl AttributeProvider for MidiServiceAttrs { uuid == Uuid16(0x2800) // FIXME not characteristics? } - fn group_end(&self, handle: Handle) -> Option<&Attribute> { + fn group_end(&self, handle: Handle) -> Option<&Attribute>> { match handle.as_u16() { 0x0001 => Some(&self.attributes[3]), 0x0002 => Some(&self.attributes[3]),