Implement node-reset before unregister #337
Conversation
WalkthroughNode removal/reset flow updated: NodeReset handling added with NODE_RESET_ACK, request/response typing and payload construction adjusted, registry.unregister_node now accepts a PlugwiseNode and performs reset + cache clearing, tests extended for reset/remove, and version/changelog bumped. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Stick
participant Net as StickNetwork
participant Reg as StickNetworkRegister
participant Node as PlugwiseNode
participant USB as USBLayer
User->>Stick: unregister_node(mac)
Stick->>Net: unregister_node(mac)
Net->>Net: validate MAC & lookup node_to_remove
Net->>Reg: unregister_node(node_to_remove)
Reg->>USB: send NodeRemoveRequest(node.mac)
USB-->>Reg: NodeResponse (accepted)
Reg->>Node: await node.reset_node()
Node->>USB: send NodeResetRequest(node.mac, node_type, timeout=4s)
USB-->>Node: NodeResponse(NODE_RESET_ACK / NACK)
alt ACK received
Node-->>Reg: reset_node completes
else no ACK
Node-->>Reg: warn about missing ACK
end
Reg->>Reg: remove_network_registration(node.mac)
Reg->>Node: await node.clear_cache()
Reg-->>Net: unregister complete
Net-->>Stick: done
Stick-->>User: completed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #337 +/- ##
==========================================
+ Coverage 81.25% 81.78% +0.52%
==========================================
Files 36 36
Lines 8110 8126 +16
==========================================
+ Hits 6590 6646 +56
+ Misses 1520 1480 -40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
plugwise_usb/messages/responses.py (1)
62-67: NODE_RESET_ACK mapping added — good; consider documenting/adding NACK when knownb"00F2" for NODE_RESET_ACK looks consistent with the reset flow. If/when a negative ACK is identified, add it to keep parity with other enums and avoid ValueError surprises when formatting response types.
plugwise_usb/network/__init__.py (1)
178-180: Use the already-fetched node and make pop idempotent.Minor cleanup and safety.
- await self._nodes[mac].unload() - self._nodes.pop(mac) + await node_to_remove.unload() + self._nodes.pop(mac, None)plugwise_usb/network/registry.py (1)
269-289: Flow LGTM; minor style and resilience suggestions.
- Consider f-strings only (avoid `+ f"...") for readability.
- If reset fails (warning only), removal proceeds—good. If you want hard-fail semantics, wrap
await node.reset_node()and decide policy explicitly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
plugwise_usb/api.py(1 hunks)plugwise_usb/messages/requests.py(1 hunks)plugwise_usb/messages/responses.py(1 hunks)plugwise_usb/network/__init__.py(1 hunks)plugwise_usb/network/registry.py(2 hunks)plugwise_usb/nodes/node.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
plugwise_usb/api.py (1)
plugwise_usb/nodes/node.py (1)
reset_node(644-656)
plugwise_usb/nodes/node.py (4)
plugwise_usb/messages/requests.py (19)
NodeInfoRequest(884-913)NodePingRequest(592-622)NodeResetRequest(480-513)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(504-513)send(534-545)send(558-567)send(580-589)send(613-622)send(659-668)send(711-720)send(767-776)send(800-809)send(840-849)send(872-881)plugwise_usb/api.py (2)
reset_node(496-508)NodeType(62-73)plugwise_usb/messages/responses.py (2)
node_type(635-637)ack_id(135-137)tests/test_usb.py (1)
send(251-257)
plugwise_usb/network/__init__.py (5)
plugwise_usb/helpers/util.py (1)
validate_mac(14-26)plugwise_usb/nodes/node.py (1)
mac(235-237)plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/network/registry.py (1)
unregister_node(269-288)plugwise_usb/__init__.py (1)
unregister_node(333-340)
plugwise_usb/network/registry.py (6)
plugwise_usb/api.py (4)
NodeType(62-73)PlugwiseNode(309-794)mac(354-355)reset_node(496-508)plugwise_usb/network/__init__.py (1)
unregister_node(167-179)plugwise_usb/__init__.py (1)
unregister_node(333-340)plugwise_usb/nodes/node.py (2)
mac(235-237)reset_node(644-656)plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/messages/requests.py (18)
NodeRemoveRequest(852-881)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(504-513)send(534-545)send(558-567)send(580-589)send(613-622)send(659-668)send(711-720)send(767-776)send(800-809)send(840-849)send(872-881)send(904-913)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and publish Python 🐍 distributions 📦 to TestPyPI
🔇 Additional comments (2)
plugwise_usb/api.py (1)
496-509: Polish docstring & verify implementers
- Fix docstring typo and move the warning to Notes — apply the diff below.
- Adding a Protocol method is a breaking change for downstream implementers; confirm all concrete node classes implement reset_node. Found an implementation at plugwise_usb/nodes/node.py:644; run the checks below to verify the rest.
- async def reset_node(self) -> None: - """Reset a node. - - Description: - Resets the mode before unregistering it from the Plugwise network. - - Returns: - None - - Raised: - A warning logger message. - - """ + async def reset_node(self) -> None: + """Reset a node before unregistering it from the Plugwise network. + + Notes: + May log a warning if the node does not acknowledge the reset. + """Verification commands:
rg -nP --type=py -C2 'async\s+def\s+reset_node\s*\(' rg -nP --type=py -C3 '\bclass\b.+\((?:.+PlugwiseNode.+)\)'plugwise_usb/network/registry.py (1)
11-11: Import update looks correct.Switching to
PlugwiseNodein this module aligns with the new reset-before-remove flow.
bouwew
force-pushed
the
improve-unregister-2
branch
from
45b57aa to
c2ebe63
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/messages/requests.py (1)
504-513: Correct: NodeResetRequest now expects NodeResponseGood fix switching send() to return NodeResponse and validating via isinstance(result, NodeResponse). This aligns with NODE_RESET_ACK semantics.
🧹 Nitpick comments (6)
plugwise_usb/messages/requests.py (2)
490-502: Normalize moduletype to support IntEnum inputsCast moduletype via getattr(..., "value", ...) so callers can pass NodeType directly without remembering .value.
- self._args += [ - Int(moduletype, length=2), - Int(timeout, length=2), - ] + module_id = getattr(moduletype, "value", moduletype) + self._args += [Int(module_id, length=2), Int(timeout, length=2)]
484-489: Tidy reply-identifier note (typo) or make it explicit in codeThe comment has a quote typo (0000'with). Either fix the comment or make intent self-documenting by explicitly setting _reply_identifier.
- # _reply_identifier is b"0000'with NODE_RESET_ACK, not b"0003" @dirixmjm & @bouwew 20250910 + # Node reset replies via NodeResponse (b"0000") with NODE_RESET_ACK/NACK. + _reply_identifier = b"0000"tests/stick_test_data.py (2)
1347-1354: Fix MAC length in human-readable labelsThe descriptive strings show 18 nibbles; MAC here is 16. Purely cosmetic but avoids confusion.
- "Reset node request for 333333333333333333", + "Reset node request for 3333333333333333",
1352-1359: Likewise fix label for remove entry- "Remove node request for 333333333333333333", + "Remove node request for 3333333333333333",tests/test_usb.py (1)
1741-1744: Stable on-disk nodetype encodingExpecting numeric codes is fine now that NodeType is IntEnum. Ensure the cache writer emits entries in a deterministic order (sorted by key) so this assertion stays stable across runs.
plugwise_usb/network/registry.py (1)
269-290: Unregister flow looks solid; consider semantic status checkReset-then-remove with clear_cache is correct. Minor: if a semantic enum exists for NodeRemoveResponse.status, prefer comparing to it over magic 1 for readability.
- if response.status.value != 1: + # e.g., if NodeRemoveStatus.SUCCESS exists: + # if response.status is not NodeRemoveStatus.SUCCESS: + if response.status.value != 1: raise NodeError(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
plugwise_usb/api.py(1 hunks)plugwise_usb/messages/requests.py(2 hunks)plugwise_usb/messages/responses.py(1 hunks)plugwise_usb/network/__init__.py(1 hunks)plugwise_usb/network/registry.py(2 hunks)plugwise_usb/nodes/node.py(2 hunks)tests/stick_test_data.py(1 hunks)tests/test_usb.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- plugwise_usb/api.py
- plugwise_usb/network/init.py
- plugwise_usb/nodes/node.py
- plugwise_usb/messages/responses.py
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_usb.py (3)
plugwise_usb/network/__init__.py (2)
unregister_node(167-182)nodes(142-146)plugwise_usb/network/registry.py (1)
unregister_node(269-289)plugwise_usb/__init__.py (2)
unregister_node(333-340)nodes(95-99)
plugwise_usb/network/registry.py (5)
plugwise_usb/api.py (3)
PlugwiseNode(311-782)mac(356-357)clear_cache(405-406)plugwise_usb/network/__init__.py (2)
unregister_node(167-182)clear_cache(163-165)plugwise_usb/nodes/node.py (3)
mac(235-237)reset_node(644-663)clear_cache(391-394)plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/messages/requests.py (18)
NodeRemoveRequest(852-881)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(504-513)send(534-545)send(558-567)send(580-589)send(613-622)send(659-668)send(711-720)send(767-776)send(800-809)send(840-849)send(872-881)send(904-913)
plugwise_usb/messages/requests.py (4)
tests/test_usb.py (1)
send(261-269)plugwise_usb/connection/__init__.py (1)
send(227-238)plugwise_usb/messages/responses.py (1)
NodeResponse(245-272)plugwise_usb/exceptions.py (1)
MessageError(20-21)
🔇 Additional comments (2)
tests/stick_test_data.py (1)
1347-1359: Additions look correct and align with reset/remove flowsThe reset (0009 -> 0000/00F2) and remove (001C -> 001D status=01) entries match the wire format and will exercise the new path.
tests/test_usb.py (1)
723-726: Good: end-to-end unregister testCovers reset+remove flow via public API and validates node map updates.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise_usb/messages/requests.py (1)
490-501: Validate moduletype/timeout to prevent malformed frames.With Int(length=2), values > 0xFF will serialize to more than 2 hex chars and break framing/CRC. Guard the range and accept IntEnum cleanly.
- def __init__( + def __init__( self, send_fn: Callable[[PlugwiseRequest, bool], Awaitable[PlugwiseResponse | None]], mac: bytes, - moduletype: int, + moduletype: int, timeout: int, ) -> None: """Initialize NodeResetRequest message object.""" super().__init__(send_fn, mac) - module_id = getattr(moduletype, "value", moduletype) - self._args += [Int(module_id, length=2), Int(timeout, length=2)] + module_id = int(getattr(moduletype, "value", moduletype)) + if not (0 <= module_id <= 0xFF): + raise MessageError(f"Invalid moduletype value: {module_id} (expected 0..255)") + if not (0 <= timeout <= 0xFF): + raise MessageError(f"Invalid timeout value: {timeout} (expected 0..255)") + self._args += [Int(module_id, length=2), Int(timeout, length=2)] + # Reset commands are side-effectful; keep retries conservative. + self._max_retries = min(self._max_retries, 1)
🧹 Nitpick comments (3)
tests/stick_test_data.py (1)
1352-1359: Nit: unify comment wording (“Circle+ mac”).Minor consistency polish with existing comments elsewhere.
- + b"0098765432101234" # Circle + mac + + b"0098765432101234" # Circle+ macplugwise_usb/messages/requests.py (2)
484-485: Clarify docstring with explicit ack identifier.Spell out that the reply is NodeResponse (identifier 0000) with ack 00F2 (NODE_RESET_ACK).
- Response message : NodeResponse with NODE_RESET_ACK/NACK + Response message : NodeResponse (identifier b"0000") with ack b"00F2" (NODE_RESET_ACK).
488-489: Make reply identifier explicit (avoid future regressions).Set _reply_identifier = b"0000" instead of relying on the base default; remove the inline comment.
- # _reply_identifier is b"0000'with NODE_RESET_ACK, not b"0003" @dirixmjm & @bouwew 20250910 + _reply_identifier = b"0000"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugwise_usb/messages/requests.py(2 hunks)plugwise_usb/messages/responses.py(1 hunks)plugwise_usb/nodes/node.py(2 hunks)tests/stick_test_data.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugwise_usb/messages/responses.py
- plugwise_usb/nodes/node.py
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/messages/requests.py (5)
plugwise_usb/messages/properties.py (1)
Int(103-129)tests/test_usb.py (1)
send(261-269)plugwise_usb/connection/__init__.py (1)
send(227-238)plugwise_usb/messages/responses.py (1)
NodeResponse(244-271)plugwise_usb/exceptions.py (1)
MessageError(20-21)
🔇 Additional comments (2)
tests/stick_test_data.py (1)
1347-1351: LGTM: Reset-node test vector matches updated protocol (0009 → 0000/NODE_RESET_ACK).The frame layout and response payload look correct for NODE_RESET_ACK (00F2).
plugwise_usb/messages/requests.py (1)
502-511: LGTM: send() now expects NodeResponse.Type handling and error message align with the protocol.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
plugwise_usb/messages/requests.py (3)
484-489: Docstring and comment polish for NodeResetRequestTighten wording and fix the stray quote in the comment.
class NodeResetRequest(PlugwiseRequest): - """TODO:Some kind of reset request. + """Reset a node in the Plugwise network. Supported protocols : 1.0, 2.0, 2.1 - Response message : NodeResponse with NODE_RESET_ACK/NACK + Response message : NodeResponse (ack=NODE_RESET_ACK) """ @@ - # _reply_identifier is b"0000'with NODE_RESET_ACK, not b"0003" @dirixmjm & @bouwew 20250910 + # Replies via NodeResponse (identifier b"0000") with ack=NODE_RESET_ACK; not NodeSpecificResponse (b"0003").
499-501: Add light bounds-checking for moduletype/timeoutDefend against out-of-range values before serializing into 2-byte Ints.
- module_id = getattr(moduletype, "value", moduletype) - self._args += [Int(module_id, length=2), Int(timeout, length=2)] + module_id = getattr(moduletype, "value", moduletype) + module_id = int(module_id) + if not (0 <= module_id <= 0xFFFF): + raise MessageError(f"Invalid module type: {module_id!r}") + if not (0 <= int(timeout) <= 0xFFFF): + raise MessageError(f"Invalid timeout: {timeout!r}") + self._args += [Int(module_id, length=2), Int(timeout, length=2)]
864-869: Rename verified — no leftover mac_to_unjoined; optional MAC normalization recommended
- Verified call sites: plugwise_usb/messages/requests.py (NodeRemoveRequest class at ~line 850), plugwise_usb/network/registry.py:276 (NodeRemoveRequest(..., node.mac)), tests/test_usb.py:1483–1484 (test passes b"1111222233334444" and "5555666677778888").
- No matches for the old name mac_to_unjoined were found.
- Optional refactor (safe normalization, handle bytes/str before appending):
mac = mac_to_unjoin
if isinstance(mac, (bytes, bytearray)): mac = mac.decode()
mac = str(mac).upper()
if len(mac) != 16: raise MessageError(f"mac_to_unjoin must be 16 hex characters, got {len(mac)}")
self._args.append(String(mac, length=16))plugwise_usb/network/cache.py (3)
34-34: Don't force full rewrites on every save; make it caller-controlled.Always passing rewrite=True can cause unnecessary disk churn on routine updates. Prefer parameterizing save_cache so prune/remove flows can request a rewrite, while simple updates can merge/append.
Apply:
- async def save_cache(self) -> None: + async def save_cache(self, rewrite: bool = False) -> None: @@ - await self.write_cache(cache_data_to_save, True) # rewrite=True is required + await self.write_cache(cache_data_to_save, rewrite)And call with rewrite=True only where needed (e.g., after pruning):
@@ - await self.save_cache() + await self.save_cache(True)
33-33: Nit: no need to cast len() to str in logging.Apply:
- _LOGGER.debug("Save NodeTypes %s", str(len(cache_data_to_save))) + _LOGGER.debug("Save NodeTypes %s", len(cache_data_to_save))
29-33: Cache format robustness — save numeric IntEnum; load numeric and legacy stringsNodeType is an IntEnum (plugwise_usb/api.py) and tests expect legacy "NodeType.NAME" strings (tests/test_usb.py). Change saving to store numeric values (node_value = str(int(node_type))) and update restore_cache to accept:
- digits → NodeType(int(...))
- "NodeType.NAME" → NodeType[NAME]
- plain "NAME" → NodeType[NAME]
Keep invalid/"None" handling. Update tests that assert written cache (mock_file_stream.writelines) to expect the new numeric string format.Location: plugwise_usb/network/cache.py (save & restore); tests: tests/test_usb.py (write assertions around ~1735).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)plugwise_usb/messages/requests.py(3 hunks)plugwise_usb/network/cache.py(1 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-30T10:46:42.649Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.649Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/network/cache.py
🧬 Code graph analysis (1)
plugwise_usb/messages/requests.py (5)
plugwise_usb/messages/properties.py (2)
Int(103-129)String(83-100)tests/test_usb.py (1)
send(261-269)plugwise_usb/connection/__init__.py (1)
send(227-238)plugwise_usb/messages/responses.py (1)
NodeResponse(244-271)plugwise_usb/exceptions.py (1)
MessageError(20-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run pytest using Python 3.13
🔇 Additional comments (1)
plugwise_usb/messages/requests.py (1)
499-511: Enum normalization and NodeResponse handling — LGTMCasting moduletype via getattr(..., "value", ...) and switching send() to NodeResponse resolves the previous mismatch and aligns with subscription filtering on b"0000".
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
plugwise_usb/network/registry.py (2)
274-274: Comment typo and intentTypo: “succesful”. With the reorder, this comment should reflect “Reset first, then unregister”.
281-285: Replace magic status '1' with a named constant/enum.NodeRemoveResponse.status is Int(0,2) (plugwise_usb/messages/responses.py:579); add a named constant (e.g. NodeRemoveStatus.SUCCESS = 1 or NodeRemoveResponse.STATUS_SUCCESS = 1) and use it in plugwise_usb/network/registry.py:281–285 instead of the literal
1.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugwise_usb/network/cache.py(1 hunks)plugwise_usb/network/registry.py(2 hunks)plugwise_usb/nodes/node.py(3 hunks)pyproject.toml(1 hunks)tests/test_usb.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- plugwise_usb/network/cache.py
- pyproject.toml
- tests/test_usb.py
- plugwise_usb/nodes/node.py
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/network/registry.py (5)
plugwise_usb/api.py (3)
PlugwiseNode(311-782)mac(356-357)clear_cache(405-406)plugwise_usb/network/__init__.py (2)
unregister_node(167-182)clear_cache(163-165)plugwise_usb/nodes/node.py (3)
mac(235-237)reset_node(645-657)clear_cache(391-395)plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/messages/requests.py (18)
NodeRemoveRequest(850-879)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(502-511)send(532-543)send(556-565)send(578-587)send(611-620)send(657-666)send(709-718)send(765-774)send(798-807)send(838-847)send(870-879)send(902-911)
🔇 Additional comments (2)
plugwise_usb/network/registry.py (2)
11-11: Add async reset_node to PlugwiseNode Protocol (plugwise_usb/api.py)Add
async def reset_node(self) -> Awaitable[None]to the class PlugwiseNode in plugwise_usb/api.py — the Protocol currently lacks this method and type-checkers will flag calls like unregister_node.⛔ Skipped due to learnings
Learnt from: dirixmjm PR: plugwise/python-plugwise-usb#302 File: plugwise_usb/network/registry.py:49-52 Timestamp: 2025-08-09T16:24:49.861Z Learning: In the plugwise_usb codebase, the `_start_node_discover` callback in `plugwise_usb/network/registry.py` must return `Awaitable[bool]`. The return value is used in `plugwise_usb/network/__init__.py` in the `node_awake_message` and `node_rejoin_message` handlers to determine whether to fall back to battery-powered node discovery. When `_discover_node` returns `False`, it triggers `_discover_battery_powered_node` for SED devices.Learnt from: dirixmjm PR: plugwise/python-plugwise-usb#302 File: plugwise_usb/network/registry.py:49-52 Timestamp: 2025-08-09T16:24:49.861Z Learning: In the plugwise_usb codebase, the `_start_node_discover` callback in `plugwise_usb/network/registry.py` must return `Awaitable[bool]`. The return value is used in `plugwise_usb/network/__init__.py` in the `node_awake_message` and `node_rejoin_message` handlers to determine whether to fall back to battery-powered node discovery. When `_discover_node` returns `False`, it triggers `_discover_battery_powered_node` for SED devices.Learnt from: dirixmjm PR: plugwise/python-plugwise-usb#302 File: plugwise_usb/network/registry.py:49-52 Timestamp: 2025-08-09T16:24:49.861Z Learning: In the plugwise_usb codebase, the `_start_node_discover` callback in `plugwise_usb/network/registry.py` must return `Awaitable[bool]`. The return value is used in `plugwise_usb/network/__init__.py` to track discovery success. In `_discover_battery_powered_node`, it checks `if not await self._discover_node(...)` to return False on failure. In the stragglers retry loop, it checks the return value to remove successfully discovered nodes from the retry list. This supports different handling approaches for regular nodes vs SED devices.Learnt from: dirixmjm PR: plugwise/python-plugwise-usb#302 File: plugwise_usb/network/registry.py:49-52 Timestamp: 2025-08-09T16:24:49.861Z Learning: In the plugwise_usb codebase, the `_start_node_discover` callback in `plugwise_usb/network/registry.py` must return `Awaitable[bool]`. The return value is used in `plugwise_usb/network/__init__.py` to track discovery success. In `_discover_battery_powered_node`, it checks `if not await self._discover_node(...)` to return False on failure. In the stragglers retry loop, it checks the return value to remove successfully discovered nodes from the retry list. This supports different handling approaches for regular nodes vs SED devices.Learnt from: dirixmjm PR: plugwise/python-plugwise-usb#302 File: plugwise_usb/network/registry.py:49-52 Timestamp: 2025-08-09T16:24:49.861Z Learning: In the plugwise_usb codebase, the `_start_node_discover` callback in `plugwise_usb/network/registry.py` must return `Awaitable[bool]`. The return value is used in `plugwise_usb/network/__init__.py` to track discovery success. The `_discover_battery_powered_node` method checks this return value to determine if discovery succeeded. There are different approaches for regular nodes (discovered through registry scans) and SED devices (discovered when they wake up through awake/rejoin handlers).
269-270: API change — registry.unregister_node now takes PlugwiseNode; callers verifiedNo direct callers pass a string literal to plugwise_usb/network/registry.py::unregister_node — plugwise_usb/network/init.py calls
await self._register.unregister_node(node_to_remove)(correct). tests/test_usb.py still calls the publicstick.unregister_node("3333333333333333")(wrapper converts mac→node).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise_usb/network/cache.py (1)
48-57: Harden restore parsing; remove brittle length check and unreachableKeyErrorpath.
len(node_value) >= 12and slicing[9:]are fragile; usestartswith("NodeType.").NodeType.__members__.get(...)never raisesKeyError; theexcept KeyErroris dead code.- Prefer name-based parsing first (new format), then numeric fallback for backward compatibility.
Apply this diff:
- try: - node_type = NodeType(int(node_value)) # e.g. "2" - except ValueError: - tmp_node_value = node_value - if len(node_value) >= 12: # e.g. "NodeType.CIRCLE" - tmp_node_value = node_value[9:] - try: - # node_value is e.g. "CIRCLE" - node_type = NodeType.__members__.get(tmp_node_value) - except KeyError: - node_type = None + # Backward-compatible parsing: prefer enum names, then numeric values + val = node_value.strip() + key = val.split(".", 1)[1] if val.startswith("NodeType.") else val + node_type = NodeType.__members__.get(key) # e.g., "CIRCLE" + if node_type is None and val.isdigit(): # e.g., "2" + try: + node_type = NodeType(int(val)) + except ValueError: + node_type = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/network/cache.py(2 hunks)tests/test_usb.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_usb.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-30T10:46:42.649Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.649Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/network/cache.py
🧬 Code graph analysis (1)
plugwise_usb/network/cache.py (2)
plugwise_usb/api.py (2)
name(328-329)NodeType(62-73)plugwise_usb/helpers/cache.py (1)
write_cache(82-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Process test coverage
🔇 Additional comments (2)
plugwise_usb/network/cache.py (2)
31-31: Serialize enum via.name— good choice for stable cache format.This aligns the on-disk representation with token-like names and eases future parsing.
33-35: Forcerewrite=Trueto prevent stale entries — agreed.Full rewrite avoids dangling MACs after removals; the inline comment documents the intent.
bouwew
force-pushed
the
improve-unregister-2
branch
from
2e1fad4 to
951b25c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugwise_usb/network/cache.py (2)
48-57: BC parsing LGTM. Consider case-insensitive names and opportunistic migration.Small improvements:
- Accept lower/ mixed-case enum tokens from legacy files.
- Optionally detect legacy formats and resave once to the new
.nameformat.Apply this minimal diff to accept case-insensitive names:
- val = node_value.strip() - key = val.split(".", 1)[1] if val.startswith("NodeType.") else val - node_type = NodeType.__members__.get(key) # e.g., "CIRCLE" + val = node_value.strip() + # Accept "NodeType.CIRCLE", "circle", or "CIRCLE" + raw_key = val.split(".", 1)[1] if val.startswith("NodeType.") else val + node_type = NodeType.__members__.get(raw_key) or NodeType.__members__.get( + raw_key.upper() + ) # e.g., "CIRCLE" if node_type is None and val.isdigit(): # e.g., "2" try: node_type = NodeType(int(val)) except ValueError: node_type = NoneIf you want one-time migration to the new format, you can (outside the changed lines) track a
needs_rewriteflag and callawait self.save_cache()at the end ofrestore_cache()when legacy strings or numbers were seen:# Pseudocode to place near the start/end of restore_cache() needs_rewrite = False # inside loop: set needs_rewrite = needs_rewrite or val.startswith("NodeType.") or val.isdigit() # after loop: if needs_rewrite: await self.save_cache()
87-96: Avoid unnecessary writes when prune results are identical.Skip save if nothing changed to reduce I/O.
async def prune_cache(self, registry: list[str]) -> None: """Remove items from cache which are not found in registry scan.""" - new_nodetypes: dict[str, NodeType] = {} + old_nodetypes = self._nodetypes + new_nodetypes: dict[str, NodeType] = {} for mac in registry: if mac == "": continue if (node_type := self.get_nodetype(mac)) is not None: new_nodetypes[mac] = node_type - self._nodetypes = new_nodetypes - await self.save_cache() + if new_nodetypes != old_nodetypes: + self._nodetypes = new_nodetypes + await self.save_cache()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/network/cache.py(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-30T10:46:42.649Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.649Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/network/cache.py
🧬 Code graph analysis (1)
plugwise_usb/network/cache.py (2)
plugwise_usb/api.py (2)
name(328-329)NodeType(62-73)plugwise_usb/helpers/cache.py (1)
write_cache(82-121)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run pytest using Python 3.13
🔇 Additional comments (2)
plugwise_usb/network/cache.py (2)
31-31: Serialize NodeType using .name — good change.Storing the enum name improves readability and forward-compatibility versus numeric values or repr strings.
33-35: rewrite=True is safe — single writer confirmed. plugwise_usb/network/cache.py is the only module defining _NETWORK_CACHE_FILE_NAME and writing nodetype.cache; other hits are tests/mocks.
bouwew
force-pushed
the
improve-unregister-2
branch
from
514ae3b to
c9ffe52
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/messages/requests.py (1)
488-489: Set _reply_identifier explicitly to b"0000" (and fix the comment typo)Relying on the base-class default is fragile. Set it explicitly for clarity and resilience against future refactors; also fix the stray quote in the comment.
Apply:
- # _reply_identifier is b"0000'with NODE_RESET_ACK, not b"0003" @dirixmjm & @bouwew 20250910 + # Node reset replies via NodeResponse (identifier b"0000") with NODE_RESET_ACK/NACK. + _reply_identifier = b"0000"
🧹 Nitpick comments (1)
plugwise_usb/messages/requests.py (1)
499-501: Enum-safe moduletype normalization: good; consider widening the type hintNormalization via getattr(..., "value", ...) is correct. Consider widening the annotation to reflect accepted IntEnum/Enum inputs.
Apply:
- moduletype: int, + moduletype: int, # accepts int or (Int)Enum; normalized via `.value` aboveOptionally import and annotate explicitly (if you prefer strict typing):
- Add:
from enum import Enum, IntEnum- Change:
moduletype: int | IntEnum | Enum
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
CHANGELOG.md(1 hunks)plugwise_usb/messages/requests.py(3 hunks)plugwise_usb/messages/responses.py(1 hunks)plugwise_usb/network/__init__.py(1 hunks)plugwise_usb/network/registry.py(2 hunks)plugwise_usb/nodes/node.py(3 hunks)pyproject.toml(1 hunks)tests/stick_test_data.py(1 hunks)tests/test_usb.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- tests/test_usb.py
- pyproject.toml
- plugwise_usb/messages/responses.py
- plugwise_usb/network/registry.py
- plugwise_usb/network/init.py
- tests/stick_test_data.py
- CHANGELOG.md
- plugwise_usb/nodes/node.py
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/messages/requests.py (4)
plugwise_usb/messages/properties.py (2)
Int(103-129)String(83-100)plugwise_usb/connection/__init__.py (1)
send(227-238)plugwise_usb/messages/responses.py (1)
NodeResponse(244-271)plugwise_usb/exceptions.py (1)
MessageError(20-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (4)
plugwise_usb/messages/requests.py (4)
484-486: Docstring alignment with actual reply type looks goodAcknowledging that NodeReset responds via NodeResponse with NODE_RESET_ACK/NACK. Matches responses.py.
502-511: send() now expects NodeResponse: correctType and error-message updates are consistent with the new reply semantics.
864-869: No internal uses ofmac_to_unjoined— alias not requiredNo occurrences of the old kwarg were found; NodeRemoveRequest is called with positional args at plugwise_usb/network/registry.py:275 and tests/test_usb.py:1483–1485.
Likely an incorrect or invalid review comment.
499-501: No issue — moduletype fits into Int(length=2)NodeType members in plugwise_usb/api.py are 0..9 (all < 0xFF); Int(length=2) serializes as two hex digits ("%02X", max 0xFF), so no truncation risk. Location: plugwise_usb/api.py (class NodeType).
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/messages/requests.py (1)
487-489: Be explicit about the reply identifier to future‑proof behaviorDefault is b"0000" today, but setting it here avoids regressions if the base default changes later.
_identifier = b"0009" + _reply_identifier = b"0000"
🧹 Nitpick comments (2)
plugwise_usb/messages/requests.py (2)
481-486: Docstring: replace TODO with a precise descriptionMake the purpose explicit; keep the ACK/NACK note below.
- """TODO:Some kind of reset request. + """Reset a node in the current Plugwise network.
493-495: Validate field ranges and accept IntEnum in the signature
- Guard against values overflowing the 1‑byte fields (length=2 hex chars).
- Annotate that moduletype may be an IntEnum (NodeType).
- moduletype: int, + moduletype: int | "IntEnum", timeout: int, @@ super().__init__(send_fn, mac) module_id = getattr(moduletype, "value", moduletype) - self._args += [Int(module_id, length=2), Int(timeout, length=2)] + if not (0 <= int(module_id) <= 0xFF): + raise MessageError(f"moduletype out of range (0..255): {module_id!r}") + if not (0 <= int(timeout) <= 0xFF): + raise MessageError(f"timeout out of range (0..255): {timeout!r}") + self._args += [Int(int(module_id), length=2), Int(int(timeout), length=2)]Add near the imports:
from enum import IntEnum # for type hintAlso applies to: 498-500
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/messages/requests.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/messages/requests.py (5)
plugwise_usb/messages/properties.py (2)
Int(103-129)String(83-100)tests/test_usb.py (1)
send(261-269)plugwise_usb/connection/__init__.py (1)
send(227-238)plugwise_usb/messages/responses.py (1)
NodeResponse(244-271)plugwise_usb/exceptions.py (1)
MessageError(20-21)
🔇 Additional comments (2)
plugwise_usb/messages/requests.py (2)
501-510: LGTM: reply handling now matches NodeResponse
send()correctly expects NodeResponse and errors accordingly.
859-868: Add a backward-compatible shim for the renamed constructor arg mac_to_unjoined → mac_to_unjoinRenaming this keyword is a breaking API change for external callers; repo search found no internal keyword uses (calls are positional in tests/test_usb.py:1483–1485 and plugwise_usb/network/registry.py:275). Add the legacy-accepting shim below.
- def __init__( + def __init__( self, send_fn: Callable[[PlugwiseRequest, bool], Awaitable[PlugwiseResponse | None]], mac_circle_plus: bytes, - mac_to_unjoin: str, + mac_to_unjoin: str | None = None, + **kwargs: Any, ) -> None: """Initialize NodeRemoveRequest message object.""" super().__init__(send_fn, mac_circle_plus) - self._args.append(String(mac_to_unjoin, length=16)) + legacy = kwargs.pop("mac_to_unjoined", None) + if legacy is not None: + _LOGGER.warning( + "NodeRemoveRequest: 'mac_to_unjoined' is deprecated; use 'mac_to_unjoin'" + ) + mac_to_unjoin = mac_to_unjoin or legacy + if mac_to_unjoin is None: + raise MessageError("mac_to_unjoin is required") + self._args.append(String(mac_to_unjoin, length=16))
Implement both node-removal and node-resetting as in Source.
Summary by CodeRabbit
New Features
Improvements
Tests
Documentation
Chores