embassy-rs / nrf-softdevice

Apache License 2.0
269 stars 80 forks source link

GATT Server CPFD #225

Closed AliMoal closed 10 months ago

AliMoal commented 10 months ago

According to the issue (#222), which tells Softdevice reserved CPFD's UUID, this PR is providing Characteristic Presentation Format Descriptor (CPFD) based on SoftDevice API and BLE documentation. Examples also are updated according to new APIs. Please merge this PR. Thanks.

alexmoon commented 10 months ago

Thanks! Looks pretty good. I would like to see a minor change to the API: instead of adding the presentation format as an optional parameter to the Metadata struct, let's use a builder-style API (like we use for Properties).

So new would just take properties and set the presentation format to None.

Then there would be a method called presentation(self, presentation: Presentation) -> Self. You should also change the with_security constructor to a similar builder method security(self, security: Security) -> Self.

That way users only have to deal with the optional pieces that they care about and you would create the metadata with a call like Metadata::new(my_properites).presentation(my_presentation).security(my_security);

AliMoal commented 10 months ago

Thanks! Looks pretty good. I would like to see a minor change to the API: instead of adding the presentation format as an optional parameter to the Metadata struct, let's use a builder-style API (like we use for Properties).

So new would just take properties and set the presentation format to None.

Then there would be a method called presentation(self, presentation: Presentation) -> Self. You should also change the with_security constructor to a similar builder method security(self, security: Security) -> Self.

That way users only have to deal with the optional pieces that they care about and you would create the metadata with a call like Metadata::new(my_properites).presentation(my_presentation).security(my_security);

I guess I understood what you mean. Yes, that's the better method. I will change the code to this way and let you know.

AliMoal commented 10 months ago

@alexmoon Please check the new changes. Thanks