Macro-configurable GATT event dispatch

[ericduvic]

Purpose

To separate concerns and avoid cluttering the GattEvent loop, it would be nice to be able to specify handler functions for characteristics in the characteristic macro itself.

Example

#[gatt_service(uuid = service::ENVIRONMENTAL_SENSING)]
pub struct TemperatureService {
    #[characteristic(uuid = characteristic::TEMPERATURE, read=get_temperature, notify)]
    pub temperature: i16,
    #[characteristic(uuid = CONTROL_TEMPERATURE_UUID, read, write=set_control_temperature)]
    pub control_temperature: i16,
}

impl TemperatureService {
    fn get_temperature() -> Result<i16, Error> {
        let result: i16 = 0;
        // Some code here to pull from an `embassy-sync` object, for example
        Ok(result) // the actual value to be returned from a `GattEvent::Read`
    }

    fn set_control_temperature(value: i16) -> Result<i16, Error> {
        // Some code here to update from an `embassy-sync` object, for example
        Ok(value) // Maybe deliver the actual value to be written to the object here if it needs to be mutated within this function
    }
}

[ivmarkov]

I think this issue is another incarnation of "hey, I don't want to store the attribute data inside the attribute table but in my own custom storage". One of the problems I had with the rs-matter BTP protocol which ended at least partially influencing the GattEvent code.

I think implementing such callbacks might make a lot of sense if we additionally:

• Make them mandatory, so that we can get rid of the variable-size-attribute-storage completely
• Make them async (because, why not, and if possible? might be an issue with &dyn)
• Will have to have - on the AttributeTable level - a generic pair of callbacks of the form "give me attribute XYZ value" and "update attribute XYZ value"
• Get rid of the attribute storage inside the attribute table.

Good side effects from this exercise:

• This would remove the 'values lifetime from AttributeTable and AttributeServer
• (Maybe?) This would remove the pesky dependency on static_cell in the proc-macro as there would be no need for the proc-macro to figure out where to store the attribute values anymore. That would be a user responsibility now. A bit of a distant complaint here.

[lulf]

