ANCS Support #2217

cyberneel · 2025-01-03T03:58:38Z

This PR Adds support for the Apple Notification Center Service (ANCS). It shows the Title and Subtitles as "Title - Subtitle" and the message is shown in the body of the InfiniTime notification. Accepting and Declining calls also worked.

The only "issue" right now is that you have to ReConnect after the first pair for ANCS discovery to go well. Maybe there is a way in the NimBLE stack to trigger ANCS Discovery after pairing change? Also, sometimes ANCS goes dormant and then works again. I think this could either be iOS throttling or

"ANCS is not guaranteed to always be present. As a result, the NC should look for and subscribe to the Service Changed characteristic of the GATT service in order to monitor for the potential publishing and unpublishing of the ANCS at any time."

… be implemented.

…AP event

Currently, the notification UID, Event ID and Category are shown.

Data source is not working properly, the Characteristic and/or descriptor isn't being found correctly

Datasource seems to go off and on.

Need to iron out some things like characteristics disappearing and reappearing.

Also added constants to set lengths for the title, subtitle, and message.

…essage is in the body.

Accepting and Declining works

github-actions · 2025-01-03T04:08:58Z

Build size and comparison to main:

Section	Size	Difference
text	381588B	8580B
data	948B	0B
bss	22720B	184B

Run in InfiniEmu

liamcharger · 2025-01-03T05:15:40Z

For reference, closes #910

Instructions: since an encrypted connection is required before ANCS is available, you’ll need to disconnect your watch from your companion app after updating, then reconnect. You’ll get a passkey on your watch to enter into the dialog on your iPhone. After bonding, toggle off and on your iPhone’s Bluetooth, then tap InfiniTime in the list of bonded devices. Finally, you’ll get an alert asking you to allow your watch to read notifications. Click allow, and now you should start receiving notifications!

Something to note: as of now, since using ANCS creates a connection through iOS directly instead of through the companion app, the app will not be able to reconnect to InfiniTime without removing the bond, which results in ANCS being disabled until the steps above are repeated. We’re working on fixing this ;)

Update: this is fixed in the latest InfiniLink commit on the rebuild branch

…re on iOS

minacode · 2025-01-16T11:44:47Z

src/components/ble/AppleNotificationCenterClient.h

+        NegativeAction = (1 << 4)
+      };
+
+      struct AncsNotitfication {


mark9064 · 2025-01-16T21:54:26Z

Does this implement subscribing to service changes? I think this is probably required given the ANCS can come and go at any time

I think the implementation of this is relatively simple, subscribe to the generic attribute service changed characteristic, and any time an indication is received service discovery should be performed again

cyberneel · 2025-01-20T17:11:43Z

@mark9064 I didn't explicitly implement the service changed part, but in all my testing the watch was always able to recover from a disconnect and showed new notifications. So maybe there is something in the stack that handles this?

mark9064 · 2025-01-23T00:13:36Z

I don't believe it's handled elsewhere, but it could be? The text you quote at the top, about the service being registered/unregistered, isn't talking about bluetooth disconnects but actually the service itself disappearing from the list of services the apple device exposes, and then re-appearing at some later time. Maybe in practice apple devices never really do this (even though the spec allows them to), so you don't observe any issues. But I think it would be wise to have this base covered if the standard allows it

minacode · 2025-02-04T10:54:43Z

How long did you test it? Could it be that the service disappears after a few hours?

liamcharger · 2025-02-04T14:05:42Z

I'm able to use this for multiple days at a time without missing a notification.

According to the logs, the watch is (very frequently) not able to subscribe to the data source, but always recovers before the next notification is sent.

minacode · 2025-02-09T23:30:19Z

Thank you for implementing this, @cyberneel!
I just typed the secure connection code into my iPhone for the first time, am already a little hyped from that and will test this PR over the next week :)

mark9064

This is an enormous achievement, well done. There's a lot of code, so I have a lot of comments - no rush to getting to them all.

There are a few overall architectural bits that I've got questions about:

There's a lot state being introduced. Is all of it being used? If possible, I think we should aim to store the minimum state required to show notification titles and text and allow calls to be accepted/declined.
There's quite a lot of std::string usage, and I'm worried it might create a lot of heap pressure which may conflict with memory intensive watchfaces like G7710. Would it be possible to minimise memory usage by moving bytes from the BLE stack into the allocated notification object more directly? If not that's OK, but we should carefully assess the memory pressure possible.
There seems to be an entire private storage of ANCS notifications inside the ANCS client. It'd be really great to just have one store of notifications, with that being the central notification controller. On a similar note, the extra ANCS ID being attached to each notification doesn't feel great. Maybe a UUID for each notification would be good though - perhaps refactoring towards a generic ID also usable by the other notification clients could be good? What do you think?

