Writeable attributes & attribute permissions

[daboross]

I realize the intended attribute interface is very much incomplete (#72, #29), but I've been using the current one, and finding it pretty usable (at least for generic code which doesn't deal with particular attributes).

It currently only supports readable attributes, though, and additionally doesn't seem to have any ways to control attribute permissions. It'd be nice to add minimal support for writeable attributes and write permissions within the current layer, if that's possible.

As a draft for the user interface, what would you think of this?

pub enum AttributeAccessPermissions {
    Readable,
    Writeable,
    ReadableAndWritable,
}

pub trait AttributeProvider {
    // ...

    /// Retrieves the permissions for the given attribute.
    ///
    /// These are used purely for access control within rubble, and won't be
    /// communicated with clients. They should be coordinated beforehand as part
    /// of a larger protocol.
    ///
    /// Defaults to read-only. If this is overwritten, `write_attribute` should
    /// be overwritten.
    fn attribute_access_permissions(&self, uuid: AttUuid) -> AttributeAccessPermissions {
        AttributeAccessPermissions::Readable
    }

    /// Attempts to write data to the given attribute.
    ///
    /// This will only be called on UUIDs for which
    /// `attribute_access_permissions` returns
    /// [`AttributeAccessPermissions::Writeable`] or [`AttributeAccessPermission::ReadableAndWriteable`].
    ///
    /// By default, panics on all writes. This should be overwritten if
    /// `attribute_access_permissions` is.
    fn write_attribute(
        &mut self,
        uuid: AttUuid,
        data: &[u8]
    ) -> Result<(), Error> {
        unimplemented!("by default, no attributes should have write access permissions, and this should never be called");
    }
}

Alternatively, this could introduce an AttributePermissions struct with a single access field and a single method attribute_permissions for retrieving it, with the intention to extend that struct when adding more permission support, rather than adding new methods for each type of permission.

I think some kind of explicit permissions interface would be good, though, since I think we need it, or something like it, to support prepared write requests querying the permissions for errors without writing anything.

For write_attribute, since we do actually have the data in an &[u8], it seems like this simple interface should suffice. But if more flexibility was needed, I think something like this could also make sense:

    /// Attempts to write data to the given attribute.
    ///
    /// This will be called with a callback, so that the `AttributeProvider` can
    /// a buffer to write data into. The function will return the number of
    /// bytes written, or an error if an error occurred parsing the remainder of
    /// the packet. The implementor of `write_attribute` is expected to relay
    /// this error.
    ///
    /// `uuid` is the uuid of packet to write to, and `length` is the length of
    /// the packet data.
    ///
    /// If the buffer is too short for the packet, the attribute value will be
    /// truncated when writing.
    fn write_attribute(
        &mut self,
        uuid: AttUuid,
        length: usize,
        f: impl FnOnce(&mut [u8]) -> Result<usize, Error>,
    ) -> Result<(), Error>;

And as a last question, is the attribute provider even the right thing to be attaching this to? I think it kind of makes sense from a semantic point of view, but I'm not seeing the full picture here, especially with regards to future higher-level interfaces.

Thoughts on this interface, and the general viability of adding this? If it's reasonable, then I'm happy to work on a PR adding support.

[jonas-schievink]

This sounds like it makes sense, but it's been a while since I thought about the (G)ATT interface. Maybe try it out and if this turns out usable, open a PR?

[daboross]

Sounds good! I've got a few things to finish first before implementing this, but at that point I'll start out writing the implementation and see how it goes.

[noloerino]

Any updates on this? I needed writeable attributes on a BLE server for a university course project and I managed to hack something on top of a fork with minimal changes, but I could definitely try to build out something akin to the above API.

One thought I had that I'm curious about: instead of forcing the user to implement write_attribute method iff the permission is set to Writeable, why not just encode it as part of the enum? Perhaps something like this:

pub enum AttributeAccessPermissions {
    Readable,
    // This structure forces any writeable property to implement data length/callback while
    // making sure a read-only property would never do so
    Writeable {
        data_length: usize,
        callback: impl FnOnce(&mut [u8]) -> Result<usize, Error>,
    },
    ReadableAndWritable {
        // ..
        // same as above
    },
}

pub trait AttributeProvider {
    // ...
    fn attribute_access_permissions(&self, uuid: AttUuid) -> AttributeAccessPermissions {
        AttributeAccessPermissions::Readable
    }
}

[daboross]

Hi!

No updates from me, unfortunately. I was working on this a bit after writing the initial issue, but I'm no longer directly working on the project which required this, and I've since run into some things in my personal life which have led to much less freeform hacking on things :).

As for having it as a method vs. using a callback, I think the main advantage of the method is that it can then use any data stored in the AttributeProvider directly. For instance, consider a read-write property which just stores the last value written. With the write_attribute method, the data could simply be stored within the AttributeProvider. Changing it to a callback which doesn't directly own or borrow the AttributeProvider would require either using global state to store the value, or some other kind of shared ownership with interior mutability.

