Improve characteristic interactions

[petekubiak]

This work has evolved considerably from the initial proposal I gave in #136 as my understanding of the code base has grown.

The scope of this PR is to make use of the new macro-style declaration of services and characteristics to improve the server's API. This has led to the following changes:

• Characteristics now hold information on their data type. This is used to infer types for the get(), set() and notify() methods on the server.
• The GattEvent generic callback structure has been changed such that it contains the characteristic's handle rather than the Characteristic type itself. This is due to the Characteristic type holding a generic as mentioned above
• To compensate for this, optional on_read and on_write callbacks have been implemented for attributes. These will be automatically called when a read or write event occurs on the attribute and have the following signatures:

fn on_read(connection: &Connection) {}

fn on_write(connection: &Connection, data: &[u8]) -> Result<(), ()> {}  // Where data is the data being written to the attribute

[petekubiak]

I have another approach which would be to implement the characteristic functions on the server directly rather than on the services. This circumvents all of the issues with referencing and ambiguous types, but has its own set of compromises:

• The gatt_server and gatt_service macros would have to change. There would instead need to be a single macro in which all services and characteristics were declared. This would mean that services could no longer be defined in separate files alongside the functionality associated with them.
• The characteristic functions would no longer be grouped by service. This can be somewhat mitigated by prefixing the characteristic function with the service name (this also eliminates the issue of characteristics with the same name). So rather than server.service.characteristic_get() you would have server.service_characteristic_get().

[lulf]

This works for the _get() and _set() functions, but there is an issue with the _notify() function in that the return type is dependent upon the Error type of the Controller used by the GattServer. I've been unsuccessful in finding a way to express this type in the GattServerInterface trait and the service's _notify() functions which doesn't result in an "ambiguous types" or "mismatched types" error.

Might be worth change the controller-specific error into a different type based on ErrorType::kind() (impemented by Controller traits), so that you can avoid the generics?

In practice, what benefits does this approach give vs. the existing? From the example, it seem like it would change

server.notify(server.battery_service.level, &conn, &[tick]).await;

to

server.battery_service.level_notify(server, &conn, &tick).await;

Personally I'm not sure why this is better, but maybe there are better examples to illustrate the advantage of this change.

I would also like to say, that if you need to make significant changes to make the API better, please do not hesitate to do that.

[petekubiak]

Personally I'm not sure why this is better, but maybe there are better examples to illustrate the advantage of this change.

I'm inclined to agree with you. There is still the benefit that these helper functions use the characteristic's type rather than requiring the user to convert to and from byte slices.
I do feel that having to pass in a reference to the server rather muddies the interface though.

I would also like to say, that if you need to make significant changes to make the API better, please do not hesitate to do that.

I appreciate the endorsement! I'm certainly willing to do this, but I think at present I'm not sure I have a clear enough vision of how it should look.
The most obvious way to implement an interface similar to that of nrf-softdevice would be to ape the structure of nrf-softdevice: that project gets around the issue of needing a reference to the server by having it instantiated as a singleton which can then be acquired as needed in a similar way to peripherals in embedded_hal. We could go down that route, but it's such a fundamental change that I'm wary it may introduce other compromises.
I'd be interested to get @alexmoon 's input on this too, considering his work on nrf-softdevice.

[lulf]

Yeah, regarding the use of singletons, I think it should be possible to avoid that, but it does indeed require some experimentation to learn what the ideal structure is. I like the structure of the stuff below GATT at the moment, but maybe for GATT it's worth putting some additional constraints in order to provide a better API.

So maybe the best course of action is to define a few directions for experimenting.

[alexmoon]

I agree that singletons tend not to work well in Rust so should be avoided if possible.

It seems like the main virtue of this API is to allow you to use the internal type of the attribute rather than a byte array. Perhaps an alternative like:

trait AttributeHandle {
  type Value: ToGatt;
  fn raw_handle(&self) -> u16;
}

trait CharacteristicHandle: AttributeHandle {}

struct GattServer {
  ...
  async fn notify<T: CharacteristicHandle>(&self, handle: &T, value: &T::Value) -> Result<...> {
    ...
  }
}

This branch contains another approach in which I have compromised a bit on the new functions' signatures: in this version they take a reference to the macro-generated server as an argument, which circumvents the issue of needing to hold references on the services. The macro-generated server's type can't be accessed directly by the code generated by the gatt_service macro, so instead I have created a GattServerInterface trait for this purpose.

What if instead the struct generated by the gatt_server macro impls AsRef<trouble::GattServer>? Then the service methods could be generic on the AsRef<trouble::GattServer> bound and you wouldn't have to introduce a new trait.

