Basic writeable connection attributes + attribute permissions #171
Conversation
| @@ -197,10 +203,17 @@ impl<A: AttributeProvider> AttributeServer<A> { | |||
| self.attrs.for_attrs_in_range( | |||
| HandleRange::new(*handle, *handle), | |||
| |_provider, attr| { | |||
| let value = if writer.space_left() < attr.value.as_slice().len() { | |||
| &attr.value.as_slice()[..writer.space_left()] | |||
| // FIXME return if attribute is not readable | |||
There was a problem hiding this comment.
This callback can't return the proper error type with the current API design due (see comment).
|
This looks pretty good so far, thanks! One think I'd like to see is to get the new demo merged into the old one. That way there's less code to deal with, and it should be fine to have both writeable and readable attributes in that demo. |
|
Sure thing. Should I include both the LED and the battery service attributes? |
|
Yeah that should be fine |
rubble/src/att/server.rs
Outdated
| AttPdu::WriteCommand { handle, value } => { | ||
| // WriteCommand shouldn't respond to the client even on failure | ||
| if self.attrs.attr_access_permissions(*handle).is_writeable() { | ||
| self.attrs.write_attr(*handle, value.as_ref()).unwrap(); |
There was a problem hiding this comment.
I think crashing is not the right course of action here. Can you instead log any error via log::error! and continue?
There was a problem hiding this comment.
Fixed, and did the same for handling write requests as well. I noticed that in the original code, errors in handling ReadReq and ExchangeMtuReq are also unwrapped - should I change that as well while I'm at it?
There was a problem hiding this comment.
Not necessary for this PR, but feel free to file a followup PR if you want. I'll go ahead and merge this now, thanks!
This PR adds basic support for the GATT
WriteReq(write w/ response) andWriteCommand(write w/o response) commands, permissions on attributes, and a demo that exposes a writeable attribute that toggles an LED. See also #146.I tested the demo on a NRF52832DK board, and it seems to work as expected. No rush for reviewing this by the way; I don't need this feature any time soon, I just happened to have some spare time over the holidays.
Notes:
Attributedirectly, but may be a little bit inelegant because of possibly differing sizes of the underlying byte buffer.group_endfunction ofAttributeProviderneeds to returnOption<&Attribute>, we can't directly return a lazily-generated attribute. The demo circumvents this by adding a static dummy attribute to end the characteristic and service groups.AttrValuetrait defined in Use a trait for representing attribute values #166 withAsRef<[u8]>because it became impossible for anAttributeto own its underlying data because the trait cannot be implemented for[u8; N]. I think this can be circumvented by passing a slice to a lazily producedAttribute. If anyone feels strongly enough about leaving theAttrValuetrait in, then I can try to work around it.The following related features are not yet implemented:
SignedWriteCommand)PrepareWriteReqandExecuteWriteReq; these would require some means of storing queued data that would take a little more design thinking)write_attributefor added flexibility (see Writeable attributes & attribute permissions #146)ReadReqis made on an attribute that does not have readable permission (see comment in diff)