This review isn't complete as there's probably more things to discuss, but hopefully it's a good starting point.

Great job with this implementing this :)

mark9064 · 2025-02-10T23:34:49Z

src/components/ble/AppleNotificationCenterClient.cpp

+      } else if (ble_uuid_cmp(&dataSourceChar.u, &characteristic->uuid.u) == 0) {
+        char msg[55];
+        snprintf(msg, sizeof(msg), "ANCS Characteristic discovered: Data Source\n%d", characteristic->val_handle);
+        NRF_LOG_INFO(msg);


Sorry probably being a bit dense here, I don't use a devkit/logging:
Why not pass characteristic->val_handle as an argument to NRF_LOG_INFO? I believe the current way it will always do the snprintf even when not in debug mode

mark9064 · 2025-02-10T23:36:37Z

src/components/ble/AppleNotificationCenterClient.cpp

+    bool silent = (eventFlags & static_cast<uint8_t>(EventFlags::Silent)) != 0;
+    // bool important = eventFlags & static_cast<uint8_t>(EventFlags::Important);
+    bool preExisting = (eventFlags & static_cast<uint8_t>(EventFlags::PreExisting)) != 0;
+    // bool positiveAction = eventFlags & static_cast<uint8_t>(EventFlags::PositiveAction);


What is this commented code for?

ANCS provides tags for what type of notification it is sending, and I wasn't using those at the time.

mark9064 · 2025-02-10T23:37:22Z

src/components/ble/AppleNotificationCenterClient.cpp

+    uint8_t messageSize = maxMessageSize + 4;
+    BYTE request[14];
+    request[0] = 0x00; // Command ID: Get Notification Attributes
+    request[1] = (uint8_t) (notificationUuid & 0xFF);


Prefer using C++ style static_cast (this applies in other locations also)

mark9064 · 2025-02-10T23:37:35Z

src/components/ble/AppleNotificationCenterClient.cpp

+    uint8_t titleSize = maxTitleSize + 4;
+    uint8_t subTitleSize = maxSubtitleSize + 4;
+    uint8_t messageSize = maxMessageSize + 4;
+    BYTE request[14];


Would uint8_t be more appropriate here?

mark9064 · 2025-02-10T23:38:03Z

src/components/ble/AppleNotificationCenterClient.cpp

+        decodedSubTitle += "...";
+      }
+
+      // if (messageSize > maxMessageSize) {


Is this unneeded?

mark9064 · 2025-02-10T23:49:52Z

src/components/ble/AppleNotificationCenterClient.h

