SHIELD remote support: discover ATVV handles per device (UNTESTED) - #3
SHIELD remote support: discover ATVV handles per device (UNTESTED)#3xbmc4lyfe wants to merge 2 commits into
Conversation
The btmon reader matched hardcoded ATT handles (0x0042 status, 0x003f audio) that are specific to the UR02, so no other ATVV remote could be seen even though they all expose the same ab5e... service. Discover the control/audio/status characteristics per device at service start and derive their ATT value handles (BlueZ names the object by the declaration handle .../charNNNN; the value handle is declaration+1 — verified against the UR02: char0041->0x0042, char003e->0x003f, char003c->0x003d). The btmon reader now matches the discovered handles, so any device exposing the ATVV voice service works without hardcoding. Add a 'Voice remote MAC (blank = auto-detect)' setting to bind a specific remote when more than one is paired (e.g. UR02 vs SHIELD); blank auto-picks the first remote exposing the voice service. No behavior change for the UR02 (discovery returns its known handles). UNTESTED on the SHIELD — the declaration+1 handle convention is the main thing to confirm there; opening for later on-device testing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe BLE voice flow now discovers ATVV characteristic paths and handles per remote, supports optional remote MAC selection, uses discovered handles during monitoring, and documents the ChangesATVV remote support
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Warning Billing warning: we have not been able to collect payment for this subscription for more than 72 hours. Please update the payment method or pay any pending invoices in Billing to avoid service interruption. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/audio_capture/ble.py`:
- Around line 93-96: Update the device filtering logic around device_address so
it matches the complete /dev_<MAC>/ path component rather than checking whether
the normalized address appears anywhere in path. Preserve the existing
case-insensitive behavior and continue when the exact device component is
absent.
In `@service.py`:
- Around line 359-365: Update _start_ble_monitor() to inspect the return values
from both _start_notify() calls and return failure when either status or audio
notification setup fails, preserving retry behavior. Update _start_notify() to
treat any nonzero dbus-send exit status as failure. Keep _send_get_caps()
optional, reporting its failure as a warning without preventing successful
monitoring when notifications are active.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e79bb64-adfd-4d9e-9741-40463ceed5af
📒 Files selected for processing (10)
addon.xmlchangelog.txtlib/audio_capture/ble.pyresources/language/resource.language.en_gb/strings.poresources/language/resource.language.es_es/strings.poresources/language/resource.language.fr_fr/strings.poresources/language/resource.language.he_il/strings.poresources/language/resource.language.it_it/strings.poresources/settings.xmlservice.py
📜 Review details
🧰 Additional context used
🪛 Ruff (0.15.21)
lib/audio_capture/ble.py
[warning] 107-107: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (10)
resources/settings.xml (1)
149-161: LGTM!resources/language/resource.language.en_gb/strings.po (1)
105-108: LGTM!resources/language/resource.language.es_es/strings.po (1)
105-108: LGTM!resources/language/resource.language.fr_fr/strings.po (1)
105-108: LGTM!resources/language/resource.language.he_il/strings.po (1)
108-111: LGTM!resources/language/resource.language.it_it/strings.po (1)
104-108: LGTM!changelog.txt (1)
1-7: LGTM!addon.xml (1)
2-2: LGTM!lib/audio_capture/ble.py (1)
2-55: LGTM!Also applies to: 72-92, 97-178, 336-341, 426-427
service.py (1)
49-55: LGTM!Also applies to: 323-358, 366-368, 436-463
![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: 5740b9289f
ℹ️ 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".
| continue | ||
| line = raw.decode("utf-8", errors="replace").rstrip() | ||
|
|
||
| status_match = "Handle: " + self._status_handle in prev_line |
There was a problem hiding this comment.
When a MAC is configured, discover_voice_endpoints(addr) selects that remote's characteristic paths, but the btmon reader later treats any packet with the same ATT handle as selected. ATT handles are only unique within a peripheral's GATT database, so with two connected ATVV remotes that use the same status/audio handle (for example, two of the same model), pressing or streaming from the non-selected remote will still trip this match and can start or corrupt capture despite the configured MAC. The watcher needs to also associate btmon packets with the selected connection/device for the new binding setting to be reliable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid — acknowledged and documented in _btmon_reader (ee92a9a). This only bites with two paired remotes of the same model (identical ATT handles); different models (e.g. UR02 vs SHIELD) use different handles and are unaffected, so the voice_device_address binding is reliable in the mixed-model case this PR targets. Full ACL-connection correlation is deferred because it needs two remotes to validate on hardware, and this PR is explicitly untested pending SHIELD testing. Leaving this thread open to track the follow-up.
CodeRabbit + Codex review on PR #3: - _scan_chars: match the exact /dev_<MAC>/ path component instead of a substring, so a partial/ambiguous voice_device_address can't bind an unintended remote. Uppercase the whole component (incl. the 'dev_' literal) before comparing to path.upper() — a unit test caught that the naive form (lowercase 'dev_' vs path.upper()) never matches and would have broken discovery whenever a MAC was configured. - _start_notify: treat a nonzero dbus-send exit as failure, tolerating only the benign already-notifying / InProgress case. - _start_ble_monitor: return False if either status/audio notification fails to enable, so the retry loop stays alive instead of silently going deaf; GET_CAPS failure remains a non-fatal warning. - Document the known handle-only matching limitation in _btmon_reader: two identical-model remotes sharing ATT handles could cross-trip; full ACL-connection correlation is deferred until testable with two remotes. Verified offline: py_compile clean; unit tests cover exact-match (case-insensitive, partial rejection, other-device rejection) and the notify tolerance logic. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee92a9adcb
ℹ️ 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".
| self._ble_backend = BLEAudioCapture( | ||
| audio_handle=endpoints["audio_handle"] | ||
| ) |
There was a problem hiding this comment.
When either StartNotify call fails below, _start_ble_monitor() returns False and run() does not start the shared btmon watcher, but this earlier assignment leaves _worker() treating BLE as an externally fed backend. Any screen/RunScript activation during that retry window then calls start_recording(external_feed=True) with no external feed running, so it waits until timeout instead of failing fast or starting its own capture; assign self._ble_backend only after notifications succeed, or clear it on the failure path.
Useful? React with 👍 / 👎.
1 participant
Summary
Generalizes voice input so ATVV remotes other than the UR02 — notably the NVIDIA SHIELD remote — can work, without hardcoding any one remote's ATT handles.
Not yet tested on a SHIELD — opening for later on-device testing. No behavior change for the UR02 (handle discovery returns its existing values, verified offline against a real capture).
Background
All these remotes expose the same Google Android TV Voice (ATVV) GATT service (
ab5e0001-...). But btmon reports ATT handles, not UUIDs, and the previous code matched the UR02's hardcoded handles (0x0042status,0x003faudio,0x003dcontrol). The SHIELD lays its attributes out at different handles, so its notifications were invisible.Changes
discover_voice_endpointsinble.py): finds the control/audio/status characteristics of a connected ATVV remote and derives each one's ATT value handle from its D-Bus path. BlueZ names the object by the declaration handle (.../charNNNN); the value handle used in ATT PDUs is declaration+1 — verified against the UR02 (char0041→0x0042,char003e→0x003f,char003c→0x003d).service.py) now matches the discoveredself._status_handle/self._audio_handleinstead of literals; the backend takes the audio handle too.voice_device_address("Voice remote MAC (blank = auto-detect)") to bind a specific remote when more than one is paired (UR02 vs SHIELD). Blank auto-picks the first remote exposing the service. Added to all five language files.Testing notes (for the SHIELD, later)
kodi.logforVoice keyboard BLE: bound <dev> (status=0x…, audio=0x…).mic button detected via btmon. If not, thedeclaration+1value-handle assumption is the first thing to check — capture withbtmonand compare the actual notification handle to the discovered one.BLEPacketFramer).Offline verification
py_compileclean; handle-mapping logic unit-tested against the real UR02 D-Bus paths (all three map to the correct value handles).🤖 Generated with Claude Code