Ancs ble fixes

[mfalkvidd]

Sorry, there were two commits to fix the ble ancs issues. The patch isn't that small unfortunately. But it works, and maybe it is possible to make it smaller before merging by removing changes that aren't strictly required.

Disclosure: I used Codex to find the problem and implement the patch.

[liamcharger]

@cyberneel can you look over this :))

[mark9064]

Really appreciate you being transparent with using LLM assistance

InfiniTime does not have a formal AI contribution policy. Therefore I will review this as normal for now

Importantly, you must understand the code you're submitting. If you couldn't have written it yourself, please learn how and why it actually works, and fix the inevitable codex bugs that get revealed by doing this (or decide not to continue with the PR)

I will try and review this properly soon but I'm short on time to go over BLE minutiae right now :)

[mark9064]

This sounds pretty dubious.. could you explain?

[mfalkvidd]

Codex suggests keeping the 5 tick delay even though we defer discovery until after a secure link had been established. Seems unnecessary to me, so in the minimal change I have removed the timer entirely.

[mfalkvidd]

Completely understandable. AI-generated code can be troublesome.

I could not have written this myself. I have quite a bit of experience with the Arduino ecosystem and its libraries but I have not used freertos before and I have no experience with nimble. Just as I have no experience with Swift so I could not have suggested the Ancs enabled yes/no fix for InfiniLink without AI assistance.

I have a personal interest in getting this PR to a mergable state (would be nice to not maintain the patch locally forever) as well as a non-personal interest because I think the community would benefit from having it fixed. I’ll just need some time (probably a few weeks) to get sufficiently deep into the stack. I’ll post follow-ups whenever I have something to share.

[mark9064]

Sounds great :) feel free to drop any questions here / in the chat channels if I can be of help

[jgonggrijp]

I'm a relative outsider here; I know C++ and I have done many reviews (though mostly for Python and JavaScript projects), but I have no experience with InfiniTime, firmware in general or Apple APIs. I'm following this PR regardless because @liamcharger suggested that it might still add value if I did a review.

I just started an attempt at doing a review, but quickly realized that the code churn in AppleNotificationCenterClient.cpp is rather heavy. I suspect the effective logic may not necessarily have changed a lot, but because some of it has been outfactored to new functions and because the grouping of the conditions under which those functions are called has changed, tracing the actual logical changes is a very time-consuming effort.

I would in principle be able to do that effort and that might even enable us to condense the patch to a smaller diff. However, I won't have time to do it until the end of June at the earliest. @mark9064 did you already do this as part of your review?

[mark9064]

Yeah, the bonus refactoring is a codex classic :D

I haven't done more than glance at it so far. Looks like I'll be pretty busy for a while too

And yes, review is by far the bottleneck in InfiniTime so anyone happy to help makes a huge difference :) whatever thoughts you have, it'd be great to hear them

[mfalkvidd]

I think you can hold off on reviewing, I’ll se what I can do to remove some of the noise first.

[mfalkvidd]

Alright, I've been digging into this a bit further.

The ble fixes contain 3 elements.

1. Defer service discovery until security link is complete (instead of the current timer that creates a race condition - the user had about 3 seconds to enter the pairing code). This is likely the minimal required change, contained in d52638d. Quite small: 4 files changed, 13 insertions(+), 15 deletions(-)
2. Change ANCS setup to use a serialized state machine. Codex provides this description:

If InfiniTime starts ANCS discovery/subscription too early, overlaps GATT operations, writes to the wrong handle, or marks discovery done before CCCD subscriptions really succeed, iOS may remain connected but not actually deliver ANCS notifications. From the user’s point of view, the watch is paired/connected, but notifications are dead until forcing the phone’s Bluetooth stack to reconnect.

This is the large, hard to review change: 6 files changed, 159 insertions(+), 51 deletions(-). It might be good to do this, but I don't think is is strictly necessary, especially considering how much complexity it adds.

3. Fix the advertising data to describe ANCS correctly. InfiniTime is an ANCS client, not an ANCS server, so the ANCS UUID should not be advertised as a local service. Advertising ANCS also causes the advertising to exceed the 31 byte legacy limit, which may cause problems. This change is already isolated in 8563252, 1 file changed, 18 insertions(+), 9 deletions(-). Probably not necessary either, but easy to review.

I'm tempted to only keep (1) for now, since it looks like that addresses the immediate problem and keeps the change small. We can always do the other ones later if we want to.

I have tested with only (1). When I pair in InfiniLink, I am prompted for the pairing code. A few seconds after entering the pairing code, I get the notification to allow the watch to read notifications. After accepting that, notifications work. So all good.

Any opinions? And if I proceed, would you prefer that I create a new PR with the new change? For now, I have reverted the prior commits in this PR and only kept (1) from above, to keep all the history in this PR but it feels a bit messy.

[jgonggrijp]

Kudos on figuring this out and doing it so soon!

Maybe create a second branch that has all three changes (basically a copy of your original branch without the reverts), then simply reset/rebase the current branch to only contain d52638d and force-push. That way, you can keep the history clean without losing the other changes. You can also keep it in the current PR in that way, which is good for keeping all discussion in one place.

To me, change 2 looks a bit as if codex might be just inventing a problem. Regardless, I would suggest creating separate issues for changes 2 and 3, referencing the code changes that codex generated in order to address them, and letting time tell whether those are actual problems that actually need addressing.

13+/15- looks like a very manageable diff, I should be able to review that soon. If everyone agrees that we're going with just that change.

[liamcharger]

So TIL Apple provides a "Require ANCS" connection flag when connecting to the watch. From my understanding, when iOS initiates the connection and this flag is enabled, it prioritizes bringing up ancs right off the bat. Without it, it may only expose well after the bond has been established

I can confirm with the flag set ancs is enabled immediately and notifications come through on the first connection without this pr. The only thing I'm concerned about is if connections will hang for infinitime versions without ancs support. Because with this change, iOS only hands the peripheral off to the app after the connection is fully completed and ancs is up and running. It seems to handle it fine, but I've pushed a commit to InfiniLink that includes a switch in developer settings to toggle this flag so others can test

[mfalkvidd]

I’ve done some more testing. Without (3) I cannot discover (and therefore not pair) the watch using bluetoothctl on my Linux laptop. And without pairing no dfu so I had to boot into the recovery firmware.

[liamcharger]

I’ve done some more testing. Without (3) I cannot discover (and therefore not pair) the watch using bluetoothctl on my Linux laptop. And without pairing no dfu so I had to boot into the recovery firmware.

Does it work fine on a build without ANCS?

[mfalkvidd]

This is what I have observed:

Firmware	Can discover with bluetoothctl
stock 1.16.0	Yes
stock "fallback" (holding down the button until the bootloader pine turns red) - 0.14.1	Yes
ANCS + local changes + this PR	No
ANCS + local changes + this PR + (3)	Yes