Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR extends the CircleRelayInitStateRequest messaging system to support dual reply identifiers (0139 and 0100). The PlugwiseRequest framework now handles multiple possible responses, and the Circle node class is updated to interpret both CircleRelayInitStateResponse and NodeAckResponse types with improved error handling. Changes
Sequence DiagramsequenceDiagram
participant Circle as Circle Node
participant Request as CircleRelayInitStateRequest
participant Framework as PlugwiseRequest<br/>(subscribe_to_response)
participant Response as Response Handler
Circle->>Request: _relay_init_get()
Request->>Request: create with _reply_identifier(0139)<br/>+ _reply_identifier_2(0100)
Request->>Framework: subscribe_to_response(combined_ids)
Framework->>Framework: filter + combine identifiers<br/>(0139, 0100)
Framework->>Response: wait for response matching<br/>0139 OR 0100
alt Receives 0139 (CircleRelayInitStateResponse)
Response->>Circle: return CircleRelayInitStateResponse
Circle->>Circle: extract + return boolean state
else Receives 0100 (NodeAckResponse)
Response->>Circle: return NodeAckResponse
Circle->>Circle: handle as acknowledgment
else No response
Response->>Circle: return None
Circle->>Circle: return None or raise error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 #425 +/- ##
==========================================
- Coverage 82.01% 81.76% -0.25%
==========================================
Files 36 36
Lines 8177 8195 +18
==========================================
- Hits 6706 6701 -5
- Misses 1471 1494 +23 ☔ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugwise_usb/connection/__init__.py`:
- Around line 235-238: The except clause uses Python 2 syntax "except NodeError,
StickError" which fails under Python 3; update the exception handling in the
method that calls self._queue.submit to catch both exceptions by parenthesizing
them (i.e., use except (NodeError, StickError):) and keep the existing return
None behavior so the call returns None on either NodeError or StickError.
In `@plugwise_usb/messages/requests.py`:
- Around line 176-180: The tuple of reply identifiers passed to
node_subscription_fn currently includes None when there's only a single reply
(it uses (self._reply_identifier, self._reply_identifier_2)), which violates
subscribe_to_node_responses()'s expected type; change the call site where
self._unsubscribe_node_response is assigned so you filter out None values before
constructing the tuple (e.g., build a sequence from self._reply_identifier and
self._reply_identifier_2 and only pass a tuple of actual bytes, or pass None if
empty) so node_subscription_fn(process_node_response, self._mac,
<filtered-reply-identifiers>, self._seq_id) always receives either None or a
tuple[bytes, ...].
In `@plugwise_usb/nodes/circle.py`:
- Around line 1154-1163: The success branch handling
CircleRelayInitStateResponse currently continues execution and triggers the
unexpected-response warning and return, causing initialize() to see a failure;
after awaiting self._relay_init_update_state(response.relay.value == 1) (or
instead of falling through) return immediately (or else put the warning/return
in an else block) so that the parsed relay-init state is returned on success and
initialize() sees a successful read.
- Around line 1176-1189: The code treats the set reply as a state payload and
reads response.relay which doesn't exist on NodeAckResponse; update the logic in
the CircleRelayInitStateRequest handling to first guard the union typed response
(ensure response is a NodeAckResponse) before inspecting node_ack_type, and in
the DEFAULT_ACK branch use the originally requested state value (the request's
state parameter) to call self._relay_init_update_state(...) instead of reading
response.relay; keep the existing None check and DEFAULT_FAIL handling but avoid
accessing attributes that only exist on a different response type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 98e277e3-c242-4f63-8d4c-f8aea2c83daa
📒 Files selected for processing (4)
plugwise_usb/connection/__init__.pyplugwise_usb/messages/requests.pyplugwise_usb/messages/responses.pyplugwise_usb/nodes/circle.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
plugwise_usb/nodes/circle.py (2)
1158-1162:⚠️ Potential issue | 🔴 CriticalReturn the parsed relay-init state on the success path.
This branch still falls through to
None, soinitialize()treats a successful relay-init read as failure and aborts initialization for RELAY_INIT nodes.🐛 Proposed fix
if isinstance(response, CircleRelayInitStateResponse): + state = response.relay.value == 1 _LOGGER.debug("Successful get of relay init state for %s", self._mac_in_str) - await self._relay_init_update_state(response.relay.value == 1) + await self._relay_init_update_state(state) + return state🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise_usb/nodes/circle.py` around lines 1158 - 1162, The success branch for handling CircleRelayInitStateResponse currently returns None and so initialize() thinks the read failed; modify the CircleRelayInitStateResponse branch to return the parsed boolean state (response.relay.value == 1) after awaiting _relay_init_update_state so callers get a truthy success value; update the block that logs "Successful get of relay init state for %s" (using self._mac_in_str) to await self._relay_init_update_state(response.relay.value == 1) and then return that boolean instead of falling through to None, ensuring initialize() recognizes successful reads for RELAY_INIT nodes.
1185-1192:⚠️ Potential issue | 🔴 CriticalNarrow the union before reading ack fields, and persist the requested state on success.
CircleRelayInitStateRequest.send()now returnsCircleRelayInitStateResponse | NodeAckResponse | None, but this code immediately readsnode_ack_typeand thenresponse.relay. On the happy-path ack response,response.relaydoes not exist, so the state update can crash withAttributeError.🐛 Proposed fix
- if response.node_ack_type == NodeAckResponseType.DEFAULT_FAIL: + if not isinstance(response, NodeAckResponse): + _LOGGER.warning( + "Unexpected response type %s for %s", + response.__class__.__name__, + self._mac_in_str, + ) + return None + + if response.node_ack_type == NodeAckResponseType.DEFAULT_FAIL: _LOGGER.warning("Failed to set relay init state for %s", self._mac_in_str) return None if response.node_ack_type == NodeAckResponseType.DEFAULT_ACK: _LOGGER.debug("Successful set relay init state for %s", self._mac_in_str) - await self._relay_init_update_state(response.relay.value == 1) + await self._relay_init_update_state(state)Add the missing import at the top of the file as well:
-from ..messages.responses import NodeAckResponseType, NodeInfoResponse, NodeResponseType +from ..messages.responses import ( + NodeAckResponse, + NodeAckResponseType, + NodeInfoResponse, + NodeResponseType, +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise_usb/nodes/circle.py` around lines 1185 - 1192, The code assumes response has both node_ack_type and relay but send() can return CircleRelayInitStateResponse | NodeAckResponse | None; first check for None and then narrow the union with an isinstance/type check: handle CircleRelayInitStateResponse by reading response.relay and calling await self._relay_init_update_state(response.relay.value == 1); handle NodeAckResponse separately (check node_ack_type for DEFAULT_FAIL/DEFAULT_ACK) and on DEFAULT_ACK persist the requested state (from the original CircleRelayInitStateRequest instance or parameter used to send()) by calling await self._relay_init_update_state(requested_state) instead of accessing response.relay; ensure imports include CircleRelayInitStateResponse and NodeAckResponse for the type checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugwise_usb/nodes/circle.py`:
- Around line 1145-1150: The except block around constructing
CircleRelayInitStateRequest is catching MessageError and re-raising NodeError
without chaining; update the handler to raise the NodeError using "from err" so
the original MessageError traceback is preserved (e.g., change the except block
that currently raises NodeError(f"{self._mac_in_str} error: {err}") to raise
NodeError(...) from err); apply the same change for the similar handler at the
CircleRelayInitStateRequest-related code around the 1172–1177 section to satisfy
Ruff B904.
---
Duplicate comments:
In `@plugwise_usb/nodes/circle.py`:
- Around line 1158-1162: The success branch for handling
CircleRelayInitStateResponse currently returns None and so initialize() thinks
the read failed; modify the CircleRelayInitStateResponse branch to return the
parsed boolean state (response.relay.value == 1) after awaiting
_relay_init_update_state so callers get a truthy success value; update the block
that logs "Successful get of relay init state for %s" (using self._mac_in_str)
to await self._relay_init_update_state(response.relay.value == 1) and then
return that boolean instead of falling through to None, ensuring initialize()
recognizes successful reads for RELAY_INIT nodes.
- Around line 1185-1192: The code assumes response has both node_ack_type and
relay but send() can return CircleRelayInitStateResponse | NodeAckResponse |
None; first check for None and then narrow the union with an isinstance/type
check: handle CircleRelayInitStateResponse by reading response.relay and calling
await self._relay_init_update_state(response.relay.value == 1); handle
NodeAckResponse separately (check node_ack_type for DEFAULT_FAIL/DEFAULT_ACK)
and on DEFAULT_ACK persist the requested state (from the original
CircleRelayInitStateRequest instance or parameter used to send()) by calling
await self._relay_init_update_state(requested_state) instead of accessing
response.relay; ensure imports include CircleRelayInitStateResponse and
NodeAckResponse for the type checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da52215c-13be-4785-8493-8c07097b3c2b
📒 Files selected for processing (2)
plugwise_usb/messages/requests.pyplugwise_usb/nodes/circle.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugwise_usb/messages/requests.py`:
- Around line 1523-1524: The docstring references a non-existent response type
name; replace the incorrect CircleInitRelayStateResponse with the actual class
name CircleRelayInitStateResponse in the docstring so it matches the
imported/returned type (search for the docstring block containing "Response
message : NodeAckResponse for set" and update the second line to use
CircleRelayInitStateResponse).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be18fdec-5521-47bc-8013-51055c237429
📒 Files selected for processing (1)
plugwise_usb/messages/requests.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
1145-1150:⚠️ Potential issue | 🟠 MajorChain the wrapped
NodeErrorfrom the caughtMessageError.Line 1150 and Line 1177 still raise without
from err, so Ruff B904 keeps failing and the original traceback is lost.♻️ Proposed fix
except MessageError as err: - raise NodeError(f"{self._mac_in_str} error: {err}") + raise NodeError(f"{self._mac_in_str} error: {err}") from errApply the same change in both handlers.
#!/bin/bash # Verify both handlers still omit exception chaining. sed -n '1145,1150p' plugwise_usb/nodes/circle.py echo '---' sed -n '1172,1177p' plugwise_usb/nodes/circle.py # Expected: each raise statement ends with `from err`.Also applies to: 1172-1177
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise_usb/nodes/circle.py` around lines 1145 - 1150, The except blocks catching MessageError when constructing the CircleRelayInitStateRequest (and the second similar handler later) raise a new NodeError but drop the original traceback; modify both except clauses to re-raise NodeError using exception chaining (raise NodeError(f"{self._mac_in_str} error: {err}") from err) so the original MessageError is preserved in the traceback and Ruff B904 is satisfied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugwise_usb/nodes/circle.py`:
- Around line 1145-1150: The except blocks catching MessageError when
constructing the CircleRelayInitStateRequest (and the second similar handler
later) raise a new NodeError but drop the original traceback; modify both except
clauses to re-raise NodeError using exception chaining (raise
NodeError(f"{self._mac_in_str} error: {err}") from err) so the original
MessageError is preserved in the traceback and Ruff B904 is satisfied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e1fda077-106a-4c40-83a3-b72188db6d18
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py
Comment on lines
+1179
to
+1191
| if (response := await request.send()) is None: | ||
| _LOGGER.warning( | ||
| "No response from %s to configure relay init setting", self._mac_in_str | ||
| ) | ||
| return None | ||
|
|
||
| if response.node_ack_type == NodeAckResponseType.DEFAULT_FAIL: | ||
| _LOGGER.warning("Failed to set relay init state for %s", self._mac_in_str) | ||
| return None | ||
|
|
||
| if response.node_ack_type == NodeAckResponseType.DEFAULT_ACK: | ||
| _LOGGER.debug("Successful set relay init state for %s", self._mac_in_str) | ||
| await self._relay_init_update_state(state) |
There was a problem hiding this comment.
Propagate relay-init write failures instead of returning stale cached state.
On Line 1179 and Line 1185, this helper only logs and returns. If _relay_config.init_state was already populated, set_relay_init() will return that stale value and relay_init_on() / relay_init_off() will appear to succeed even though the write failed. Any non-default ack also falls through silently.
🐛 Proposed fix
if (response := await request.send()) is None:
_LOGGER.warning(
"No response from %s to configure relay init setting", self._mac_in_str
)
- return None
+ raise NodeError(
+ f"No response from {self._mac_in_str} to configure relay init setting"
+ )
if response.node_ack_type == NodeAckResponseType.DEFAULT_FAIL:
_LOGGER.warning("Failed to set relay init state for %s", self._mac_in_str)
- return None
+ raise NodeError(f"Failed to set relay init state for {self._mac_in_str}")
if response.node_ack_type == NodeAckResponseType.DEFAULT_ACK:
_LOGGER.debug("Successful set relay init state for %s", self._mac_in_str)
await self._relay_init_update_state(state)
+ return
+
+ raise NodeError(
+ f"Unexpected ack {response.node_ack_type!r} for relay init set on {self._mac_in_str}"
+ )
There was a problem hiding this comment.
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
1179-1191:⚠️ Potential issue | 🟡 MinorSilent failure may return stale cached state to callers.
When
_relay_init_setfails (no response orDEFAULT_FAIL), it logs a warning and returnsNone. However, the callerset_relay_init()then returnsself._relay_config.init_state, which may contain a stale value from a previous successful write.For example:
- Previous successful write set
init_state=False- User calls
relay_init_on()→_relay_init_set(True)fails silentlyset_relay_init()returnsFalse(stale) without raising an errorIf the current behavior is intentional to avoid excessive error propagation, consider at minimum returning a success/failure indicator or documenting this behavior.
🐛 Possible fix to propagate failures
if (response := await request.send()) is None: _LOGGER.warning( "No response from %s to configure relay init setting", self._mac_in_str ) - return None + raise NodeError( + f"No response from {self._mac_in_str} to configure relay init setting" + ) if response.node_ack_type == NodeAckResponseType.DEFAULT_FAIL: _LOGGER.warning("Failed to set relay init state for %s", self._mac_in_str) - return None + raise NodeError(f"Failed to set relay init state for {self._mac_in_str}") if response.node_ack_type == NodeAckResponseType.DEFAULT_ACK: _LOGGER.debug("Successful set relay init state for %s", self._mac_in_str) await self._relay_init_update_state(state) + return + + raise NodeError( + f"Unexpected ack {response.node_ack_type!r} for relay init set on {self._mac_in_str}" + )Based on learnings: "bouwew prefers not to add excessive validation decorators or protective error handling" - if the current behavior is by design, this can be deferred.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise_usb/nodes/circle.py` around lines 1179 - 1191, The _relay_init_set currently returns None on failure which lets callers like set_relay_init() incorrectly return a stale self._relay_config.init_state; change _relay_init_set (the function handling request.send(), response.node_ack_type checks and calling _relay_init_update_state) to return an explicit boolean: return False when response is None or response.node_ack_type == NodeAckResponseType.DEFAULT_FAIL, and return True after a successful _relay_init_update_state call (i.e., when NodeAckResponseType.DEFAULT_ACK); also update set_relay_init() to propagate/return that boolean result so callers can detect failure instead of reading stale _relay_config.init_state.
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
45-45: ImportCircleRelayInitStateResponsefrom its source module.
CircleRelayInitStateResponseis defined in..messages.responses, not..messages.requests. While this works becauserequests.pyimports it, importing from the actual source module is clearer and more maintainable.♻️ Suggested change
from ..messages.requests import ( CircleClockGetRequest, CircleClockSetRequest, CircleEnergyLogsRequest, CircleMeasureIntervalRequest, CirclePowerUsageRequest, CircleRelayInitStateRequest, - CircleRelayInitStateResponse, CircleRelaySwitchRequest, EnergyCalibrationRequest, NodeInfoRequest, ) -from ..messages.responses import NodeAckResponseType, NodeInfoResponse, NodeResponseType +from ..messages.responses import ( + CircleRelayInitStateResponse, + NodeAckResponseType, + NodeInfoResponse, + NodeResponseType, +)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugwise_usb/nodes/circle.py` at line 45, The import in plugwise_usb/nodes/circle.py currently pulls CircleRelayInitStateResponse from ..messages.requests but its canonical definition lives in ..messages.responses; change the import to import CircleRelayInitStateResponse directly from ..messages.responses (update the import list that includes CircleRelayInitStateResponse) so the module references the source of truth and avoids relying on transitive imports from requests.py.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@plugwise_usb/nodes/circle.py`:
- Around line 1179-1191: The _relay_init_set currently returns None on failure
which lets callers like set_relay_init() incorrectly return a stale
self._relay_config.init_state; change _relay_init_set (the function handling
request.send(), response.node_ack_type checks and calling
_relay_init_update_state) to return an explicit boolean: return False when
response is None or response.node_ack_type == NodeAckResponseType.DEFAULT_FAIL,
and return True after a successful _relay_init_update_state call (i.e., when
NodeAckResponseType.DEFAULT_ACK); also update set_relay_init() to
propagate/return that boolean result so callers can detect failure instead of
reading stale _relay_config.init_state.
---
Nitpick comments:
In `@plugwise_usb/nodes/circle.py`:
- Line 45: The import in plugwise_usb/nodes/circle.py currently pulls
CircleRelayInitStateResponse from ..messages.requests but its canonical
definition lives in ..messages.responses; change the import to import
CircleRelayInitStateResponse directly from ..messages.responses (update the
import list that includes CircleRelayInitStateResponse) so the module references
the source of truth and avoids relying on transitive imports from requests.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5454df7e-2d80-4ce8-9125-11eea697f382
📒 Files selected for processing (4)
CHANGELOG.mdplugwise_usb/messages/requests.pyplugwise_usb/nodes/circle.pypyproject.toml
✅ Files skipped from review due to trivial changes (2)
- CHANGELOG.md
- pyproject.toml
|
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 by CodeRabbit
Bug Fixes
Chores