embassy-rs / nrf-softdevice

Apache License 2.0
258 stars 79 forks source link

Support for read callbacks #123

Closed quentinmit closed 1 year ago

quentinmit commented 2 years ago

Right now AFAICT the API requires that you always "set" the current value for every characteristic when you want it to change. The softdevice also supports a callback-based API, where the softdevice asks you for a value every time someone reads an attribute.

In C, you would do something along the lines of

// In the ble_gatts_attr_md_t, set
attr_md.rd_auth = 1;

// In event handler
T mydata;
if (event->header.event_id == BLE_GATTS_EVT_RW_AUTHORIZE_REQUEST) {
  ble_gatts_evt_rw_authorize_request_t * request = &event->evt.gatts_evt.params.authorize_request;
  if (request->type == BLE_GATTS_AUTHORIZE_TYPE_READ) {
    reply.type = BLE_GATTS_AUTHORIZE_TYPE_READ;
    reply.params.read.gatt_status = BLE_GATT_STATUS_SUCCESS;
    reply.params.read.update = 1;
    reply.params.read.offset = 0;
    reply.params.read.len = sizeof(T);
    reply.params.read.p_data = (uint8_t*) &mydata;
    sd_ble_gatts_rw_authorize_reply(conn_hdl, &reply);
  }
}

Ideally this would be exposed to Rust in such a way that I can provide my own callback/event handler to return arbitrary-length data (or presumably also to return failures instead of success). I'm willing to take a stab at implementation if you can help me identify the best way to integrate it into the API. It could be slick if there was a Read trait with on_read(&self, handle) -> Result<&[u8], E>, but I'm not sure how you would handle the lifetime of the slice. Would it be sufficiently idiomatic to give the callback an FnOnce to call with the data?

peter9477 commented 2 years ago

@quentinmit I'm not quite up to this point yet, but I'll likely need to support both authorized reads and writes, and only just noticed that the current code (I think we're talking about gatt_server.rs, right?) supports only non-authorized reads and writes. I'm following the matrix chat (link in header at https://embassy.dev/) in case you want to discuss it further there with someone who is only just coming up to speed on embassy/Rust but has a few years of experience with the SoftDevice stuff.

alexmoon commented 2 years ago

The new Builder API allows you to build a service with authorized reads and writes, but the gatt_server does not currently dispatch the events. I have an implementation for that in my product, but I haven't yet had a chance to whip it into shape for a PR yet.

alexmoon commented 2 years ago

Support for the events required for deferrred reads and writes is implemented in #137. Feel free to have a look and let me know if there are any issues for your use case.

peter9477 commented 1 year ago

I've done a basic test of this and it worked fine. I had been using the gatt_server and gatt_service macros (based on the examples) and, if I understand correctly, those have no support for this, and for that matter are incompatible for several reasons. Switching to the Builder API worked out though.

I'm not sure this behaves any differently than how the Nordic SDK handles this stuff, but I'll note that even if you leave write_without_response out of the properties, you'll still get a call to on_write() if the central does a write-command instead of a write-request. That would not go to the on_deferred_write() obviously. I wonder if that should be filtered out, maybe for no other reason than that it would waste processor time calling on_write() on possibly many services and characteristics, none of which are actually going to handle it. Or maybe this is how it's meant to work, and it's just up to the programmer to ignore it?

alexmoon commented 1 year ago

Interesting. Thanks for trying it out. It should be the same as the Nordic SDK; that's just going to be how the softdevice works even if it's a bit wierd.

I've merged the PR.