Making them async would be a requirement from my side. For instance, in watchful, the Nordic nRF DFU protocol relies on an implicit flow control in the gatt server. In that particular case, each write must be written to flash, using async (because you can't do blocking flash with nrf and bluetooth).

[ivmarkov]

Making them async would be a requirement from my side.

That might be slightly problematic yet doable, because &dyn Trait cannot have async methods, and DynamicServer::process needs to become async, because it needs to call the callbacks of each attribute, which potentially would be async.

Or in other words, make those async might require getting rid of the &dyn DynamicServer at the cost of introducing an extra generic type for it in the containing structures.

On the other hand, introducing a generic for DynamicServer in - say - GattConnection - would mean the 'server lifetime would go out, and a new S: DynamicServer generic would come in, so net-net we would have the same amount of generics (as lifetimes are generics too). :)

[lulf]

I've been thinking about this problem, and I think it would be great to just get rid of the attribute server/attribute table. What I'm thinking right now is an API like this:

#[gatt_service(uuid = service::BATTERY)]
struct BatteryService {
    // ... whatever custom fields/context you'd like.
}

impl BatteryService {
    /// Battery Level
    #[descriptor(uuid = descriptors::VALID_RANGE, read, value = [0, 100])]
    #[descriptor(uuid = descriptors::MEASUREMENT_DESCRIPTION, name = "hello", read, value = "Battery Level")]
    #[characteristic(uuid = characteristic::BATTERY_LEVEL, read, notify]
    async fn level(&self, req: GattRequest) -> Result<(), AttResponseCode> {
            // Can only be read request
            let _offset = req.offset; // Read offset 
            req.respond(b"response").await;
    }

    #[characteristic(uuid = "408813df-5dd4-1f87-ec11-cdb001100000", read, write, notify)]
    async fn status(&self, req: GattRequest) -> Result<(), AttResponseCode> {
           // Can be both read and write.
           ...
    }
}

async fn gatt_events_task<P: PacketPool>(server: &Server<'_>, conn: &GattConnection<'_, '_, P>) -> Result<(), Error> {
    let reason = loop {
        match conn.next().await {
            GattConnectionEvent::Disconnected { reason } => break reason,
            GattConnectionEvent::Gatt { event } => match event.process(server).await { // Dispatches
                Ok(_) => {
                    info!("[gatt] server processed event successfully");
                }
                Err(_) => {
                    info!("[gatt] server processed event successfully");
                }
            },
            _ => {} // ignore other Gatt Connection Events
        }
    };
    info!("[gatt] disconnected: {:?}", reason);
    Ok(())
}

• Server implementation no longer stores data, it just dispatches to async functions
• Functions to handle service discovery and descriptors are generated by macro and called if needed.
• GattEvent::process() takes a handle to the server which is expected to do the appropriate dispatch
• Any regular characteristic must have an async fn that it points to. Just for simplicity.

Another refactor that would be nice is to get rid of the large enums for Att request/response, because it generates a lot of code that maybe unused.

Thoughts on the above, @ivmarkov, @HaoboGu ?

[ivmarkov]

I've been thinking about this problem, and I think it would be great to just get rid of the attribute server/attribute table. What I'm thinking right now is an API like this:

• Server implementation no longer stores data, it just dispatches to async functions
• Functions to handle service discovery and descriptors are generated by macro and called if needed.
• GattEvent::process() takes a handle to the server which is expected to do the appropriate dispatch
• Any regular characteristic must have an async fn that it points to. Just for simplicity.

Sounds reasonable. It would remove the need for static_cell and all storage of GATT attributes. I wonder how you would implement notify though? It does need some storage for subscribers, and a way to notify them with a new value. Where would that storage live?

Another refactor that would be nice is to get rid of the large enums for Att request/response, because it generates a lot of code that maybe unused.

Well I think we do need some way to model the ATT layer. And my wish is for that layer to stay public so that - whatever happens - one can operate on that layer as well.

If you have a clever idea how to model the ATT layer without enums, I'm all ears. :)

[HaoboGu]

Generally I like the idea, but according to the current code, I'm not very clear about how to specify the data type? Take the BatteryService as an example, if I want "level" to be a u16 instead of u8(two bytes on the air), how to do it?

And, for

    #[descriptor(uuid = descriptors::VALID_RANGE, read, value = [0, 100])]
    #[descriptor(uuid = descriptors::MEASUREMENT_DESCRIPTION, name = "hello", read, value = "Battery Level")]
    #[characteristic(uuid = characteristic::BATTERY_LEVEL, read, notify]
    async fn level(&self, req: GattRequest) -> Result<(), AttResponseCode> {
            // Can only be read request
            let _offset = req.offset; // Read offset 
            req.respond(b"response").await;
    }

I think it exposes too much details to users, the end users might not want to understand what's GattRequest and AttResponseCode, as well as the offset part. So if we do this, we can keep this for advanced users, but an easier/simpler representation is also needed imo.

[lulf]

Generally I like the idea, but according to the current code, I'm not very clear about how to specify the data type? Take the BatteryService as an example, if I want "level" to be a u16 instead of u8(two bytes on the air), how to do it?

And, for

#[descriptor(uuid = descriptors::VALID_RANGE, read, value = [0, 100])]
#[descriptor(uuid = descriptors::MEASUREMENT_DESCRIPTION, name = "hello", read, value = "Battery Level")]
#[characteristic(uuid = characteristic::BATTERY_LEVEL, read, notify]
async fn level(&self, req: GattRequest) -> Result<(), AttResponseCode> {
        // Can only be read request
        let _offset = req.offset; // Read offset 
        req.respond(b"response").await;
}

I think it exposes too much details to users, the end users might not want to understand what's GattRequest and AttResponseCode, as well as the offset part. So if we do this, we can keep this for advanced users, but an easier/simpler representation is also needed imo.

I didn't think too much about this part yet, but I think it can be fixed independently. One suggestion is to use the desired type as a generic parameter to the request type (Maybe a better type name would actually be 'ReadCharacteristic<T>')

     #[descriptor(uuid = descriptors::VALID_RANGE, read, value = [0, 100])]
     #[descriptor(uuid = descriptors::MEASUREMENT_DESCRIPTION, name = "hello", read, value = "Battery Level")]
     #[characteristic(uuid = characteristic::BATTERY_LEVEL, read, notify]
     async fn level(&self, req: GattRequest<u8>) -> Result<(), AttResponseCode> {
             req.respond(1).await;
     }

Regarding the response code, I think you're right it's maybe a bit too low level. But probably solvable by introducing some more sufficiently generic error type that can translate to the AttResponseCode.

Regarding the offset, maybe it can be exposed a bit more elegantly, but since att read requests may have an offset, it should at least be available in the API (in this case, it would be unused, but if you hold a large byte slice, you need to handle this case as a consumer of the API).

[ivmarkov]

I didn't think too much about this part yet, but I think it can be fixed independently. One suggestion is to use the desired type as a generic parameter to the request type (Maybe a better type name would actually be 'ReadCharacteristic')

 async fn level(&self, req: GattRequest<u8>) -> Result<(), AttResponseCode> {

Perhaps let's see how it actually pans out when we have a concrete PR?

For one, I would very much like to have the basic &[u8] type also supported somehow. That would be quite useful if the GATT layer is (ab)used to form a bi-directional streaming pipe between two peers (the Matter BTP protocol case; EDIT: and not only; one more proprietary use case I can think of but can't reveal).

In that case, the attribute might have a "variable, unfixed" length, as it is simply not something which "persists its data".

[lulf]

I've been thinking about this problem, and I think it would be great to just get rid of the attribute server/attribute table. What I'm thinking right now is an API like this:

• Server implementation no longer stores data, it just dispatches to async functions
• Functions to handle service discovery and descriptors are generated by macro and called if needed.
• GattEvent::process() takes a handle to the server which is expected to do the appropriate dispatch
• Any regular characteristic must have an async fn that it points to. Just for simplicity.

Sounds reasonable. It would remove the need for static_cell and all storage of GATT attributes. I wonder how you would implement notify though? It does need some storage for subscribers, and a way to notify them with a new value. Where would that storage live?

That's a good observation. I think it could be some kind of per-connection state object either provided when creating the GattConnection or when calling the process().

Another refactor that would be nice is to get rid of the large enums for Att request/response, because it generates a lot of code that maybe unused.

Well I think we do need some way to model the ATT layer. And my wish is for that layer to stay public so that - whatever happens - one can operate on that layer as well.

If you have a clever idea how to model the ATT layer without enums, I'm all ears. :)

I'm thinking about a similar approach to bt-hci event refactor done recently. Introduce an AttReqKind (the request code as today), and AttReqPacket which contains AttKind and a byte slice of the request data. And the consumer of the API is responsible for doing

 match kind {
    AttKind::ReadReq => {
        let req = AttReadReq::from_bytes(data);
    }

This allows using the _ => {} for skipping requests that's not supported, thereby avoiding having the deserialization code for those variants.

It's not a great API, I'm not sure it can be made more fail-safe.

[lulf]

I didn't think too much about this part yet, but I think it can be fixed independently. One suggestion is to use the desired type as a generic parameter to the request type (Maybe a better type name would actually be 'ReadCharacteristic')

 async fn level(&self, req: GattRequest<u8>) -> Result<(), AttResponseCode> {

Perhaps let's see how it actually pans out when we have a concrete PR?

For one, I would very much like to have the basic &[u8] type also supported somehow. That would be quite useful if the GATT layer is (ab)used to form a bi-directional streaming pipe between two peers (the Matter BTP protocol case; EDIT: and not only; one more proprietary use case I can think of but can't reveal).

Yes, you could use &[u8] as the type param with this approach as well I think. If you want a stream, please consider l2cap connection oriented channels if you can though, it's much more efficient.

[ivmarkov]

I've been thinking about this problem, and I think it would be great to just get rid of the attribute server/attribute table. What I'm thinking right now is an API like this:

• Server implementation no longer stores data, it just dispatches to async functions
• Functions to handle service discovery and descriptors are generated by macro and called if needed.
• GattEvent::process() takes a handle to the server which is expected to do the appropriate dispatch
• Any regular characteristic must have an async fn that it points to. Just for simplicity.

Sounds reasonable. It would remove the need for static_cell and all storage of GATT attributes. I wonder how you would implement notify though? It does need some storage for subscribers, and a way to notify them with a new value. Where would that storage live?

That's a good observation. I think it could be some kind of per-connection state object either provided when creating the GattConnection or when calling the process().

Make sense. As long as this storage is somehow "injected by the user", and is also explicitly allocated by the user and not hidden in some sort of static_cell generated by the proc-macro, I think that would be ideal.

Another refactor that would be nice is to get rid of the large enums for Att request/response, because it generates a lot of code that maybe unused.

Well I think we do need some way to model the ATT layer. And my wish is for that layer to stay public so that - whatever happens - one can operate on that layer as well. If you have a clever idea how to model the ATT layer without enums, I'm all ears. :)

I'm thinking about a similar approach to bt-hci event refactor done recently. Introduce an AttReqKind (the request code as today), and AttReqPacket which contains AttKind and a byte slice of the request data. And the consumer of the API is responsible for doing

 match kind {
    AttKind::ReadReq => {
        let req = AttReadReq::from_bytes(data);
    }

This allows using the _ => {} for skipping requests that's not supported, thereby avoiding having the deserialization code for those variants.

As long as we still provide the escape hatch for the user so that she can operate/process concrete requests at the ATT layer, I'm not too opinionated about the "beauty" of this API.

It would anyway be used rarely outside trouble-host but I think we need to have it anyway and just in case.

[ivmarkov]

Yes, you could use &[u8] as the type param with this approach as well I think. If you want a stream, please consider l2cap connection oriented channels if you can though, it's much more efficient.

Don't have a choice, at least for the Matter case.

Matter BTP really does want GATT specifically, with pre-defined (and registered) Matter Service GATT UUID, and pre-defined C1 and C2 characteristics with their UUIDs, where each characteristic needs to have well defined access patterns too - as in one should support confirmed writes, and the other - ~~notifications~~ indications.

Performance is not a problem - this is used during initial commissioning only, and is fast enough according to my observations.

EDIT: I haven't looked at l2cap in details, but I suspect it has no or primitive congestion control capabilities. Layering the streaming pipe on top of GATT confirmed writes and indications allows for an easy congestion control. As in, the (more powerful) peer will not send the next confirmed write, until after silly little MCU-based device did confirm that it had successfully processed the current write - with an ACK. Ditto for indications, for the other peer.

[lulf]

Yes, you could use &[u8] as the type param with this approach as well I think. If you want a stream, please consider l2cap connection oriented channels if you can though, it's much more efficient.

Don't have a choice, at least for the Matter case.

Matter BTP really does want GATT specifically, with pre-defined (and registered) Matter Service GATT UUID, and pre-defined C1 and C2 characteristics with their UUIDs, where each characteristic needs to have well defined access patterns too - as in one should support confirmed writes, and the other - ~notifications~ indications.

Performance is not a problem - this is used during initial commissioning only, and is fast enough according to my observations.

Yes, l2cap is generally only if you control both ends or have a spec that demands otherwise.

EDIT: I haven't looked at l2cap in details, but I suspect it has no or primitive congestion control capabilities. Layering the streaming pipe on top of GATT confirmed writes and indications allows for an easy congestion control. As in, the (more powerful) peer will not send the next confirmed write, until after silly little MCU-based device did confirm that it had successfully processed the current write - with an ACK. Ditto for indications, for the other peer.

Not relevant, but just to mention, that l2cap connection oriented channels does have good flow control capabilities via a credit grant/flow mechanism, which allows good control over flow.