Resolve iOS app display names through ANCS - #2
Merged
Conversation
Constraint: Resolve display names through ANCS without external APIs or MQTT device merging Rejected: Publish then enrich | duplicate Home Assistant events Rejected: Remove static names immediately | temporary ANCS failures would regress known apps Confidence: high Scope-risk: moderate Directive: Keep app lookup failure separate from notification delivery failure Tested: Design self-review, repository cross-check, and git diff --cached --check Not-tested: Production code and physical iPhone notification flow are not implemented yet
Constraint: Implement protocol, firmware, Home Assistant migration, and release in test-first order Rejected: Parallel notification enrichment | duplicate Home Assistant events Confidence: high Scope-risk: broad Directive: Keep firmware 0.3.4 and companion 0.6.3 verification scopes distinct Tested: Plan self-review for spec coverage, placeholders, API consistency, and git diff --cached --check Not-tested: Implementation steps have not run yet
Constraint: GitHub Pages must publish firmware documentation only Rejected: Leave internal plans untracked | approved design and execution records must remain reviewable Confidence: high Scope-risk: narrow Directive: Stage Pages content with the superpowers directory excluded Tested: 29 release integrity and documentation contract tests Not-tested: Remote Pages deployment runs after push
Constraint: Display names must come from the connected iPhone without an external catalog or API Rejected: Reuse notification-attribute framing | app responses carry a null-terminated identifier instead of a UID Confidence: high Scope-risk: moderate Directive: Keep app parser errors separate from notification completeness Tested: RED for missing app command/parser APIs; ESP32-C6 -Werror syntax checks for implementation and Unity tests Not-tested: Unity execution on physical hardware is deferred to the integrated firmware image
Constraint: App DisplayName enrichment must stay bounded and reset on reconnect Rejected: Persistent or global app catalog | Names can change and stale catalogs do not cover new apps Confidence: high Scope-risk: narrow Directive: Keep native lookup failures non-fatal so notification delivery can fall back Tested: RED missing resolver API; target ESP compiler syntax check with warnings as errors; resolver cache tests added Not-tested: Unity execution on physical ESP32-C6 deferred to integrated firmware verification
Constraint: Existing MQTT discovery and static app mapping must remain available Rejected: Replacing the static mapping entirely | Native ANCS enrichment can fail or be absent Confidence: high Scope-risk: narrow Directive: Preserve native then mapping then app-id precedence and JSON escaping Tested: RED MQTT source contract; 18 contract tests passed; four changed ESP-IDF objects compiled Not-tested: Unity runtime assertions await integrated ESP32-C6 test firmware
Constraint: Publish one Home Assistant event per iPhone notification Rejected: Publish then patch app name later | It creates duplicate or inconsistent notification events Confidence: high Scope-risk: moderate Directive: App-name failure must never become notification failure and MQTT fallback must remain available Tested: Three RED then GREEN orchestration contracts; 21 focused Python tests; actual ESP32-C6 ancs_client object compiled with full IDF dependency graph Not-tested: Full firmware link and physical ANCS exchange deferred to release verification
Constraint: MQTT entities and raw published_at_ms data must remain untouched Rejected: Removing the MQTT field or shared source device | The companion integration is intentionally separate and MQTT remains enabled Confidence: high Scope-risk: narrow Directive: Migration may remove only the ha_ios_ancs sensor with the published_at_ms suffix Tested: RED sensor migration and translation contracts; 127 Home Assistant tests passed; compileall passed Not-tested: Migration against the user's live Home Assistant registry deferred to deployment verification
Constraint: Firmware 0.3.4 and companion 0.6.3 must remain independently identifiable Rejected: Keeping the previous binaries with new manifests | Release paths and hashes must identify the exact new build Confidence: high Scope-risk: moderate Directive: Keep MQTT Discovery and the companion iOS ANCS device independent Tested: 151 tools tests, 127 Home Assistant tests, seven-target ESP-IDF build matrix, compileall, release integrity Not-tested: Fresh physical iPhone notification after device deployment
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7409ed43d2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| const int serial_result = | ||
| notification_sink_format_json(notification, device_name, payload, capacity); | ||
| ancs_notification_t enriched = *notification; |
There was a problem hiding this comment.
When MQTT is connected, the observer runs synchronously on the ANCS worker configured with an 8,192-byte stack, but this local copy occupies 5,728 bytes with the default field limits. That leaves less than 2.5 KiB for the existing worker/observer frames and the nested JSON formatter and libc calls, making ordinary notification publication liable to overflow the task stack and reset or corrupt the device. Allocate the temporary object dynamically or serialize with an app-name override without copying the entire notification.
Useful? React with 👍 / 👎.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan