Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement async GATT event processing #200

Merged
merged 12 commits into from
Dec 20, 2024
Merged

Implement async GATT event processing #200

merged 12 commits into from
Dec 20, 2024

Conversation

lulf
Copy link
Member

@lulf lulf commented Dec 17, 2024

  • Remove GattServer run, processing now per connection
  • Allow processing GATT requests without attribute server
  • Allow processing GATT events before attribute server

Issue #133, #163

Still very WIP, but the example should show what this will look like.

CC @jamessizeland, @petekubiak

@lulf lulf marked this pull request as ready for review December 18, 2024 08:44
@lulf
Copy link
Member Author

lulf commented Dec 18, 2024

/test

@jamessizeland
Copy link
Collaborator

This looks much neater!

@lulf
Copy link
Member Author

lulf commented Dec 18, 2024

/test

@lulf
Copy link
Member Author

lulf commented Dec 18, 2024

This looks much neater!

Glad you like it! So, the options are now:

  • Run without a GATT server/table, but you must handle the request and response dance manually.
  • Run with a GATT server, inspect read/write events before they get processed by server
  • Run with a GATT server, inspect read/write state after processed by server

I think that covers most of it. It does feel a bit strange to pass the attribute server around perhaps, but I don't see any different solution that allows supporting no attribute server use cases.

@lulf
Copy link
Member Author

lulf commented Dec 18, 2024

/test

1 similar comment
@lulf
Copy link
Member Author

lulf commented Dec 18, 2024

/test

Copy link
Collaborator

@petekubiak petekubiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the new API, very clean! Nice to remove the extra layer of server() indirection as well 👍

self.server.table().set(characteristic, input)
}

#visibility fn deref(&self) -> &AttributeServer<'values, #mutex_type, _ATTRIBUTE_TABLE_SIZE> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the implementation of the Deref trait below you shouldn't need this method I think

examples/apps/src/ble_bas_peripheral.rs Outdated Show resolved Hide resolved
host/src/attribute.rs Outdated Show resolved Hide resolved
host/src/attribute.rs Show resolved Hide resolved
host/src/attribute.rs Outdated Show resolved Hide resolved
}

pub(crate) async fn send(&self, index: u8, pdu: Pdu<'d>) {
let handle = self.with_mut(|state| state.connections[index as usize].handle.unwrap());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Investigating why handle is an Option<> type led me down a bit of a rabbit hole, but I can see now that when a "slot" in the connection storage is empty then its handle will be None.

Obviously this is unrelated to this PR, but is there a reason for having the storage as type [ConnectionStorage; N] which is initially filled with [ConnectionStorage::DISCONNECTED; N] as opposed to making the storage of type [Option<ConnectionStorage>; N], so that when the slot is empty it just contains None?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has some implications on stack/RAM usage because now you'd need to create a "big" ConnectionStorage when allocing, rather than just modifying one field. It's a while ago, so I don't have the numbers.

But it could be worth trying again, and measuring size change.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, I'd have expected the compiler to optimise to modify-in-place if the array already exists, but maybe I'm not fully grasping the way it's used.

host/src/gatt.rs Outdated Show resolved Hide resolved
host/src/gatt.rs Show resolved Hide resolved
host/src/gatt.rs Show resolved Hide resolved
host/src/gatt.rs Show resolved Hide resolved
@jamessizeland
Copy link
Collaborator

another thorough review from @petekubiak 🎸

@petekubiak
Copy link
Collaborator

petekubiak commented Dec 18, 2024

I get easily carried away haha

lulf added 8 commits December 19, 2024 11:34
* Remove GattServer, processing now per connection
* Allow processing GATT requests without attribute server
* Allow processing GATT events before attribute server
* Remove on_read, on_write callback
@lulf
Copy link
Member Author

lulf commented Dec 19, 2024

/test

@lulf
Copy link
Member Author

lulf commented Dec 19, 2024

/test

@lulf lulf merged commit 60a068d into main Dec 20, 2024
2 checks passed
@lulf lulf deleted the unify-callback branch December 20, 2024 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants