[Draft, for discussion] Introduce async handler for reading/writing attr data

[ivmarkov]

Disclaimer: this code only type-checks. I've not run it yet. Tests and examples are not fixed and will fail to build.
There might be logical bugs too!

But at least we have some ground for discussion which way we should go.

I'll split the elaboration of the changes into two aspects:

(Easy) Externalize attribute read/writes (and possibly other operations in future) with a user-supplied callback

This change consists of:

• Removal of the 'd lifetime from AttributeData<'d> as it no longer serves as attribute data storage (modulo CCCD data and service data), and all the way up the call chain.
• New trait, AttrHandler (let's put aside the name of this thing for a while) which serves as the user-supplied "mega-callback"
  • Since I did the trait as async trait, I had to change the attribute table's lock from blocking to async, which resulted in the propagation of async to methods up the call chain. It also slightly changed the attributes' iteration metaphor
  • If we want a blocking callback, we can revert some of these changes
• GattServer::next is renamed to GattServer::process, as GattEvent is retired. Now that we have a callback, I'm not so sure if and how much a set of "post-factum" events might be useful. If we want the user to stay informed about attributes' processing (or other stuff) which happens without her intervention, we can always extend AttrHandler with additional, "FYI" methods. Or provide a GattHandler: AttrHandler trait for GATT-specific notifications.
  • Note that GattEvent is actually restored with the following, second change described below, but more on that later

(A bit more controversial) Inversion of control

There is a new method, GattServer::split and a new module, split (let's put the module name aside for a while, I just wanted to externalize this brand-new code in a separate file for easier review) that tries to outline a processing metaphor different to the "mega callback" one. With that said, it is currently implemented on top and with the help of the "mega callback" one. And relies on the fact that the "mega callback" is async, and can await.

It implements something called GattEvents::next which does return a GattEvent instance which pretends to mutably borrow from GattEvents to enforce sequential processing of each event. The event is no longer "FYI", but contains either an attribute read, or an attribute write payload that the user has to process (not that we can enforce that). If the user just drops the GattEvent instance, 0-len data will be returned to the peer, or the incoming attribute data would not be saved in the user's attribute data storage.

It relies on something called ExchangeArea (can be implemented in many different ways), which is just a rendezvous point, where a hidden "mega callback" (ExchangeArea itself) pushes the data for an attribute read or write and awaits for the user to process it via pulling events from GattEvents.

Can this be implemented without the hidden "mega callback" and without paying the price of having ExchangeData and its extra synchronization around?
Maybe, but that might mean much bigger changes to the Gatt server.

[ivmarkov]

I've done an extra change by introducing a new, extra "mega callback" called GattHandler (name is only "for now").

The major difference between GattHandler and AttrHandler is that AttrHandler is an attribute_server thing, while GattHandler is a GattServer thing.

As such, GattHandler is aware of GATT connections, while the AttrHandler isn't. Passing to the user the information on which connection the attribute request came is - I guess - important, as the value of an attribute might be connection-specific?

(We might/should make AttrHandler a private detail if the whole notion of an attribute_server is a private thing.)

There's currently the inconvenience, that - in the inversion of control code - I have difficulties converting the &'a Connection<'a> thing that GattHandler supplies to something with a static lifetime that I can safely store in the ExchangeArea. But I also haven't looked into it too deeply for now. Perhaps just converting the Connection to connection handle, storing the handle in the ExchangeArea and then converting back to a &Connection to be passed to GattEvent is a possibility? In any case this touches a bit on the problem of handles in general:

• Do we want to expose these to the user (i.e. in the callbacks?) Bluedroid - in its "mega callback" - actually does that - it always refers to services/attributes/connections by their handles, but then it also has extra callback methods to inform the user about e.g. the attribute UUID-to-handle mapping it had created, and so on for all handles.

[alexmoon]

I like the inversion of control version. I think it's useful to model bluetooth as streams of events, since that matches well with the underlying HCI protocol. Rust also tends to work well with inverted control, since it can often make lifetimes more tractable.

I question whether GattServer and AttrServer should take exclusive references or shared references on its methods. It can be useful to process some read/write requests asynchronously without blocking other operations. For example, a write might require writing data to flash before returning a success/fail to the client which could take some time. Or a read could require gathering data from a sensor or similar. Also, it can be useful to shift processing for a read or write to a separate task (by sending the work through a channel or similar).

This may require some API changes. For example, you probably don't want to pass a &mut [u8] to the read method directly, because that (pre-allocated) buffer then needs to be alive during the entire lifetime of the future even though it's only used at the very end. In nrf_softdevice I implemented this by passing "Reply" structs in to the read/write handlers (e.g. DeferredReadReply and DeferredWriteReply). Those reply objects can be sent to other tasks to allow for fully asynchronous/cross-task handling and allow for the read handler to just pass the data back to the host as a &[u8] at the very end of the processing. You may not want to copy that mechanism exactly, but it does provide some very useful flexibility.

Similarly, in the inversion of control version, it would be nice if GattReadRequest did not capture an exclusive lifetime to GattEvents so that multiple requests can be handled concurrently. Also, in the inversion of control version there doesn't seem to be a way to report AttErrors which can be important.

[ivmarkov]

I like the inversion of control version. I think it's useful to model bluetooth as streams of events, since that matches well with the underlying HCI protocol. Rust also tends to work well with inverted control, since it can often make lifetimes more tractable.

I question whether GattServer and AttrServer should take exclusive references or shared references on its methods. It can be useful to process some read/write requests asynchronously without blocking other operations. For example, a write might require writing data to flash before returning a success/fail to the client which could take some time. Or a read could require gathering data from a sensor or similar. Also, it can be useful to shift processing for a read or write to a separate task (by sending the work through a channel or similar).

This may require some API changes. For example, you probably don't want to pass a &mut [u8] to the read method directly, because that (pre-allocated) buffer then needs to be alive during the entire lifetime of the future even though it's only used at the very end. In nrf_softdevice I implemented this by passing "Reply" structs in to the read/write handlers (e.g. DeferredReadReply and DeferredWriteReply). Those reply objects can be sent to other tasks to allow for fully asynchronous/cross-task handling and allow for the read handler to just pass the data back to the host as a &[u8] at the very end of the processing. You may not want to copy that mechanism exactly, but it does provide some very useful flexibility.

It might require very deep changes to the GattServer and AttrServer code actually.

For one, I don't see how such an approach can be layered "on top" of the "mega callback" idea, which - for one - uses an owned singleton (ExchangeArea) to achieve the inversion of control. Obviously, if we are to support multiple GattEvents "in action", this singleton has to go, along with the callback. Furthermore, all handle_ methods in AttrServer need to be possibly unrolled, as they invoke the attribute read/write callback while closing over the attributes' table and keeping a lock on it.

Similarly, in the inversion of control version, it would be nice if GattReadRequest did not capture an exclusive lifetime to GattEvents so that multiple requests can be handled concurrently.

Ditto, as per above.
Perhaps the deficiency of the current proposal is that it tries to do two usability improvements simultaneously (with one layered above the other), also with minimal changes, but then the price to pay is stalling the processing pipeline.

Maybe we can just choose an extreme?

• Extreme 1: Keep only the mega-callback approach, and make the callback blocking. This way the pipeline will not get stalled because the user cannot await in the callback, but this also limits the usefulness of the callback. But minimal changes.

• Extreme 2: Keep only the "inversion of control" approach, but only offer it without the attribute table. And without the attribute server. It would then be the responsibility of the user to do everything the attribute table does and attribute server does. I.e. users will have to deal with u16 handles from the PDU payload, and figure out - with the help of a "utility" attribute table - what attribute we are talking about - see below

  • Perhaps, the attribute table can be kept as a "utility" of sorts (modulo data storage) and the user might take advantage of (for e.g. checking attr permissions), but the GattEvents would just be wrappers around Pdus, plus some of the response-encoding utilities using WriteCursor which are currently in the attribute table/attribute server/gatt server
  • Since multiple Pdus can be "in-flight" (right?), this would open the possibility for processing GattEvents concurrently

• Extreme 3: Stick with the current approach (which is - in a way "an inversion of control" already) and only (somehow) solve the pesky problem where the storage of the "attribute table" is mutably borrowed into it, which creates lifetime issues (i.e. most users - including me - might just want to pre-allocate the table - along with its "data" in a static/longer-lived storage and be done with it). UPDATE: There is an on-going suggestion in this direction using proc-macros.

  • This might be advantageous, because the Matter use case of emulating a duplex bidirectional pipe on top of two attributes (which does need to "see" each incoming attr write) is a niche use case.
  • Most users might just want a global storage that just keeps certain data about their device and sends this over BLE automatically anyway?
  • Also, with some minimal extensions to the existing GattEvent I might be able to (ab)use the existing approach to cover the Matter use case. Checking this right now, might have some results till EOW.

We can also do Extreme2 + Extreme3 as they seem orthogonal to each other. Not 100% sure.

Also, in the inversion of control version there doesn't seem to be a way to report AttErrors which can be important.

I understand, but this is a small issue to solve compared with the rest, I hope.

[petekubiak]

I believe this is aligned with what we're hoping to achieve building on top of #131. For the purposes of that PR we wanted to make as little change to the host as possible, so characteristic data is allocated statically and passed to the AttributeTable.
Ultimately however the idea is to implement the nrf_softdevice model, where services are accessed as fields within the server struct, and characteristics are fields within the service structs. Characteristics would hold their own data internally, eliminating the need for static/globally allocated storage. This does away with the need for the AttributeTable and AttributeServer, as the services and characteristics are accessible through the server instead.
As for callbacks, I've been toying with the idea of both approaches - we could either keep the GattEvent mechanism but modify the logic such that the "mega-callback" is called before data is written to the characteristic, or alternatively there could be a mechanism to register a callback on a characteristic (perhaps passed as part of the attribute arguments). This would then be called when an event on a particular callback was triggered.

I'm going to try to put together a POC for this - maybe we can look at these two initiatives side-by-side so we can make sure we're aligned on direction?

[alexmoon]

I think there are a few general options for the API:

First, a synchronous callback that uses a reply object that can be sent to another task for asynchronous handling:

fn on_read(&mut self, conn: Connection, attr: Handle, reply: ReadReply);
fn on_write(&mut self, conn: Connection, attr: Handle, data: &[u8], reply: WriteReply);

Where ReadReply has a method like:

fn reply(self, result: Result<&[u8], AttError>);

This is what nrf_softdevice currently does.

Second, an asynchronous callback:

async fn on_read(&self, conn: Connection, attr: Handle, writer: W) -> Result<(), AttError>;
async fn on_write(&self, conn: Connection, attr: Handle, data: &[u8]) -> Result<(), AttError>;

writer could be sent to another task if needed with on_read waiting on a one-shot channel for the result.

The main issues I see with this version are that &mut self would prevent other events from being handled while one operation is in progress, so it would need to be &self. Secondly, that buffers like data and possibly a buffer in writer would have their lifetime's captured by the future, which may make things difficult.

Third, an asynchronous callback but with an API like the synchronous callbacks:

async fn on_read(&mut self, conn: Connection, attr: Handle, reply: ReadReply);
async fn on_write(&mut self, conn: Connection, attr: Handle, data: &[u8], reply: WriteReply);

The advantage of this would be if you just want to do some simple asynchronous logic (like writing to flash or something) and don't care about handling events concurrently you could do it inline. However if you want concurrency, you could send the reply object to another task and allow the buffers and &mut self lifetime to go out of scope.

Fourth, an asynchronous iterator/inversion of control/event based API:

async fn next(&mut self) -> GattEvent;

enum GattEvent<'a> {
   Read(ReadEvent)
   Write(WriteEvent<'a>)
}

impl ReadEvent {
    fn reply(self, result: Result<&[u8], AttError>);
}

struct WriteEvent<'a> {
   data: &'a [u8]
   reply: WriteReply
}

impl<'a> WriteEvent<'a> {
  fn into_reply(self) -> WriteReply;
}

Converting a WriteEvent into a WriteReply drops the data buffer and so drops the borrow on self from next(). That allows for concurrent processing of events to proceed until the write is finished and WriteReply::reply is called.

I think that any of those options could work, though I would tend to prefer either the third or fourth. I don't think there's a need to support both callback and inversion of control APIs, I think it would be better to pick one and stick to it.

[lulf]

Thanks for putting a lot of good ideas on the table everyone. My preference are the inversion of control APIs, as I think they are a better/natural fit for async. I don't have any strong opinions on how it's implemented or which exactly which of those variants are chosen, but I would say all options are on the table, and that changing existing APIs are completely fine at this stage.

I think the API should be usable without relying on macros, but the option of using macros to simplify the lifetime and generics (i.e. by having macros always using statics and some generic defaults) of the types.