embassy-rs / nrf-softdevice

Apache License 2.0
256 stars 76 forks source link

Add report descriptors to gatt_service macro #164

Closed hkjolhede closed 1 year ago

hkjolhede commented 1 year ago

This PR adds syntax for declaring report descriptors in the gatt_service macro. Three additional keywords have been added to the characteristics to be able to declare a characteristic as an input, output or feature report, and which report id it has in the report map. This is required by the HIDS protocol.

In this example of a HID Service there are three input reports declared, which correspond to the reports in the report map (not shown here).

#[nrf_softdevice::gatt_service(uuid = "1812")]
pub struct HIDService {
    #[characteristic(uuid = "2a4e", read, write_without_response)]
    pub protocol_mode: ProtocolMode,

    #[characteristic(uuid = "2a4b", read)]
    pub report_map: ReportMap,

    #[characteristic(uuid = "2a4d", read, notify, input = 3)]
    pub mouse_input_report: MouseReport,

    #[characteristic(uuid ="2a4d", read, notify, input = 1)]
    pub keyboard_input_report: KeyboardReport,

    #[characteristic(uuid ="2a4d", read, notify, input = 2)]
    pub consumer_input_report: ConsumerReport,

    #[characteristic(uuid = "2a33", read, notify)]
    pub boot_mouse_input_report: MouseReport,

    #[characteristic(uuid = "2a22", read, notify)]
    pub boot_keyboard_input_report: KeyboardReport,

    #[characteristic(uuid = "2a4a", read)]
    pub hid_information: HIDInformation,

    #[characteristic(uuid = "2a4c", write_without_response)]
    hid_control: HIDControlPoint,
}
alexmoon commented 1 year ago

Thanks for the PR, however I'm not sure it makes sense to add functionality to the gatt macro that is specific to only a single service type. I'd be open to a change that allowed a more general way of attaching descriptors to characteristics. At the moment the intention is that for situations that are more complex than a simple GATT service one should use the Builder API in your application directly rather than relying on the macro.

hkjolhede commented 1 year ago

Yes, it should probably be made more generic? My main gripe with the builder api is that it is very verbose and mainly consists of lots of boilerplate. What do you think of the following syntax?

 #[characteristic(uuid ="2a4d", read, notify, descriptor = ReportDescriptor(Input, 1)]
pub keyboard_input_report: KeyboardReport,

where ReportDescriptor would be a class implementing the CharacteristicDescriptor (or some such name) trait. Other implementations could be made for other types of descriptors. Each descriptor implementation would contain a function creating the descriptor data and a constant of the UUID.

alexmoon commented 1 year ago

That’s better, but it’s possible for characteristics to have multiple descriptors, and those descriptors could have other settings. What about using a separate attribute for descriptors: #[descriptor …]?

BTW, for my HID device implementation I’m using the builder API (in fact it was the motivation to create that API). It is more verbose, but that’s the price you pay to have access to all of the softdevice options. I ended up needing a lot more than just the descriptors for my application, and you may end up finding the same.

hkjolhede commented 1 year ago

Yes, perhaps not worth the effort right now. I am not sufficiently well versed in the different BLE services that I can predict all scenarios. I'll just close the PR now. If someone thinks it would be worth developing further and have better insights I am open to helping with the effort.