Add possibility to listen to notifications #102

pferreir · 2024-09-01T19:11:28Z

This allows the GattClient to also register to listen to notifications for a Characteristic.

lulf

Thanks for the PR. Looks good in general.

Couple of things:

I think maybe it's gonna explode RAM usage storing that Vec<> in the channels. Consider using the PacketPool instead, which should reduce the RAM requirements considerably, because at the moment you risk having a lot of 'unused RAM' from defining the MAX limits.
I would also like if you modified the ble_bas_central example to use this.

lulf · 2024-09-02T08:08:16Z

host/src/gatt.rs

+    const MAX_SERVICES: usize,
+    const MAX_NOTIF: usize,
+    const NOTIF_QSIZE: usize,
+    const L2CAP_MTU: usize = 27,
+    const ATT_MTU: usize = 23,


These are a bit hard to keep in sync. I'd rather just use l2cap_mtu for the vec size to avoid them.

pferreir · 2024-09-02T21:32:52Z

I think maybe it's gonna explode RAM usage storing that Vec<> in the channels. Consider using the PacketPool instead, which should reduce the RAM requirements considerably, because at the moment you risk having a lot of 'unused RAM' from defining the MAX limits.

I think the problem with this is that calls to PacketPool::alloc need to have a static lifetime, and that means I'll have to require &'static self in some of the methods...

lulf · 2024-09-03T07:46:56Z

I think maybe it's gonna explode RAM usage storing that Vec<> in the channels. Consider using the PacketPool instead, which should reduce the RAM requirements considerably, because at the moment you risk having a lot of 'unused RAM' from defining the MAX limits.

I think the problem with this is that calls to PacketPool::alloc need to have a static lifetime, and that means I'll have to require &'static self in some of the methods...

You'd need to use a StaticCell for the pool, and pass it to the gatt_client when creating, it doesn't mean the gatt client itself must be static, only the Packet, i.e. Packet<'static>. Then it can assume static lifetime of an allocated packet. This is how the l2cap channel manager works (https://github.com/embassy-rs/trouble/blob/main/host/src/channel_manager.rs#L34-L41), where it holds a ref to a 'static pool, but doesn't require itself to be 'static.

There may very well be other pool implementations that is a better fit, but this one is at least already there with no new dependencies needed.

pferreir · 2024-09-03T18:21:59Z

You'd need to use a StaticCell for the pool, and pass it to the gatt_client when creating

Yeah, I had excluded this because it seemed not very user-friendly. But I've done it and it seems to work.

pferreir · 2024-09-04T13:22:44Z

OK, I think this is more or less what you asked for. I made a design decision which I am not 100% sure of, which is to make the task which handles rx mandatory, so that messages can be triaged and notifications be handled in parallel with regular read/write operations. That also entailed making the service list internally mutable. I created a ble_bas_central example for the rp2040 to demonstrate the concept.
Let me know what you think!

pferreir · 2024-09-04T13:35:10Z

BTW, haven't found anything to test the official example on. I have a device which has a battery service but I don't think it sends any notifications. I know the notification code works because I tested it with a BLE MIDI device I have, but I don't know what to do with the example.

lulf

Looks good to me, let me know if you want to address my inline comment or just go with it as is, I'm fine with either, this should be ready to merge once CI passes.

I agree it's not the ideal user experience having to create the packet pool, but maybe one future improvement would be to utilize the 'config system' (see gen_config.py) and host/src/config.rs to parameterize the pool at build time and keep it 'hidden' rather than having to be passed into the client? It limits how flexible you can configure the options, but otoh maybe that's not needed at this level.

lulf · 2024-09-05T07:29:08Z

host/src/gatt.rs

    pub(crate) rx: DynamicReceiver<'reference, (ConnHandle, Pdu)>,
    pub(crate) ble: &'reference BleHost<'resources, T>,
    pub(crate) connection: Connection<'reference>,
+    pub(crate) request_channel: Channel<NoopRawMutex, (ConnHandle, Pdu), NOTIF_QSIZE>,
+
+    pub(crate) notification_pool: &'static PacketPool<NoopRawMutex, L2CAP_MTU, MAX_NOTIF, 1>,


You should be able to use the &'static dyn GlobalPacketPool to reduce the 'generics hell' 😅 if you want to. This should get rid of L2CAP_MTU and MAX_NOTIF parameters from GattClient.

OK, pushed. But I haven't been able to get rid of those parameters. Otherwise, how do I build the corresponding arrays?

Ah you're right about the notif const. But the L2CAP_MTU should be removable? I don't see it used anywhere. In any case, I think it's fine to merge.

Just rebase it on latest main and we can merge it

Ah you're right about the notif const. But the L2CAP_MTU should be removable? I don't see it used anywhere

it's still used in request to build the receive buffer. Anyway, rebased and pushed!

lulf · 2024-09-10T06:15:33Z

/test

pferreir · 2024-09-10T15:44:49Z

WRT to the test failing, I don't get it how [u8] seems to implement LowerHex in the pico w but not on the desktop?