+      const ble_uuid16_t gattServiceUuid = {BLE_UUID_TYPE_16, 0x1801};
+      const ble_uuid16_t serviceChangedCharUuid = {BLE_UUID_TYPE_16, 0x2A05};
+
+      enum class Categories : uint8_t {


For all of these constants above and below, is there a reference document somewhere? It would be great to have the URL linked in a comment

mark9064 · 2025-02-10T23:51:57Z

src/components/ble/AppleNotificationCenterClient.h

+      bool isGattDiscovered {false};
+      bool isGattCharacteristicDiscovered {false};
+      bool isGattDescriptorFound {false};
+      bool isDiscovered {false};
+      bool isCharacteristicDiscovered {false};
+      bool isDescriptorFound {false};
+      bool isControlCharacteristicDiscovered {false};
+      bool isControlDescriptorFound {false};
+      bool isDataCharacteristicDiscovered {false};
+      bool isDataDescriptorFound {false};


Massive networks of booleans like this scare me a bit. There are theoretically 2^10 = 1024 states here. Could this be refactored into a state enum somehow?

mark9064 · 2025-02-10T23:59:34Z

src/components/ble/AppleNotificationCenterClient.cpp

+  ble_gattc_disc_svc_by_uuid(connectionHandle, &ancsUuid.u, OnDiscoveryEventCallback, this);
+}
+
+void AppleNotificationCenterClient::DebugNotification(const char* msg) const {


Old debugging code?

mark9064 · 2025-02-11T00:04:24Z

src/components/ble/AppleNotificationCenterClient.cpp

+  auto isInFontDefinition = [](uint32_t codepoint) -> bool {
+    // Check if the codepoint falls into the specified font ranges or is explicitly listed
+    return (codepoint >= 0x20 && codepoint <= 0x7E) ||   // Printable ASCII
+           (codepoint >= 0x410 && codepoint <= 0x44F) || // Cyrillic


How come there's a Cyrillic check? I thought this wasn't supported by any of the fonts. More generally do we need some generic way to check this is within the font ranges? How does the existing notification client handle this?

mark9064 · 2025-02-11T00:15:14Z

src/components/ble/AppleNotificationCenterClient.cpp

+  } else {
+    NRF_LOG_INFO("ANCS New alert subscribe ERROR");
+  }
+  if (isDescriptorFound == isControlDescriptorFound && isDescriptorFound == isDataDescriptorFound)


This is hard to read. What does it mean semantically to compare the values of isDescriptorFound and isControlDescriptorFound (for example)? Is the meaning the same if they're both false compared to when they're both true?

minacode · 2025-02-11T09:20:45Z

Yes, it is really impressive that you did it. Thank you!

Yesterday I found that we have no-device support for German umlauts öäü and other such characters. Since the notifications come directly from iOS, we have to discuss how far we can and want to go on the watch.

But that would be no blocking issue for this PR from my side as long as the BLE service works fine otherwise. So maybe discuss this in the linked issue instead of here.

minacode · 2025-02-14T23:49:57Z

My test over the last week was very positive. Great job!
Feature-wise I would merge it as is, but haven't looked at the code in detail yet and will hopefully do so in the next days.

minacode

Again, thank you for your contribution! I have reviewed the first half of your code and found some smaller issues. Overall, the code is written well and one can see that you figured out some clever things to get this feature up and running.
My feedback primarily focuses on two areas:

Seeing if we can cut some needed memory.
Reducing the amount of magic numbers by using constants or adding some explanations.

minacode · 2025-02-15T15:22:14Z

src/components/ble/AppleNotificationCenterClient.cpp

+#include "components/ble/AppleNotificationCenterClient.h"
+#include <algorithm>
+#include "components/ble/NotificationManager.h"
+#include "systemtask/SystemTask.h"


nitpick: you could sort the imports

minacode · 2025-02-15T16:48:18Z

src/components/ble/AppleNotificationCenterClient.cpp

+    if (isDataCharacteristicDiscovered) {
+      ble_gattc_disc_all_dscs(connectionHandle, dataSourceHandle, ancsEndHandle, OnANCSDescriptorDiscoveryEventCallback, this);
+    }
+    if (isCharacteristicDiscovered == isControlCharacteristicDiscovered && isCharacteristicDiscovered == isDataCharacteristicDiscovered) {


Can you explain those conditions, please?

minacode · 2025-02-15T16:50:05Z

src/components/ble/AppleNotificationCenterClient.cpp

+  } else {
+    if (characteristic != nullptr) {


you could inline this to else if, right?

minacode · 2025-02-15T17:09:37Z

src/components/ble/AppleNotificationCenterClient.cpp

+      if (titleSize >= maxTitleSize) {
+        decodedTitle.resize(maxTitleSize - 3);
+        decodedTitle += "...";
+        if (!decodedSubTitle.empty()) {
+          decodedTitle += " - ";
+        } else {
+          decodedTitle += "-";
+        }


if decodedTitle is longer than maxTitleSize, then you resize it to 3 chars less than max. Then you add "...", which makes it full again, and then you add " - " or even -. To me, it seems like decodedTitle is again too long after that.

minacode · 2025-02-15T17:12:28Z

src/components/ble/AppleNotificationCenterClient.cpp

+    uint8_t eventId;
+    uint8_t eventFlags;
+    uint8_t category;
+    uint8_t categoryCount;
+    uint32_t notificationUuid;
+
+    os_mbuf_copydata(event->notify_rx.om, 0, 1, &eventId);
+    os_mbuf_copydata(event->notify_rx.om, 1, 1, &eventFlags);
+    os_mbuf_copydata(event->notify_rx.om, 2, 1, &category);
+    os_mbuf_copydata(event->notify_rx.om, 3, 1, &categoryCount);
+    os_mbuf_copydata(event->notify_rx.om, 4, 4, &notificationUuid);


Maybe gcc is smart enough to get this, but I will ask anyway 😄
You create those local variables only to insert them into a notification. Couldn't you cut the local vairables and os_mbuf_copydata directly into the ancsNotif?

minacode · 2025-02-15T17:17:56Z

src/components/ble/AppleNotificationCenterClient.cpp

+    uint8_t titleSize = maxTitleSize + 4;
+    uint8_t subTitleSize = maxSubtitleSize + 4;
+    uint8_t messageSize = maxMessageSize + 4;


Can you add a comment here, where that + 4 comes from?

minacode · 2025-02-15T17:23:32Z

src/components/ble/AppleNotificationCenterClient.cpp

+    std::string notifStr;
+
+    if (incomingCall) {
+      notifStr += "Incoming Call:";
+      notifStr += decodedTitle;
+      notifStr += "\n";
+      notifStr += decodedSubTitle;
+    } else {
+      notifStr += decodedTitle;


The first notifStr += ... in the if and else can be an notifStr = ... without +, because notifStr is new.

minacode · 2025-02-15T17:26:05Z

src/components/ble/AppleNotificationCenterClient.cpp

+    if (notifStr.size() > 100) {
+      notifStr.resize(97);


NotificationManager has an attribute MessageSize which encodes this 100.
Please use this and replace the two magic numbers.

minacode · 2025-02-15T17:26:52Z

src/components/ble/AppleNotificationCenterClient.cpp

+      notifStr += "...";
+    }
+
+    notif.message = std::array<char, 101> {};


Replace magic number: 101 -> MessageSize + 1

minacode · 2025-02-15T17:29:14Z

src/components/ble/AppleNotificationCenterClient.cpp

+    if (incomingCall)
+      notif.message[13] = '\0'; // Separate Title and Message
+    else if (!decodedSubTitle.empty())
+      notif.message[titleSize + subTitleSize] = '\0'; // Separate Title and Message
+    else
+      notif.message[titleSize - 1] = '\0'; // Separate Title and Message


Please use braces here, just to be safe.

cyberneel and others added 19 commits December 18, 2024 20:15

Started working on ANCS

965e69a

Started working on he client, which is the correct half that needs to…

646c209

… be implemented.

Formatting

315f69b

Added RTT logging

fcdecbb

Added debugging through watch notifications

b9cc3c3

Removed encryption of battery and added required pairing on connect G…

72df4fc

…AP event

ANCS Partially works

0a26a59

Currently, the notification UID, Event ID and Category are shown.

Started working on using Control Point and Data Source:

3cc4ece

Data source is not working properly, the Characteristic and/or descriptor isn't being found correctly

More debugging

dfa13a9

Got Datsource to somewhat work and notifications now show title.

8916920

Datasource seems to go off and on.

Fixed check to only let new notifications pass (maybe)

7f0e066

Proper Notification works.

fce4a60

Need to iron out some things like characteristics disappearing and reappearing.

Fixed the event flag detection. Only shows a notification when it's new.

9fbc594

Also added constants to set lengths for the title, subtitle, and message.

Fixed first letter in Subtitle being cut off

1bed731

Improve message display

670c379

Updated notification, Title and Subtitle are in the orange test and m…

5bf8406

…essage is in the body.

Implemented Incoming Calls to ANCS

288d167

Accepting and Declining works

Turned off non important debugs

f370d63

Formatted Code

0ba425a

Remove RTT settings

2fe3a6b

liamcharger mentioned this pull request Jan 4, 2025

What is the current state of getting iOS notifications, and third-party music apps to work? InfiniTimeOrg/InfiniLink#71

Closed

cyberneel added 5 commits January 4, 2025 16:08

Removed more debug notifications

1e4989f

Added check for special characters and show unknown symbol.

8ab8296

removed all debug notifications

bb48b5f

Ignore silent notifications

c2b392b

Make trunacated messages end in "..." to indicate that message has mo…

f888483

…re on iOS

cyberneel mentioned this pull request Jan 7, 2025

Music controls don't work for iOS #460

Open

minacode reviewed Jan 16, 2025

View reviewed changes

mark9064 added the new feature This thread is about a new feature label Jan 16, 2025

liamcharger force-pushed the ancs branch from c7520c0 to f888483 Compare January 16, 2025 14:29

liamcharger added 2 commits January 16, 2025 09:30

Fix typos and remove unnecessary commented code

65ead6e

Merge branch 'main' into ancs

b8da191

Merge branch 'main' into ancs

de63dce

mark9064 reviewed Feb 11, 2025

View reviewed changes

minacode mentioned this pull request Feb 11, 2025

Umlauts (UTF-8) in notifications #212

Open

minacode requested changes Feb 15, 2025

View reviewed changes

ANCS Support #2217

Are you sure you want to change the base?

ANCS Support #2217

Conversation

cyberneel commented Jan 3, 2025

github-actions bot commented Jan 3, 2025 • edited Loading

liamcharger commented Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mark9064 commented Jan 16, 2025

cyberneel commented Jan 20, 2025

mark9064 commented Jan 23, 2025

minacode commented Feb 4, 2025

liamcharger commented Feb 4, 2025

minacode commented Feb 9, 2025

mark9064 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minacode commented Feb 11, 2025

minacode commented Feb 14, 2025

minacode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

minacode Feb 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 3, 2025 •

edited

Loading

liamcharger commented Jan 3, 2025 •

edited

Loading

minacode Feb 15, 2025 •

edited

Loading