feat(split): allow central to connect to multiple peripherals

[xudongzheng]

This addresses #314 and #609.

This might also affect #281 and #718.

[xudongzheng]

Most likely some additional changes would be needed for settings reset. It's not clear how that should be implemented, perhaps just loop and delete ble/peripheral_addresses/[0,7] or just call nvs_clear() to handle peripheral addresses and everything else.

[xudongzheng]

@petejohanson

Is there a reason for this section of code https://github.com/zmkfirmware/zmk/blob/main/app/src/split/bluetooth/central.c#L403-L411? It's not clear to me why split_central_process_connection() would be called there as it's already called from the connect event handler.

I would consider getting rid of the lookup and having https://github.com/slicemk/zmk/commit/44e0e7b158b08d5f014dc813741f72e578eac04f#diff-18de66983adf7197deaf9e826c4087b486beb147e0ab19445d41a85ba7be9fe0R374-R384.

[petejohanson]

@petejohanson

Is there a reason for this section of code https://github.com/zmkfirmware/zmk/blob/main/app/src/split/bluetooth/central.c#L403-L411? It's not clear to me why split_central_process_connection() would be called there as it's already called from the connect event handler.

I would consider getting rid of the lookup and having slicemk@44e0e7b#diff-18de66983adf7197deaf9e826c4087b486beb147e0ab19445d41a85ba7be9fe0R374-R384.

IIRC, at least with Zephyr from several versions ago when I first wrote this, some times we would end up having an existing connection, after the scanning first discovers the advertising info, we and we needed to handle both scenarios:

1. No connect, create it.
2. Already connected, process the connection.

If we can test this thoroughly and see this no longer happening with the stack, we should simplify this, for sure.

[xudongzheng]

My suggestion is to only handle a new connection when the connected callback happens. This should happen exactly once per new connection.

If there is an existing connection, reserve_peripheral_slot() should fail because each peripheral will resolve to the same index each time - that's in zmk_ble_put_peripheral_addr().

Will have an updated PR shortly.

[bezmi]

I have been testing this (on the latest one as of this comment) and it's working well. @xudongzheng would you please consider pushing on top of the current commit instead of re-writing history with the force pushes? Makes it a little easier to explore the changes and can always be squashed when merging. Also, see my comment above RE scan timeout.

[xudongzheng]

@bezmi good point, you can see today's changes in https://github.com/zmkfirmware/zmk/commit/d26b0ff8747476620764da27a401aa1083515286 on top of the earlier commit

I'm not familiar with the scan timeout. I'd consider explicitly stopping the scanning using a custom timer, which might be necessary regardless for maximum flexibility.

[bezmi]

@xudongzheng, I have been testing this PR on my daily driver split keyboard for pretty much as long as it has existed. I often run into a particular latency issue where keypresses from each half will arrive out of order, making it difficult to type quickly. It is very similar in description to #1633. Have you had any such issues and any success in troubleshooting them?

[xudongzheng]

@bezmi The first thing to try is placing the dongle right between the two keyboard halves. If it works fine there, it's likely a signal issue and not a firmware issue. You can bump the Bluetooth TX power as is typically recommended for ZMK.

[petejohanson]

@xudongzheng Can you please rebase now that your refactor #1815 is in? Code looks good w/ your latest push, will test here then merge, barring any problems appearing. Thanks!

[petejohanson]

@xudongzheng See the pre-commit check issue please on this.

[xudongzheng]

@petejohanson Oops, somehow missed that. The new commit seems to pass all the commit checks.