device: replace assert with proper prefix check in enumerate_lang; add multi-packet test - #34
Merged
Merged
Conversation
…d multi-packet test The while loop in enumerate_lang correctly reads all N response packets from the firmware (GET_LANG_LIST, case 8, sends 4 back-to-back). Replace the assert — which is silently disabled with python -O — with a logged warning + break so a stale or mismatched reply stops the loop cleanly rather than crashing in debug and passing corrupt data in optimised mode. Add tests/device/poly_kybd_test.py to exercise the multi-packet reading path with a mocked HID interface: verifies all 4 packets are consumed, all expected language codes end up in all_languages, and that early timeout or unexpected prefix stops the loop without crashing. https://claude.ai/code/session_01GhZQeFcV4JFnxdr8K1YGx9
Adds TestPolyKybdMockAllFirmwareLanguages — a test class that initialises PolyKybdMock with the complete 58-language string from hid_com.c case 8 (the same 4 HID packets the real firmware sends). Verifies the full string round-trips through enumerate_lang, get_lang_list returns exactly 58 entries, every code is present, and change_language succeeds for all of them. https://claude.ai/code/session_01GhZQeFcV4JFnxdr8K1YGx9
…t startup If the keyboard isn't plugged in when PolyKybdHost starts, enumerate_lang fails during __init__ and keeb_lang_menu is never created. reconnect() was only calling query_current_lang on later connections, leaving the language selection menu permanently absent. Fix: inside the compatible branch of reconnect(), build keeb_lang_menu via add_supported_lang if it is still None. The check is a no-op on subsequent reconnects once the menu exists. https://claude.ai/code/session_01GhZQeFcV4JFnxdr8K1YGx9
Previously add_supported_lang always called menu.addMenu() unconditionally, so it could only safely be called once. This meant keeb_lang_menu was never refreshed when a different keyboard (with a different language set) connected. Two changes: - add_supported_lang now updates in-place when keeb_lang_menu already exists: setTitle + clear + repopulate, keeping the menu's position in the tray. - reconnect() always calls add_supported_lang inside the compatible branch (dropped the keeb_lang_menu is None guard), so swapping keyboards always reflects the new device's language list. https://claude.ai/code/session_01GhZQeFcV4JFnxdr8K1YGx9
Reviewer's GuideReplaces a fragile assert-based protocol check in PolyKybd.enumerate_lang with a logged prefix check and early-exit, updates the UI language menu to be reusable on reconnect, and adds comprehensive language-list tests including multi-packet HID responses and full firmware language coverage using mocks. Sequence diagram for PolyKybd.enumerate_lang multi-packet read with prefix checksequenceDiagram
participant Host
participant PolyKybd
participant HidInterface
Host->>PolyKybd: enumerate_lang()
PolyKybd->>HidInterface: read_with_lock(15, lock)
HidInterface-->>PolyKybd: result, reply, lock
PolyKybd->>PolyKybd: expected = expect(Cmd.GET_LANG_LIST).decode()
loop while result and len(reply) > 3
PolyKybd->>PolyKybd: reply = reply.decode().strip('\x00')
alt reply startswith expected
PolyKybd->>PolyKybd: lang_str += reply[3:]
PolyKybd->>HidInterface: read_with_lock(15, lock)
HidInterface-->>PolyKybd: result, reply, lock
else unexpected prefix
PolyKybd->>PolyKybd: log.warning [unexpected reply prefix]
PolyKybd->>PolyKybd: break
end
end
PolyKybd-->>Host: (result, message)
Flow diagram for host.add_supported_lang language menu reuse on reconnectflowchart TD
A["add_supported_lang(menu)"] --> B["keeb.enumerate_lang"]
B --> C{"result is True?"}
C -->|No| Z["Return"]
C -->|Yes| D["get_current_lang"]
D --> E["Build title from current_lang"]
E --> F{"keeb_lang_menu is None?"}
F -->|Yes| G["keeb_lang_menu = menu.addMenu(icon, title)"]
F -->|No| H["keeb_lang_menu.setTitle(title)"]
H --> I["keeb_lang_menu.clear"]
G --> J["all_languages = keeb.get_lang_list"]
I --> J
J --> K["Populate language actions"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Warning Review limit reached
More reviews will be available in 51 minutes and 17 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds resilience to firmware language enumeration by replacing assertion-based prefix validation with graceful early termination when multi-packet reads encounter unexpected data. The host menu refresh logic is updated to repopulate the language menu on reconnect. Comprehensive unit tests validate multi-packet reading behavior, error recovery, and full firmware language coverage. ChangesLanguage Enumeration and Menu Refresh
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="tests/device/poly_kybd_test.py" line_range="73-82" />
<code_context>
+ self.assertIn("enUS", langs)
+ self.assertNotIn("bgBG", langs) # P2 was never read
+
+ def test_unexpected_prefix_stops_loop(self):
+ keeb = _make_keeb()
+ junk = _pad(b"X\x00.garbage")
+ keeb.hid.send_and_read_validate_with_lock.return_value = (True, _P1, None)
+ keeb.hid.read_with_lock.side_effect = [(True, junk, None)]
+ ok, _ = keeb.enumerate_lang()
+ self.assertTrue(ok)
+ langs = keeb.get_lang_list()
+ self.assertIn("enUS", langs) # P1 was processed
+ self.assertNotIn("bgBG", langs) # loop stopped before P2
</code_context>
<issue_to_address>
**suggestion (testing):** Add a case where the unexpected prefix appears after at least one valid follow-up packet.
Right now the test only exercises a bad prefix on the first follow-up packet. To fully validate the guard, please add a variant where P2 or P3 is valid and a later packet has the wrong prefix (e.g., P2 ok, P3 junk), so we confirm earlier languages are retained and the loop stops immediately on the mid-stream bad prefix.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Addresses Sourcery review suggestion: verify that when a junk packet arrives after at least one valid follow-up (P2 ok, P3 junk), languages from the valid packets are retained and the loop stops immediately. https://claude.ai/code/session_01GhZQeFcV4JFnxdr8K1YGx9
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
polyhost/host.py (1)
481-517:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMenu refresh is still gated by connection-state flips.
At Line 481,
if connected_now != self.connected:prevents Line 517 from running onTrue -> Truereconnect cycles, so language menu refresh can be skipped even when the attached keyboard changes without an observed disconnected interval.Suggested direction
- if connected_now != self.connected: + if connected_now != self.connected: self.connected, msg = self.keeb.query_version_info() if self.connected: ... if compatible: self.add_supported_lang(self.menu) + elif connected_now: + # Refresh menu for same-state reconnect cycles (e.g., device swap not observed as disconnected). + # Consider guarding with device identity/version signature to avoid unnecessary rebuilds. + self.add_supported_lang(self.menu)🤖 Prompt for 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. In `@polyhost/host.py` around lines 481 - 517, The connection-check branch uses "if connected_now != self.connected:" which prevents refreshing the language menu when the device remains connected but the attached keyboard or its capabilities change; modify the logic so that when the keyboard is determined compatible (compatible is True and self.connected is True) you always call self.add_supported_lang(self.menu) regardless of whether connected_now differs from self.connected (either move the add_supported_lang(self.menu) call out of the connected_now != self.connected guard or add an explicit call in the compatible True path), keeping the existing compatibility checks and status updates intact.
🤖 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 `@tests/device/poly_kybd_test.py`:
- Line 36: The parameter annotation for extra_packets in _setup_reads uses an
implicit Optional (list[bytes] = None); update the signature to use an explicit
nullable type such as extra_packets: list[bytes] | None = None (or
extra_packets: Optional[list[bytes]] = None) and add the corresponding typing
import (Optional) if you choose that form; keep the parameter name _setup_reads
and extra_packets unchanged and ensure any stub/type checks still accept None.
---
Outside diff comments:
In `@polyhost/host.py`:
- Around line 481-517: The connection-check branch uses "if connected_now !=
self.connected:" which prevents refreshing the language menu when the device
remains connected but the attached keyboard or its capabilities change; modify
the logic so that when the keyboard is determined compatible (compatible is True
and self.connected is True) you always call self.add_supported_lang(self.menu)
regardless of whether connected_now differs from self.connected (either move the
add_supported_lang(self.menu) call out of the connected_now != self.connected
guard or add an explicit call in the compatible True path), keeping the existing
compatibility checks and status updates intact.
🪄 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: 9ca26af0-7b51-4dfd-9fd6-780cc4c8547d
📒 Files selected for processing (4)
polyhost/device/poly_kybd.pypolyhost/host.pytests/device/poly_kybd_mock_test.pytests/device/poly_kybd_test.py
|
|
||
| class TestEnumerateLangMultiPacket(unittest.TestCase): | ||
|
|
||
| def _setup_reads(self, keeb: PolyKybd, extra_packets: list[bytes] = None): |
Contributor
There was a problem hiding this comment.
Use explicit optional typing for extra_packets on Line 36.
list[bytes] = None triggers Ruff RUF013; use list[bytes] | None (or Optional[list[bytes]]) for explicit nullability.
Suggested patch
- def _setup_reads(self, keeb: PolyKybd, extra_packets: list[bytes] = None):
+ def _setup_reads(self, keeb: PolyKybd, extra_packets: list[bytes] | None = None):#!/bin/bash
# Verify there are no remaining implicit-Optional annotations in test files.
rg -nP --type=py ':\s*[^=\n]+=\s*None' tests🧰 Tools
🪛 Ruff (0.15.15)
[warning] 36-36: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
🤖 Prompt for 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.
In `@tests/device/poly_kybd_test.py` at line 36, The parameter annotation for
extra_packets in _setup_reads uses an implicit Optional (list[bytes] = None);
update the signature to use an explicit nullable type such as extra_packets:
list[bytes] | None = None (or extra_packets: Optional[list[bytes]] = None) and
add the corresponding typing import (Optional) if you choose that form; keep the
parameter name _setup_reads and extra_packets unchanged and ensure any stub/type
checks still accept None.
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
2 participants
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.
The while loop in enumerate_lang correctly reads all N response packets
from the firmware (GET_LANG_LIST, case 8, sends 4 back-to-back). Replace
the assert — which is silently disabled with python -O — with a logged
warning + break so a stale or mismatched reply stops the loop cleanly
rather than crashing in debug and passing corrupt data in optimised mode.
Add tests/device/poly_kybd_test.py to exercise the multi-packet reading
path with a mocked HID interface: verifies all 4 packets are consumed,
all expected language codes end up in all_languages, and that early
timeout or unexpected prefix stops the loop without crashing.
https://claude.ai/code/session_01GhZQeFcV4JFnxdr8K1YGx9
Summary by Sourcery
Improve language enumeration robustness and test coverage for the Poly keyboard firmware integration.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Bug Fixes
Improvements
Tests