Small changes to Circle+ load function to align to Circle load function #322
Conversation
WalkthroughOn Circle+ node load, cache-hit path now sets _loaded and skips prior post-cache initialization and loaded-event emission; logs standardized to use internal Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CirclePlus as CirclePlus.load()
participant Cache
participant Device
Caller->>CirclePlus: load()
CirclePlus->>Cache: try load from cache
alt Cache hit
Note right of CirclePlus#e6f5ff: set _loaded, skip post-cache init\n(do not emit loaded event)
else Cache miss
CirclePlus->>Device: retrieve node info / node_info_update()
end
CirclePlus->>Device: check online AND node_info_update()
alt Not online or no response
Note right of CirclePlus#ffe6e6: log single warning (not online/not responding)
CirclePlus-->>Caller: return False
else OK
CirclePlus->>CirclePlus: set _loaded = True
CirclePlus->>Device: initialize()
alt initialize fails
CirclePlus-->>Caller: return False
else initialize succeeds
CirclePlus-->>Caller: return True
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle_plus.py (1)
59-66: Don’t set_loaded = Truebefore initialization; also roll back on init failure.Currently
_loadedis set to True before_setup_protocol+initialize(). Ifinitialize()fails, the method returns False but_loadedremains True, leaving the object in an inconsistent “loaded” state and potentially bypassing@raise_not_loadedguards elsewhere. Also, setting_loadedearly may allow concurrent callers to use the node before it’s ready.Apply this diff to defer and guard the loaded flag:
- self._loaded = True - self._setup_protocol(CIRCLE_PLUS_FIRMWARE_SUPPORT, FEATURES_CIRCLE_PLUS) - if not await self.initialize(): - return False + if not await self.initialize(): + # Roll back a previously set _loaded (e.g., by cache) to avoid inconsistent state + self._loaded = False + return False + + # Mark as loaded only after successful init (noop if already set by cache) + self._loaded = True
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle_plus.py (2)
44-46: Nit: Logging style and identifier — use sentence case andself.mac.Minor consistency/readability tweak: align message casing with the rest of the codebase and use the
macproperty instead of_mac_in_str.- if not self._loaded: - _LOGGER.debug("Retrieving Info For Circle+ node %s", self._mac_in_str) + if not self._loaded: + _LOGGER.debug("Retrieving info for Circle+ node %s", self.mac)
48-56: Clarify boolean precedence and avoid implicit double-initialize on cache hits.
- Readability: The combined
and/orwith awaits relies on Python precedence; it’s easy to misread.- Behavior: When
_loadedis True due to a cache hit and_availableis False,is_online()will triggerinitialize()internally (see node.py:576-584), and theninitialize()is called again below — potentially performing a second init.Two options:
Option A — keep logic, but make precedence explicit and use
self.macin logs. Low risk, improves readability:- # Check if node is online - if ( - not self._available - and not await self.is_online() - or await self.node_info_update() is None - ): - _LOGGER.warning( - "Failed to load Circle+ node %s because it is not online or not responding", - self._mac_in_str, - ) + # Check if node is online + if ((not self._available and not await self.is_online()) + or (await self.node_info_update() is None)): + _LOGGER.warning( + "Failed to load Circle+ node %s because it is not online or not responding", + self.mac, + ) return FalseOption B — avoid double init by skipping
is_online()when_loadedis already True (cache hit). This keeps the current semantics while preventinginitialize()being invoked twice:- # Check if node is online - if ( - not self._available - and not await self.is_online() - or await self.node_info_update() is None - ): - _LOGGER.warning( - "Failed to load Circle+ node %s because it is not online or not responding", - self._mac_in_str, - ) - return False + # Ensure we can talk to the node and we have its info + if self._loaded: + # Avoid is_online() here to prevent a double initialize when loaded-from-cache + if await self.node_info_update() is None: + _LOGGER.warning( + "Failed to load Circle+ node %s because it is not responding", + self.mac, + ) + return False + else: + if (not await self.is_online()) or (await self.node_info_update() is None): + _LOGGER.warning( + "Failed to load Circle+ node %s because it is not online or not responding", + self.mac, + ) + return FalseNote: Node.is_online at plugwise_usb/nodes/node.py:576-584 calls initialize() when
_loadedis True and_initializedis False. If you prefer Option A, please confirm initialize() is idempotent.
📜 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_plus.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/circle_plus.py (2)
plugwise_usb/nodes/node.py (4)
mac(235-237)_load_from_cache(396-410)is_online(577-585)node_info_update(452-465)plugwise_usb/nodes/circle.py (2)
_load_from_cache(959-1002)node_info_update(1052-1085)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #322 +/- ##
=======================================
Coverage ? 80.63%
=======================================
Files ? 36
Lines ? 7559
Branches ? 0
=======================================
Hits ? 6095
Misses ? 1464
Partials ? 0 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle_plus.py (1)
41-41: Prefer using the publicmacproperty in logs for consistency and safety.While
_mac_in_stris safe here, the codebase predominantly logsself.mac. Using the property also preserves encapsulation.- _LOGGER.debug("Loading Circle+ node %s from cache", self._mac_in_str) + _LOGGER.debug("Loading Circle+ node %s from cache", self.mac)
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle_plus.py (3)
44-46: Nit: Unify log style and useself.mac.Lowercase the message for consistency and use the public
macproperty.- _LOGGER.debug("Retrieving Info For Circle+ node %s", self._mac_in_str) + _LOGGER.debug("Retrieving info for Circle+ node %s", self.mac)
48-57: Avoid ambiguous boolean precedence and unnecessary network calls on cache-hit.The mixed
and/orcondition is easy to misread and currently forcesnode_info_update()even when_node_infois already loaded from cache (left side short-circuits to False if available). Refactor to compute the two checks explicitly and only callnode_info_update()if_node_infois missing.- if ( - not self._available - and not await self.is_online() - or await self.node_info_update() is None - ): - _LOGGER.warning( - "Failed to load Circle+ node %s because it is not online or not responding", - self._mac_in_str, - ) - return False + # Check if node is online and that we have (or can retrieve) node info + online_ok = self._available or await self.is_online() + node_info_ok = self._node_info is not None or (await self.node_info_update()) is not None + if not (online_ok and node_info_ok): + _LOGGER.warning( + "Failed to load Circle+ node %s because it is not online or not responding", + self.mac, + ) + return False
59-66: Prevent double-initialization whenis_online()already initialized the node.
is_online()may callinitialize()when_loadedis True and_initializedis False. The subsequent unconditional initialize here can run twice. Guard it on_initialized.- self._setup_protocol(CIRCLE_PLUS_FIRMWARE_SUPPORT, FEATURES_CIRCLE_PLUS) - if not await self.initialize(): - return False + if not self._initialized: + self._setup_protocol(CIRCLE_PLUS_FIRMWARE_SUPPORT, FEATURES_CIRCLE_PLUS) + if not await self.initialize(): + return False
📜 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_plus.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-20T07:58:41.534Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.
Applied to files:
plugwise_usb/nodes/circle_plus.py
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/circle_plus.py (2)
plugwise_usb/nodes/circle.py (2)
_load_from_cache(959-1002)node_info_update(1052-1085)plugwise_usb/nodes/node.py (3)
_load_from_cache(396-410)is_online(577-585)node_info_update(452-465)
⏰ 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_plus.py (1)
65-66: LOADED event is still emitted (contrary to the summary).
_loaded_callback(NodeEvent.LOADED, ...)remains in place; if the intention was to remove it on cache-hit, confirm desired behavior.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle_plus.py (1)
41-43: LGTM: cache-load log now uses a safe identifier and matches Circle's styleUsing
_mac_in_stravoids touching_node_infobefore it exists and aligns with logging incircle.py.
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle_plus.py (1)
48-52: Clarify operator precedence in the combined online/info checkThe current condition is correct, but
andvsorprecedence is non-trivial at a glance. Add parentheses to make the intent explicit and future-proof:- if ( - not self._available - and not await self.is_online() - or await self.node_info_update() is None - ): + if ( + (not self._available and not await self.is_online()) + or (await self.node_info_update() is None) + ):
📜 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_plus.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-20T07:58:41.534Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.
Applied to files:
plugwise_usb/nodes/circle_plus.py
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/circle_plus.py (5)
plugwise_usb/nodes/scan.py (1)
_load_from_cache(144-173)plugwise_usb/nodes/sed.py (1)
_load_from_cache(146-180)plugwise_usb/nodes/circle.py (2)
_load_from_cache(959-1002)node_info_update(1052-1085)plugwise_usb/nodes/node.py (3)
_load_from_cache(396-410)is_online(577-585)node_info_update(452-465)plugwise_usb/api.py (1)
is_online(329-330)
Summary by CodeRabbit
Refactor
Chores