[petekubiak]

I agree that singletons tend not to work well in Rust so should be avoided if possible.

It seems like the main virtue of this API is to allow you to use the internal type of the attribute rather than a byte array. Perhaps an alternative like:

trait AttributeHandle {
  type Value: ToGatt;
  fn raw_handle(&self) -> u16;
}

trait CharacteristicHandle: AttributeHandle {}

struct GattServer {
  ...
  async fn notify<T: CharacteristicHandle>(&self, handle: &T, value: &T::Value) -> Result<...> {
    ...
  }
}

What if instead the struct generated by the gatt_server macro impls AsRef<trouble::GattServer>? Then the service methods could be generic on the AsRef<trouble::GattServer> bound and you wouldn't have to introduce a new trait.

I like both of these suggestions, I think I'll use this PR to bring in the GattValue traits and extend the existing server.notify function to accept generics.

[petekubiak]

It also feels like on_read and on_write give you the same thing as processing events with server.next(), except them being non-async. Maybe we can find a way to unify it in the future?

The main difference between the callbacks and server.next() is that the callbacks trigger before the event is processed. Hopefully the new signature of the write callback allowing the application code a say in accepting / rejecting the request helps to differentiate it and give it more of a unique purpose too.

I did wonder whether there were any other parameters we might want to provide to the callbacks to increase their usefulness. If anyone has any ideas I'd be keen to hear them.

I did originally try to implement them as async, but I can't use the &impl Future<Output=T> return type for a function pointer, so would have had to resort to Box to be able to make them async. If there's a way to unify things though I'd certainly be keen for it!

[petekubiak]

Last but not least, I really like the GattServer::set|get with the GattValue, but the GattValue is not used during writes. I think it should be used to parse/verify on write. We can still store the bytes representation in the attribute table (otherwise we will probably get a mess which would need Box<dyn GattValue> or something), but this way, we allow user defined formats and much more customization.

Just a quick update to say that I tried this but &impl Trait is unfortunately not allowed in function pointers, so it is not possible to implement the write callback to take the type expected for the callback's attribute.
As I mentioned earlier though, you can still convert the byte slice into the correct data type within your callback using the <type>::from_gatt() function.
Please note that from_gatt() now returns a Result type, the variant of which is dependent upon length-checking of the slice and any other fallible operations which may be required to complete the conversion.
(I've just realised there's still a panic in the implementation of FixedGattValue for T: Primitive. I'll remove this tomorrow!)

[petekubiak]

/test

[lulf]

/test

Agent is back up now.

[petekubiak]

/test

[petekubiak]

/test

[petekubiak]

/test

[petekubiak]

/test

[jamessizeland]

this assert should probably be removed now we're returning a result. We can bubble this error check up.

[petekubiak]

394cba9

[jamessizeland]

this unwrap can be handled

[petekubiak]

9ff71f1

[petekubiak]

/test

[Erik1000]

@petekubiak I just switched my application to your branch! One thing I noticed is that GattServer::notify takes handle: Characteristic<T> but I think only a &Charateristic<T> is required for self.server.table.set(&handle, value)?;. Can we change that? Because it is annoying if the type T does not implement Copy and avoids potentially expensive cloning.

[petekubiak]

@petekubiak I just switched my application to your branch! One thing I noticed is that GattServer::notify takes handle: Characteristic<T> but I think only a &Charateristic<T> is required for self.server.table.set(&handle, value)?;. Can we change that? Because it is annoying if the type T does not implement Copy and avoids potentially expensive cloning.

Thanks for this, real-world testing is always appreciated! Yes, seems like that change can be made without changing anything else in the code - good spot!
adb1c9b

[petekubiak]

/test

[petekubiak]

It also feels like on_read and on_write give you the same thing as processing events with server.next(), except them being non-async. Maybe we can find a way to unify it in the future?

I do think that being able to use async in the callbacks would provide huge benefit. I was talking to @jamessizeland about this yesterday and we realised that heapless::Box exists, which would enable us to store function pointers with a Future return type.
This may incur a performance penalty due to the memory management and it requires setting up a memory pool for heapless::Box to use, so I'm thinking it would be an optional feature.
I'll create an issue for it.

[lulf]

If you require async, you could also use the server.next() and handle it there. I think with Box you'd anyway end up needing to store the value first for the future to reach it.

I think some kind of async iterator API is more powerful than the callbacks and more natural to async, so if we can get some way to poll characteristics for events or the like, maybe that could be an alternative.