I mean, all of this is probably going in global storage eventually, but I find it nicer to keep data used by structs local within them when possible? It lets us take much more advantage of rust's safety features, and prevents concurrency bugs.

Regardless, feel free to work on this! I'm happy to discuss anything, though I won't be writing any code in the near future.

As a last note, apologies for dropping this without writing anything. I do kind of hope to work on my contributions more in the future, but I can't say when I'll be in a place to do that.

[daboross]

Just double checked, and if you're interested, I do have some WIP code implementing this at https://github.com/daboross/rubble/tree/writeable-connection-attributes. It's been forever since I've looked at that, though, and I have no idea how far along it is or what needs to be done. Feel free to use as a base for this if you want, or completely ignore.

Note: accidentally hit "comment and close", my apologies. Thumbs on mobile.

[noloerino]

Sure, I'd be happy to do some work on this if I can find the time. Honestly, I'm not sure what more beyond your WIP would need to be added either (maybe just messing around with it to see usability), but I'll see what I can do.

Hope everything's ok with you!

[daboross]

Thanks. That sounds good to me!

If it seems relatively complete, maybe the only thing left was to actually test it, and submit the PR 😅

[noloerino]

What's the best way to go about testing this? I don't see any existing tests for the att/gatt modules (I imagine because the API surface is still changing) or any mention in CONTRIBUTING.md. I can put together a demo of writeable attributes (which would double as both a test and an example), but I don't have a UART/USB adapter to test with logging (the hardware I have access to does have a JTAG connection so I can use RTT).

Also, is it all right to leave PrepareWriteReq/ExecuteWriteReq unimplemented for now? My understanding is that prepare write puts a chunk of a write into a queue to then be atomically executed upon seeing an execute command; since ReadBlob/ReadMultiple aren't yet implemented either it makes sense to me to hold off on the grouped writes as well. Keeping track of this also sounds a bit hairy, since per spec every client should get its own queue, and it seems a bit hard to figure out the correct place to store this intermediate state.

Sorry for all the questions! I'm still fairly new to BLE, and I'd like to make sure I'm doing things right.

[noloerino]

I've got a (nearly) working demo up here: https://github.com/noloerino/rubble/blob/writeable-connection-attributes/demos/nrf52-writeable-attributes/src/main.rs

One issue I've run into and am not sure how to reconcile is how to get writes to update the value in an attribute within the write_attribute function. If the attributes of the provider are 'static, then since HexSlice takes in a reference to data, the data passed to HexSlice::set_value must also have 'static lifetime. Since the AttributeProvider trait doesn't come with a lifetime variable, there doesn't seem to be a clean way to enforce this lifetime constraint without poisoning a lot of other structs with lifetimes as well. Forcing the data argument of write_attribute to be 'static runs into a similar problem (we probably can't guarantee that anyway).

My feeling is that it makes more sense for an Attribute to own its value, although this runs into other difficulties (it no longer becomes possible for a read-only attribute to have its value be a reference to an array stored elsewhere). Perhaps something akin to a Cow might come in handy, although we'd probably have to implement our own enum (perhaps distinguishing between readable, which can have its value be a reference to other data; and readable | writeable, which must own its value). Either way, I'm wondering if there's some sort of obvious solution that I'm missing that requires a less drastic change.

[daboross]

@noloerino Sorry it took me a while to respond to this! The demo looks good, though yeah, it has the problem you've described.

The way I'd solve this as a rust programmer would be to never stores Attributes inside of LedBlinkAttrs, and instead create them on-demand in group_end and for_attrs_in_range. That way, you can actually own the data, and the lifetime the Attributes reference can be the lifetime of LedBlinkAttrs itself and the data it stores.

I think that's the way that this API is intended to be used as well, since for_attrs_in_range looks specifically designed to never require exposing a &[Attribute] slice.

Maybe we could add an OwnedAttribute type to make this pattern easier to use - but even without that, it should be possible to implement the interface right now just using a custom owned data type which is convertible into Attribute.

Thoughts on using the API this way - would that work for your use case?

[daboross]

I didn't notice this when I wrote the above comment, but #166 might also help significantly with this! With that, it should be possible to actually have owned attribute values, and then the demo should just work.

[jonas-schievink]

I think that's the way that this API is intended to be used as well, since for_attrs_in_range looks specifically designed to never require exposing a &[Attribute] slice.

Yeah, that was the idea. I wanted to support lazily producing Attributes, for example from compressed data in flash.

[noloerino]

Oh, that makes sense. I'll see if I can make things work with this in mind, thanks for the feedback.

Thoughts on using the API this way - would that work for your use case?

I actually ended up switching to C for the course project because we were provided a fairly robust C framework, and we were running into really weird timing/power issues with Rust. I also had to return the hardware (including the NRF board) back to the university since the semester ended, but I'll see if I can purchase a dev kit board for myself. It might take a while before I can get it and test functionality, although I can certainly try some compiler-driven development on this in the meantime.