Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice progress!
I think I will start by writing a regular macro for creating a service that expands to be as many attributes as required. (this may just not work, and could be a waste of time to not go straight for the custom derive)
Technically we don't strictly need a regular or a procedural macro to get this to work, it's just a more convenient way to define stuff. I think it might be good to wait until after (or at) Oxidize with this if it would be a lot of work (the proc macro definitely would be).
temp: [Attribute { | ||
att_type: AttUuid::Uuid16(Uuid16(0x2800)), | ||
handle: AttHandle::from_raw(0x1234), | ||
value: HexSlice(&[0xCD, 0xAB]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment explaining what the attribute type and value mean in this context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (locally 😉)
src/ble/att/mod.rs
Outdated
@@ -289,16 +302,16 @@ impl<A: Attributes> AttributeServer<A> { | |||
pdu: Pdu, | |||
responder: &mut L2CAPResponder, | |||
) -> Result<(), AttError> { | |||
match pdu.params { | |||
let mut buf = [0u8; 16]; | |||
let mut writer = ByteWriter::new(&mut buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to directly use the L2CAPResponder
instead, but we can refactor this later. The fixed-size buffer isn't very nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely agree
I made your suggested changes, still really unhappy with the API. This is the state I got it in so that it even builds and runs, I've been fighting the borrow checker all over the place to try and make it so a user can define services, which get turned into their respective attributes (plural!!). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commented on a few more things I noticed. I assume we'll figure out the API once we need more than just dummy data.
@@ -268,14 +273,14 @@ impl Default for AttPermission { | |||
|
|||
/// Trait for attribute sets that can be hosted by an `AttributeServer`. | |||
pub trait Attributes { | |||
fn attributes(&self) -> &[Attribute]; | |||
fn attributes(&mut self) -> &[Attribute]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we've talked about this already, but if this trait is modified to look like this instead, perhaps we can avoid some problems wrt storing the attributes in the future:
trait Attributes<'a> {
type Iter: Iterator<Item=&'a mut Attribute>;
fn attributes(&'a mut self) -> Self::Iter;
}
This can also be done later though, just wanted to write down what I had in mind here.
bbdfaea
to
d825b49
Compare
d825b49
to
eb145c6
Compare
This is much more flexible than `respond` and is needed by ATT cleanups.
Primary Service is now showing up in the app!
Closes #29 (maybe)
Need some discussion on how it should be organised and structured:
I think I will start by writing a regular macro for creating a service that expands to be as many attributes as required. (this may just not work, and could be a waste of time to not go straight for the custom derive)
Once I have that working, and have implemented more of ATT and GATT, I will take Jonas's suggestion and write a much more user friendly and feature-complete custom derive.