lulf · 2024-09-10T16:31:13Z

This is a defmt vs log issue. You can reproduce by compiling trouble-host with the log feature. Basically the format specifiers that works with defmt doesnt always work with core fmt.

You need to remove that log line or guard it behind a defmt feature gate

pferreir · 2024-09-10T17:15:22Z

Ok, should be fine now!

lulf · 2024-09-10T18:10:38Z

/test

lulf · 2024-09-10T18:15:30Z

The integration tests fail, you can see the error here https://github.com/embassy-rs/trouble/actions/runs/10798211914/job/29951213402#step:5:153. If you want I can merge it as is and fix the tests after the fact if you're not able to run them yourself (You need 2 HCI dongles). Seems like tests need some increase in the max number of attributes or something.

pferreir · 2024-09-10T18:23:30Z

Oh. How are these tests run in CI, out of curiosity?

…

On Tue, 10 Sept 2024, 20:15 Ulf Lilleengen, ***@***.***> wrote: The integration tests fail, you can see the error here https://github.com/embassy-rs/trouble/actions/runs/10798211914/job/29951213402#step:5:153. If you want I can merge it as is and fix the tests after the fact if you're not able to run them yourself (You need 2 HCI dongles). Seems like tests need some increase in the max number of attributes or something. — Reply to this email directly, view it on GitHub <#102 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAVC2ZVK7DOSEVSSQBCFDZV4ZNRAVCNFSM6AAAAABNPCXC62VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGNBRGY4TCOBVG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

lulf · 2024-09-11T06:38:23Z

@pferreir Like this: https://github.com/embassy-rs/trouble/blob/main/.github/workflows/tests.yaml#L30-L37

Basically I have a local GHA with 2 HCI dongles attached which run the tests in https://github.com/embassy-rs/trouble/tree/main/host/tests

pferreir force-pushed the add-notifs branch 3 times, most recently from 885bbc1 to 1e512ab Compare September 1, 2024 19:33

lulf requested changes Sep 2, 2024

View reviewed changes

pferreir force-pushed the add-notifs branch 2 times, most recently from 51894f4 to 1a8d8f6 Compare September 4, 2024 13:00

pferreir force-pushed the add-notifs branch 3 times, most recently from 8092551 to c7e89c1 Compare September 4, 2024 13:30

lulf approved these changes Sep 5, 2024

View reviewed changes

pferreir force-pushed the add-notifs branch 2 times, most recently from 197a74b to 2a46e38 Compare September 6, 2024 18:05

Add .gitignore

e262d9a

pferreir force-pushed the add-notifs branch from 2a46e38 to 323c1dd Compare September 9, 2024 10:25

pferreir added 2 commits September 10, 2024 19:11

Add possibility to listen to notifications

2a116b0

Fix some clippy warnings

1c5426a

pferreir force-pushed the add-notifs branch from 323c1dd to 1c5426a Compare September 10, 2024 17:11

lulf merged commit 0e0d942 into embassy-rs:main Sep 11, 2024
4 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add possibility to listen to notifications #102

Add possibility to listen to notifications #102

pferreir commented Sep 1, 2024

lulf left a comment

lulf Sep 2, 2024

pferreir commented Sep 2, 2024 •

edited

Loading

lulf commented Sep 3, 2024

pferreir commented Sep 3, 2024

pferreir commented Sep 4, 2024

pferreir commented Sep 4, 2024

lulf left a comment

lulf Sep 5, 2024

pferreir Sep 6, 2024

lulf Sep 9, 2024

pferreir Sep 9, 2024

lulf commented Sep 10, 2024

pferreir commented Sep 10, 2024

lulf commented Sep 10, 2024

pferreir commented Sep 10, 2024

lulf commented Sep 10, 2024

lulf commented Sep 10, 2024

pferreir commented Sep 10, 2024 via email

lulf commented Sep 11, 2024

Add possibility to listen to notifications #102

Add possibility to listen to notifications #102

Conversation

pferreir commented Sep 1, 2024

lulf left a comment

Choose a reason for hiding this comment

lulf Sep 2, 2024

Choose a reason for hiding this comment

pferreir commented Sep 2, 2024 • edited Loading

lulf commented Sep 3, 2024

pferreir commented Sep 3, 2024

pferreir commented Sep 4, 2024

pferreir commented Sep 4, 2024

lulf left a comment

Choose a reason for hiding this comment

lulf Sep 5, 2024

Choose a reason for hiding this comment

pferreir Sep 6, 2024

Choose a reason for hiding this comment

lulf Sep 9, 2024

Choose a reason for hiding this comment

pferreir Sep 9, 2024

Choose a reason for hiding this comment

lulf commented Sep 10, 2024

pferreir commented Sep 10, 2024

lulf commented Sep 10, 2024

pferreir commented Sep 10, 2024

lulf commented Sep 10, 2024

lulf commented Sep 10, 2024

pferreir commented Sep 10, 2024 via email

lulf commented Sep 11, 2024

pferreir commented Sep 2, 2024 •

edited

Loading