Ancs ble fixes by mfalkvidd · Pull Request #20 · cyberneel/InfiniTime

mfalkvidd · 2026-05-07T13:59:23Z

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.

Delay BLE service discovery until the link is encrypted instead of starting it immediately after connection. Some phones appear to reject or ignore ANCS setup while pairing/security is still in progress, leaving notifications stuck until Bluetooth is toggled on the phone. Also make ANCS descriptor setup more robust by discovering CCCDs one at a time, using each characteristic's actual handle range, subscribing sequentially, and finishing discovery only once subscription state is known.

Start BLE service discovery only after the connection has reached an encrypted state instead of using the generic BleConnected delay. iOS only exposes and authorizes ANCS after pairing/security is complete, so the old timer-based path could race the pairing flow and miss ANCS entirely. In that case the notification permission dialog would not appear until a later Bluetooth reconnect. Also tighten the ANCS setup path so descriptor discovery and CCCD writes are serialized and scoped to the actual Notification Source and Data Source characteristics. This removes the bogus write to the ANCS service end handle and avoids treating the Control Point as a subscribable characteristic. Finally, 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. Keep DFU in the local 128-bit service UUID list and place ANCS in the scan response as a 128-bit Service Solicitation UUID. This keeps the legacy advertising payload within size limits and should make LE scanners such as bluetoothctl behave more reliably.

liamcharger · 2026-05-07T16:32:37Z

@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 · 2026-05-13T22:41:10Z

-          // Services discovery is deferred from 3 seconds to avoid the conflicts between the host communicating with the
-          // target and vice-versa. I'm not sure if this is the right way to handle this...
+          isBleDiscoveryStarted = true;
+          // Service discovery is started after link security is established. Keep a short defer so the security callback can unwind.


This sounds pretty dubious.. could you explain?

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 · 2026-05-14T17:53:11Z

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 · 2026-05-14T20:24:13Z

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

jgonggrijp · 2026-05-14T20:25:18Z

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 · 2026-05-14T23:10:36Z

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 · 2026-05-15T05:08:35Z

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

This reverts commit 96eebee.

This reverts commit 8563252.

The old timer method creates a race condition, especially when user has to enter ble pairing code.

mfalkvidd · 2026-05-16T21:09:36Z

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

The ble fixes contain 3 elements.

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(-)
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.

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 · 2026-05-17T00:37:32Z

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 · 2026-05-17T02:28:30Z

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 · 2026-05-21T21:51:45Z

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 · 2026-05-22T01:18:33Z

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 · 2026-05-25T08:42:31Z

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

jgonggrijp

Keeping in mind my previously mentioned caveats (familiar with C++ and reviews but not with ANCS, InfiniTime or embedded software), here is my review of the condensed changes.

I find it credible that these changes will improve/enable successful ANCS discovery. I'm somewhat concerned that the code is not yet robust against unsuccessful pairings (see second code comment below). However, I don't think this would be a regression compared to the previous situation, so as far as I'm concerned, that is not a reason not to merge this.

jgonggrijp · 2026-07-08T13:22:56Z

        connectionHandle = event->connect.conn_handle;
        bleController.Connect();
        systemTask.PushMessage(Pinetime::System::Messages::BleConnected);
-        // Service discovery is deferred via systemtask


Is this comment no longer true?

jgonggrijp · 2026-07-08T13:39:26Z

-          isBleDiscoveryTimerRunning = true;
-          bleDiscoveryTimer = 5;
+          isBleDiscoveryStarted = false;
+          break;
+        case Messages::BleStartDiscovery:
+          if (!isBleDiscoveryStarted) {
+            isBleDiscoveryStarted = true;
+            nimbleController.StartDiscovery();
+          }


My one concern with this change is that only the success case is explicitly covered in the code. When the nimbleController starts discovering, the isBleDiscoveryStarted member is set to true; when the discovery is successful and a connection is made, it is set back to false. I can imagine that discovery might time out or that a connection might fail, but neither of those conditions is represented in the Messages enum or in members of the SystemTask class. NimbleController might have this information (I did not check whether this is the case), but SystemTask does not seem to access the information if it exists. As far as I can tell, that means that SystemTask will keep the isBleDiscoveryStarted == true state indefinitely when things don't go as planned.

mfalkvidd · 2026-07-08T16:50:03Z

Bug thanks for reviewing. I’ll look deeper i to your comments when I’m at a computer. Might be a few days. Will report back here.

mfalkvidd · 2026-07-19T20:57:37Z

I haven’t forgotten about this, but finding time while the kids are home is proving difficult. I’ll report as soon as I can.

mfalkvidd added 2 commits May 7, 2026 15:45

liamcharger mentioned this pull request May 12, 2026

ANCS Support InfiniTimeOrg/InfiniTime#2217

Open

mark9064 reviewed May 13, 2026

View reviewed changes

mfalkvidd added 3 commits May 16, 2026 22:35

Revert "ble: start service discovery after encryption"

bd796e1

This reverts commit 96eebee.

Revert "ble: defer ANCS discovery until link encryption"

671166b

This reverts commit 8563252.

ble: start service discovery after encryption

d52638d

The old timer method creates a race condition, especially when user has to enter ble pairing code.

jgonggrijp reviewed Jul 8, 2026

View reviewed changes

Conversation

mfalkvidd commented May 7, 2026

Uh oh!

liamcharger commented May 7, 2026

Uh oh!

mark9064 left a comment

Choose a reason for hiding this comment

Uh oh!

mark9064 May 13, 2026

Choose a reason for hiding this comment

Uh oh!

mfalkvidd May 16, 2026

Choose a reason for hiding this comment

Uh oh!

mfalkvidd commented May 14, 2026

Uh oh!

mark9064 commented May 14, 2026

Uh oh!

jgonggrijp commented May 14, 2026

Uh oh!

mark9064 commented May 14, 2026

Uh oh!

mfalkvidd commented May 15, 2026

Uh oh!

mfalkvidd commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgonggrijp commented May 17, 2026

Uh oh!

liamcharger commented May 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfalkvidd commented May 21, 2026

Uh oh!

liamcharger commented May 22, 2026

Uh oh!

mfalkvidd commented May 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jgonggrijp left a comment

Choose a reason for hiding this comment

Uh oh!

jgonggrijp Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

jgonggrijp Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

mfalkvidd commented Jul 8, 2026

Uh oh!

mfalkvidd commented Jul 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mfalkvidd commented May 16, 2026 •

edited

Loading

liamcharger commented May 17, 2026 •

edited

Loading

mfalkvidd commented May 25, 2026 •

edited

Loading