Improve node_info_update and update_node_details logic#333
Conversation
WalkthroughRefactors PlugwiseCircle control flow: Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant Circle as PlugwiseCircle
participant Super as BaseNode
participant Relay as _relay_update_state
participant Log as Logger
rect rgba(230,245,255,0.5)
Note over Caller,Circle: update_node_details new flow
Caller->>Circle: update_node_details(node_info)
alt node_info is None
Circle-->>Caller: False
else node_info present
Circle->>Relay: _relay_update_state(node_info.relay_state)
Relay-->>Circle: relay state updated
opt pointer-based rollover check
Circle->>Log: "Rollover log address from %s to %s for node %s"
Note right of Circle: update _current_log_address if pointer differs
end
Circle->>Super: update_node_details(node_info)
Super-->>Circle: bool
Circle-->>Caller: bool
end
end
sequenceDiagram
autonumber
actor Caller
participant Circle as PlugwiseCircle
participant Super as BaseNode
rect rgba(240,240,240,0.6)
Note over Caller,Circle: node_info_update is now pass-through
Caller->>Circle: node_info_update(node_info)
Circle->>Super: node_info_update(node_info)
Super-->>Circle: NodeInfo | None
Circle-->>Caller: self._node_info
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
✨ Finishing Touches🧪 Generate unit tests
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 #333 +/- ##
==========================================
- Coverage 81.30% 81.30% -0.01%
==========================================
Files 36 36
Lines 8063 8062 -1
==========================================
- Hits 6556 6555 -1
Misses 1507 1507 ☔ 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: 0
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
1049-1056: Rollover detection condition likely incorrect (spurious logs when current_log_address == 1).A rollover should be logged when the new pointer is lower than the previous one. The extra
or self._current_log_address == 1will falsely log rollover on a normal increment from 1→2. Drop that clause.- and ( - self._current_log_address > node_info.current_logaddress_pointer - or self._current_log_address == 1 - ) + and ( + self._current_log_address > node_info.current_logaddress_pointer + )
1058-1063: Nit: logging wording.Change “into” → “to” for readability.
- "Rollover log address from %s into %s for node %s", + "Rollover log address from %s to %s for node %s",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (4)
plugwise_usb/api.py (2)
node_info(330-331)relay_state(464-468)plugwise_usb/nodes/node.py (2)
node_info(225-227)relay_state(298-302)plugwise_usb/messages/responses.py (2)
relay_state(643-645)current_logaddress_pointer(638-640)plugwise_usb/nodes/helpers/counter.py (1)
timestamp(179-187)
🔇 Additional comments (1)
plugwise_usb/nodes/circle.py (1)
1045-1047: Good move: delegate relay state updates to the helper.This centralizes state changes, preserves timestamps, and ensures subscriber notifications/cache writes happen via one code path.
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)
plugwise_usb/nodes/circle.py (1)
1041-1047: Require non-optionalnode_infoparameter: remove the default= Nonefrom thenode_infoargument inupdate_node_details(plugwise_usb/nodes/circle.py, line 1041) since every call site passes a value—this prevents accidentalNonedereferences.
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
1065-1066: Nit: avoid redundant writes.Skip assignment when the pointer didn’t change to reduce churn/log noise.
- if node_info.current_logaddress_pointer is not None: - self._current_log_address = node_info.current_logaddress_pointer + if ( + node_info.current_logaddress_pointer is not None + and node_info.current_logaddress_pointer != self._current_log_address + ): + self._current_log_address = node_info.current_logaddress_pointer
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (3)
plugwise_usb/api.py (2)
node_info(330-331)relay_state(464-468)plugwise_usb/nodes/node.py (2)
node_info(225-227)relay_state(298-302)plugwise_usb/messages/responses.py (2)
relay_state(643-645)current_logaddress_pointer(638-640)
⏰ 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/nodes/circle.py (1)
1045-1047: Good move: centralize relay state updates via the helper.Using
_relay_update_state(...)here ensures consistent cache + publish behavior and avoids double-setting.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
1049-1061: Rollover detection fix matches prior feedback — no more false positives on 1→2.Condition now only flags rollover when the pointer decreases. This resolves the previous “== 1” false positive path.
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
1045-1047: Good move: delegate relay updates to the single source of truth.Routing NodeInfo relay changes through
_relay_update_state(...)centralizes cache + publish behavior. Consider guarding against stale NodeInfo timestamps to avoid overwriting a newer state.- if node_info.relay_state is not None: - await self._relay_update_state( - node_info.relay_state, timestamp=node_info.timestamp - ) + if node_info.relay_state is not None: + # Ignore stale updates if we already have a newer relay timestamp + if ( + self._relay_state.timestamp is None + or node_info.timestamp is None + or node_info.timestamp > self._relay_state.timestamp + ): + await self._relay_update_state( + node_info.relay_state, timestamp=node_info.timestamp + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (3)
plugwise_usb/api.py (2)
node_info(330-331)relay_state(464-468)plugwise_usb/nodes/node.py (2)
node_info(225-227)relay_state(298-302)plugwise_usb/messages/responses.py (2)
relay_state(643-645)current_logaddress_pointer(638-640)
🔇 Additional comments (1)
plugwise_usb/nodes/circle.py (1)
1062-1066: Drop unnecessary check:PlugwiseBaseNode.update_node_detailsnever mutates_current_log_address. The base implementation only initializes the attribute and contains no assignments to it, so there’s no risk of double‐writes or ordering conflicts.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
1052-1064: Rollover detection condition fixed as requested previously.Now flags rollover only when the new pointer is lower than the previous address; removes the false-positive branch.
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
1052-1069: Minor: avoid repeated attribute access and duplicate None-check.Store the pointer once and reuse; keeps the logic identical and a bit clearer.
Apply:
- if ( - node_info.current_logaddress_pointer is not None - and self._current_log_address is not None - and self._current_log_address > node_info.current_logaddress_pointer - ): + ptr = node_info.current_logaddress_pointer + if ( + ptr is not None + and self._current_log_address is not None + and self._current_log_address > ptr + ): # Rollover of log address _LOGGER.debug( - "Rollover log address from %s to %s for node %s", - self._current_log_address, - node_info.current_logaddress_pointer, - self._mac_in_str, + "Rollover log address from %s to %s for node %s", + self._current_log_address, + ptr, + self._mac_in_str, ) - if ( - node_info.current_logaddress_pointer is not None - and node_info.current_logaddress_pointer != self._current_log_address - ): - self._current_log_address = node_info.current_logaddress_pointer + if ptr is not None and ptr != self._current_log_address: + self._current_log_address = ptr
1041-1046: Broadenupdate_node_detailssignature in plugwise_usb/nodes/circle.py to acceptNodeInfoMessage:--- a/plugwise_usb/nodes/circle.py @@ -1040,7 +1040,7 @@ async def update_node_details( - self, node_info: NodeInfoResponse | None = None + self, node_info: NodeInfoResponse | NodeInfoMessage | None = None ) -> bool:Aligns the override with the base class and the preceding
node_info_update, ensuring forwardedNodeInfoMessageinstances are correctly handled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (4)
plugwise_usb/api.py (2)
node_info(330-331)relay_state(464-468)plugwise_usb/nodes/node.py (2)
node_info(225-227)relay_state(298-302)plugwise_usb/messages/responses.py (2)
relay_state(643-645)current_logaddress_pointer(638-640)plugwise_usb/nodes/helpers/counter.py (1)
timestamp(179-187)
🔇 Additional comments (2)
plugwise_usb/nodes/circle.py (2)
1044-1046: Early None-guard in update_node_details is fine.Prevents accidental partial updates when no payload is provided.
1047-1051: Good move: use _relay_update_state (with timestamp) instead of mutating state.Ensures cache persistence and subscriber notifications without duplication.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
1052-1064: Rollover detection fix looks correct.Condition now triggers only when the new pointer is lower than the previous one, resolving the prior false positive on 1→2.
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
1044-1046: Log on early-return to aid traceability.Returning False silently when node_info is None makes troubleshooting harder. Add a debug line.
- if node_info is None: - return False + if node_info is None: + _LOGGER.debug("update_node_details called with None for %s", self._mac_in_str) + return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (3)
plugwise_usb/api.py (3)
node_info(330-331)NodeInfoMessage(136-144)relay_state(464-468)plugwise_usb/nodes/node.py (2)
node_info(225-227)relay_state(298-302)plugwise_usb/messages/responses.py (3)
NodeInfoResponse(582-654)relay_state(643-645)current_logaddress_pointer(638-640)
⏰ 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 (3)
plugwise_usb/nodes/circle.py (3)
1041-1042: Signature widened to accept NodeInfoMessage — good.Keeps type symmetry with node_info_update and aligns with the NodeInfoMessage schema.
1047-1051: No change needed:.timestampis inherited fromPlugwiseMessage
PlugwiseMessage.__init__setsself.timestamp, andNodeInfoResponseinherits viaPlugwiseResponse, sonode_info.timestampis always available.
1065-1071: No changes needed toupdate_node_detailsreturn value
All call sites ignore its Boolean result—calls innode_info_update,_node_info_load_from_cache, and tests do not branch on it—so OR-ing local-change flags would have no effect.
|
2 participants
Push updating of relay and _current_logaddress_pointer into update_node_details
remove double setting of values.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores