Improve registry discovery and SED/SCAN configuration management #302
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #302 +/- ##
==========================================
- Coverage 80.92% 80.63% -0.30%
==========================================
Files 36 36
Lines 7593 7540 -53
==========================================
- Hits 6145 6080 -65
- Misses 1448 1460 +12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Caution Review failedThe pull request is closed. WalkthroughRegistry and discovery were refactored to use MAC-only registrations; node constructors and network/address APIs had address parameters removed; SED/SCAN configs gained Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant Stick as StickNetwork
participant Reg as StickNetworkRegister
participant Cache as NetworkRegistrationCache
participant Node as PlugwiseNode
Test->>Stick: start(load=True)
Stick->>Reg: register start-node-discover callback
Stick->>Reg: register scan-completed callback
Stick->>Cache: load_registrations_from_cache()
Cache-->>Reg: return cached MAC list
Reg->>Stick: _exec_node_discover_callback(mac, node_type, ping_first)
Stick->>Node: _create_node_object(mac, node_type)
Reg->>Reg: update_missing_registrations_circleplus() (scan addresses)
Reg->>Reg: update_network_registration(mac) -> added?
Reg->>Cache: prune_cache(registry_list)
Reg->>Stick: invoke scan-completed callback
Stick-->>Test: scan completed signal
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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. 🪧 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 comments)
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: 10
🔭 Outside diff range comments (2)
plugwise_usb/network/registry.py (2)
93-95: Fix type mismatch in registry propertyThe property signature returns a dictionary but
_registryis now a list. This will cause a runtime error.Either update the return type or modify the implementation:
@property - def registry(self) -> dict[int, tuple[str, NodeType | None]]: + def registry(self) -> list[str]: """Return dictionary with all joined nodes.""" return deepcopy(self._registry)
258-261: Fix incorrect registry iterationThe code tries to call
values()on_registrywhich is now a list, not a dictionary.mac_registered = False - for registration in self._registry.values(): - if mac == registration[0]: - mac_registered = True - break + mac_registered = mac in self._registry
♻️ Duplicate comments (6)
tests/test_usb.py (5)
695-696: Duplicate synchronization patternSame synchronization approach as mentioned earlier. Consider refactoring both occurrences.
771-772: Another instance of synchronization patternThird occurrence of the same synchronization approach.
910-911: Fourth instance of synchronization patternSame synchronization approach repeated.
2153-2153: Consistent boolean cache supportSame monkeypatch pattern as used earlier.
2442-2443: Synchronization pattern repeatedSame synchronization approach as noted earlier.
plugwise_usb/network/registry.py (1)
223-223: Same callback signature mismatchThis has the same issue as line 202 - passing 3 arguments to what should be a single-argument callback.
🧹 Nitpick comments (6)
plugwise_usb/nodes/circle.py (1)
913-916: Clarify complex conditional logicThe conditional logic could be misinterpreted due to operator precedence. Consider adding parentheses or restructuring for clarity.
- 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):Alternatively, for better readability:
- if ( - not self._available - and not await self.is_online() - or await self.node_info_update() is None - ): + node_info = await self.node_info_update() + if (not self._available and not await self.is_online()) or node_info is None:plugwise_usb/network/__init__.py (2)
281-283: Simplify complex conditional logicThe nested conditions make the code harder to read and maintain.
- if ( - self._register.update_node_registration(mac) - and self._nodes.get(mac) is None - ): + registration_updated = self._register.update_node_registration(mac) + node_not_exists = self._nodes.get(mac) is None + + if registration_updated and node_not_exists:
507-511: Simplify task creation conditionThe condition for creating the discover stragglers task is duplicated and could be extracted.
- if len(self._registry_stragglers) > 0 and ( - self._discover_stragglers_task is None - or self._discover_stragglers_task.done() - ): - self._discover_stragglers_task = create_task(self._discover_stragglers()) + if len(self._registry_stragglers) > 0: + if self._discover_stragglers_task is None or self._discover_stragglers_task.done(): + self._discover_stragglers_task = create_task(self._discover_stragglers())Consider extracting this pattern into a helper method since it's also used at line 485-488 for
_load_stragglers_task.tests/test_usb.py (2)
504-504: Fix typo in debug log message- _LOGGER.debug("Node %s has even %s", mac, str(pw_api.NodeEvent)) + _LOGGER.debug("Node %s has event %s", mac, str(pw_api.NodeEvent))
567-568: Consider improving synchronization approachThe current approach accesses private attributes and uses a polling loop. Consider:
- Adding a public method or property to check scan completion status
- Using an event or future for more efficient synchronization
- while not stick._network._register.scan_completed: - await asyncio.sleep(0.1) + # Consider adding a public method like: + # await stick.wait_for_discovery_completion()plugwise_usb/nodes/sed.py (1)
193-196: Remove debug loggingThe debug log appears to be leftover development code and should be removed.
def _clock_sync_from_cache(self) -> bool | None: """Load clock sync state from cache.""" - _LOGGER.debug( - "MDI: Bool: %s", str(self._get_cache_as_bool(CACHE_SED_CLOCK_SYNC)) - ) return self._get_cache_as_bool(CACHE_SED_CLOCK_SYNC)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
CHANGELOG.md(1 hunks)plugwise_usb/api.py(3 hunks)plugwise_usb/network/__init__.py(11 hunks)plugwise_usb/network/cache.py(1 hunks)plugwise_usb/network/registry.py(8 hunks)plugwise_usb/nodes/__init__.py(0 hunks)plugwise_usb/nodes/circle.py(2 hunks)plugwise_usb/nodes/node.py(1 hunks)plugwise_usb/nodes/scan.py(11 hunks)plugwise_usb/nodes/sed.py(16 hunks)plugwise_usb/nodes/sense.py(2 hunks)plugwise_usb/nodes/switch.py(2 hunks)pyproject.toml(1 hunks)tests/test_usb.py(28 hunks)
💤 Files with no reviewable changes (1)
- plugwise_usb/nodes/init.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: in the plugwise_usb codebase, bouwew prefers not to add excessive validation decorators or protectiv...
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#252
File: plugwise_usb/__init__.py:0-0
Timestamp: 2025-06-03T09:58:44.611Z
Learning: In the plugwise_usb codebase, bouwew prefers not to add excessive validation decorators or protective error handling when they would amount to "too much protection". The existing validation level is considered sufficient.
Applied to files:
pyproject.toml
📚 Learning: in plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_colle...
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.pyplugwise_usb/nodes/sense.pyplugwise_usb/nodes/node.pyplugwise_usb/network/__init__.pyplugwise_usb/nodes/scan.pytests/test_usb.py
📚 Learning: in the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `none` means th...
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.pyplugwise_usb/nodes/switch.pyplugwise_usb/nodes/sense.pyplugwise_usb/nodes/node.pyplugwise_usb/nodes/scan.pytests/test_usb.py
⏰ 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 (39)
pyproject.toml (1)
7-7: LGTM! Appropriate alpha version bump.The version increment to "0.44.10a0" correctly indicates this is an alpha release with significant changes, aligning with the extensive refactoring described in the PR objectives.
CHANGELOG.md (1)
3-7: LGTM! Clear documentation of ongoing changes.The "Ongoing" section effectively documents the current improvements, with entries that accurately reflect the PR objectives for registry collection, node discovery, and configuration handling enhancements.
plugwise_usb/network/cache.py (1)
82-90: LGTM! Well-implemented cache pruning logic.The method correctly filters the cache to maintain only entries for MAC addresses present in the registry, properly handles empty MAC addresses, and persists the changes. This aligns well with the unified registry management approach.
plugwise_usb/api.py (2)
121-122: LGTM! Improved configuration state tracking.The addition of the
dirtyflag toBatteryConfigprovides a clean way to track when configuration changes are pending device synchronization, replacing the previous_new_variable approach mentioned in the PR objectives.Also applies to: 130-130
237-238: LGTM! Consistent configuration management.The
dirtyflag inMotionConfigfollows the same improved pattern asBatteryConfig, providing consistent configuration state tracking across different node types.Also applies to: 244-244
plugwise_usb/nodes/sense.py (2)
49-49: LGTM! Consistent with MAC-only identification.The removal of the
addressparameter aligns with the broader refactoring to use MAC addresses exclusively for node identification, simplifying the constructor interface.
97-97: LGTM! Improved node info update scheduling.Setting
_sed_node_info_update_task_scheduled = Trueafter assigning default values ensures that these changes are properly synchronized with the device, improving the reliability of the node initialization process.Also applies to: 100-100, 103-103
plugwise_usb/nodes/switch.py (1)
85-91: _sed_node_info_update_task_scheduled Already Defined in Parent ClassThe
_sed_node_info_update_task_scheduledflag is initialized inNodeSED.__init__(plugwise_usb/nodes/sed.py:93), so its usage inplugwise_usb/nodes/switch.pyis valid. No changes are required.tests/test_usb.py (11)
699-699: Test expectation updated correctlyThe discovered nodes count of 7 aligns with the refactored discovery logic.
1922-1934: Cache key updates align with refactoringThe new cache key naming with
CACHE_SED_prefix and boolean return values improve type safety and organization.
1937-1937: Correct monkeypatch for boolean cache valuesAdding
_get_cache_as_boolmonkeypatch is necessary for the new boolean cache values.
1979-2003: Well-implemented dirty flag mechanismThe battery configuration dirty flag correctly tracks pending changes and is cleared after successful application.
2028-2033: Validation range and dirty flag correctly implementedThe maintenance interval validation and dirty flag management are properly implemented.
2046-2068: Clock interval implementation consistent with refactoringThe clock interval cache keys and dirty flag management follow the established pattern.
2116-2150: Scan node cache implementation follows best practicesThe
CACHE_SCAN_prefixed keys and motion config dirty flag are consistently implemented.
2185-2223: Motion config dirty flag properly implementedThe dirty flag mechanism for motion configuration is consistent with the battery configuration pattern.
2343-2343: Boolean cache support
2545-2545: Node loading assertion is appropriateThe assertion correctly verifies the node is loaded after discovery.
2328-2340: Switch node correctly uses CACHE_SED_ keys
ThePlugwiseSwitchclass inherits fromNodeSED(plugwise_usb/nodes/switch.py:33), and there are noCACHE_SWITCH_constants defined in the codebase. Therefore, using theCACHE_SED_–prefixed keys in tests is intentional and correct.plugwise_usb/nodes/scan.py (9)
35-42: Cache key naming convention is consistentThe renaming of cache keys with the
CACHE_SCAN_CONFIG_prefix improves code organization and aligns with the similar naming pattern used insed.py.
46-56: Good use of explicit typing for constantsThe addition of
Finaltype hints to the default constants improves type safety and makes the code more maintainable.
86-86: Constructor correctly updated to remove address parameterThe removal of the
addressparameter from the parent class initialization is consistent with the broader refactoring to use MAC addresses exclusively.
136-146: Clear tracking of node info updates neededThe use of
_sed_node_info_update_task_scheduledflag and debug logging for cache misses improves observability and maintenance tracking.
179-218: Cache getter methods correctly updatedThe cache getter methods properly use the new cache key names and handle missing values appropriately. The default
Truefor the dirty flag ensures configuration updates when cache state is unknown.
301-347: Configuration setters correctly implement dirty flag patternThe use of
dataclasses.replace()withdirty=Trueand immediate cache updates provides a clean and consistent configuration management pattern.
426-431: Well-structured awake task executionThe method properly extends the parent's awake task execution and cleanly integrates configuration updates based on the dirty flag.
441-478: Configuration method correctly manages dirty flag lifecycleThe
scan_configuremethod properly uses the stored configuration, handles responses appropriately, and clears the dirty flag only on successful configuration.
480-498: Cache update method provides comprehensive state persistenceThe method correctly persists all configuration values including the dirty flag, ensuring consistent state across restarts.
plugwise_usb/network/registry.py (5)
26-28: Good use of constants for scan timingThe explicit constants for scan timing improve code readability and make it easier to tune the scanning behavior.
46-54: Registry data structure simplified appropriatelyThe change from dictionary to list simplifies the registry since addresses are no longer needed. The new callback names are more descriptive.
102-108: Clean callback registration interfaceThe simplified callback methods with clear single purposes improve the API design.
150-181: Registration methods correctly implement MAC-based trackingThe simplified registration methods properly manage the MAC list and include appropriate cache pruning on removal.
216-220: Smart optimization of scan timing based on cache sizeAdjusting the scan delay when the cache has fewer than 4 nodes helps with faster initial discovery while being conservative with larger networks.
plugwise_usb/nodes/sed.py (6)
26-33: Consistent cache key naming with CACHE_SED_ prefixThe cache keys follow a clear naming convention that aligns with the pattern used in scan.py.
87-93: Constructor correctly updated for MAC-only identificationThe removal of the address parameter and addition of the update task flag aligns with the overall refactoring pattern.
241-327: Configuration setters correctly implement dirty flag patternAll setters consistently use
dataclasses.replace()to update configuration with the dirty flag. Unlike scan.py, these don't trigger immediate cache updates, which is appropriate for battery-powered devices that update during awake periods.
547-557: Awake tasks properly handle both update typesThe method correctly checks for both node info updates and configuration updates, executing them in the appropriate order.
558-595: Configuration method properly manages battery settingsThe method correctly uses the battery configuration dataclass and appropriately clears the dirty flag only on successful configuration.
597-622: Comprehensive cache persistence for battery configurationThe method correctly saves all configuration values including the dirty flag, ensuring complete state recovery after restarts.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/test_usb.py (2)
577-579: Remove redundant MAC address assignment.The hardcoded assignment on line 578 overwrites the value obtained from the future, making the test less meaningful. This issue was previously identified.
mac_awake_node = await self.test_node_awake - mac_awake_node = "5555555555555555" assert mac_awake_node == "5555555555555555"
1963-1967: Address commented test code.This commented-out NodeInfo test code was previously identified and should either be implemented or removed to keep the codebase clean.
🧹 Nitpick comments (3)
tests/test_usb.py (1)
504-504: Fix typo and improve logging format.There's a typo in the debug message: "has even" should be "has event". Also, the logging format could be improved to show the actual event value rather than the enum class.
- _LOGGER.debug("Node %s has even %s", mac, str(pw_api.NodeEvent)) + _LOGGER.debug("Node %s has event %s", mac, event)plugwise_usb/nodes/sed.py (2)
145-147: Fix variable naming convention.Variable
LoadSuccessshould follow Python naming conventions.- LoadSuccess = True + load_success = True if not await super()._load_from_cache(): - LoadSuccess = False + load_success = FalseAnd update the return statement:
- return LoadSuccess + return load_success
330-332: Fix type annotation for dirty property.The type annotation indicates
intbut the property returns a boolean value fromself._battery_config.dirty.@property - def dirty(self) -> int: + def dirty(self) -> bool: """Battery configuration dirty flag."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/nodes/sed.py(16 hunks)tests/test_usb.py(29 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_colle...
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:
tests/test_usb.py
📚 Learning: in the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `none` means th...
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:
tests/test_usb.py
🧬 Code Graph Analysis (1)
tests/test_usb.py (6)
plugwise_usb/nodes/node.py (13)
mac(235-237)PlugwiseBaseNode(56-843)battery_config(121-127)set_awake_duration(736-742)set_maintenance_interval(763-769)set_clock_interval(745-751)clock_sync(131-137)set_clock_sync(754-760)motion_config(249-255)set_motion_reset_timer(781-787)set_motion_daylight_mode(772-778)set_motion_sensitivity_level(790-796)is_loaded(208-210)plugwise_usb/api.py (13)
mac(316-317)NodeEvent(30-36)battery_config(484-495)set_awake_duration(526-549)set_maintenance_interval(594-617)set_clock_interval(551-572)set_clock_sync(574-592)motion_config(498-508)set_motion_reset_timer(637-660)set_motion_daylight_mode(619-635)set_motion_sensitivity_level(662-679)MotionSensitivity(22-27)is_loaded(284-285)plugwise_usb/network/registry.py (1)
scan_completed(98-100)plugwise_usb/__init__.py (2)
joined_nodes(112-120)nodes(95-99)plugwise_usb/nodes/sed.py (11)
battery_config(342-351)dirty(330-332)awake_duration(335-339)set_awake_duration(225-243)_awake_response(402-458)set_maintenance_interval(282-301)maintenance_interval(368-375)clock_interval(354-358)set_clock_interval(245-264)clock_sync(361-365)set_clock_sync(266-280)plugwise_usb/nodes/scan.py (7)
dirty(231-233)reset_timer(272-276)motion_config(262-269)set_motion_reset_timer(309-329)daylight_mode(236-240)set_motion_daylight_mode(288-307)set_motion_sensitivity_level(331-347)
⏰ 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 (17)
tests/test_usb.py (9)
567-568: LGTM! Proper synchronization with async discovery.The addition of scan completion waiting is necessary to ensure tests properly synchronize with the new asynchronous node discovery flow. This prevents race conditions in test assertions.
695-699: LGTM! Proper scan synchronization and node count validation.The scan completion wait ensures proper synchronization, and the node count assertions validate the expected discovery results. The differentiation between joined nodes (9) and discovered nodes (7) correctly reflects the network topology.
771-772: LGTM! Consistent scan synchronization.Proper application of the scan completion wait pattern, ensuring consistency across test methods.
910-912: LGTM! Consistent scan synchronization in energy tests.Proper synchronization pattern maintained across all test methods.
922-937: LGTM! Cache key updates align with architectural changes.The cache key renaming to use
CACHE_SED_prefixes and the addition of_get_cache_as_boolpatching properly reflect the configuration management improvements and cache handling refactoring mentioned in the PR objectives.
1979-2110: LGTM! Proper testing of dirty flag configuration system.The battery configuration tests correctly verify the new dirty flag mechanism that tracks when configuration changes are pending application. The test flow properly validates:
- Initial clean state
- Dirty flag set on configuration changes
- Dirty flag persistence through failed configurations
- Dirty flag clearing after successful application
This aligns well with the improved SED configuration management described in the PR objectives.
2126-2272: LGTM! Proper testing of Scan motion configuration dirty flags.The motion configuration tests correctly implement the same dirty flag testing pattern for Scan nodes, with appropriate cache key updates using
CACHE_SCAN_prefixes. The test flow properly validates the configuration state management for motion-related settings.
2420-2472: LGTM! Proper test setup for refactored node discovery.The new fake_cache function correctly uses updated cache key naming conventions, and the test properly synchronizes with the async discovery process. The node count assertion and loaded state verification ensure proper test coverage of the discovery and loading functionality.
2572-2572: LGTM! Node loaded state verification.Good addition to verify that the node loading process completed successfully.
plugwise_usb/nodes/sed.py (8)
5-5: LGTM - Import additions support new functionality.The additional asyncio imports and BatteryConfig import align well with the dirty flag refactoring and enhanced async task management.
Also applies to: 12-12
26-33: LGTM - Improved cache key organization.The CACHE_SED_ prefix provides better namespacing and the additional constants support the new dirty flag functionality and enhanced cache management.
87-87: LGTM - Constructor aligns with address parameter removal.The removal of the address parameter from the super constructor call is consistent with the PR's goal of eliminating unused address propagation. The new task scheduling flag supports the enhanced awake task management.
Also applies to: 93-93
148-174: LGTM - Solid dirty flag handling logic.The dirty flag logic correctly handles missing cache values by setting defaults and marking them as dirty. The bitwise OR operation properly combines with the cached dirty state, ensuring configuration updates are applied when needed.
179-223: LGTM - Consistent cache loading helpers.The cache loading methods are well-structured with proper null handling and consistent return types. The new
_sed_config_dirty_from_cachemethod correctly defaults toTruewhen no cached dirty state exists, ensuring configuration is applied when uncertain.
238-325: LGTM - Clean configuration setter pattern.The setter methods follow a consistent, immutable pattern using dataclass
replace()with proper validation, change detection, and dirty flag management. This approach ensures configuration changes are properly tracked for later application.
388-400: LGTM - Improved task execution logic.The updated
_configure_sed_taskproperly checks the dirty flag before proceeding, and the new_run_awake_tasksmethod provides a clean consolidation of awake task handling. The logic flow is correct and improves maintainability.Also applies to: 544-554
555-620: LGTM - Streamlined SED configuration management.The configuration methods now directly use
_battery_configfields, properly manage the dirty flag lifecycle, and use the correct cache constants. The separation of concerns between configuration application and cache updates improves code clarity.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugwise_usb/network/registry.py (1)
102-107: Update callback signature to match actual usage.The callback signature suggests it takes
(str, NodeType | None, bool)parameters, but looking at the usage in the codebase, it appears the callback is invoked differently. Please verify the signature matches the actual method being passed as callback.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
plugwise_usb/network/__init__.py(11 hunks)plugwise_usb/network/cache.py(1 hunks)plugwise_usb/network/registry.py(8 hunks)plugwise_usb/nodes/scan.py(11 hunks)plugwise_usb/nodes/sed.py(16 hunks)tests/test_usb.py(28 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- plugwise_usb/network/cache.py
- plugwise_usb/nodes/scan.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_colle...
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/network/registry.pytests/test_usb.pyplugwise_usb/network/__init__.py
📚 Learning: in the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `none` means th...
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:
tests/test_usb.pyplugwise_usb/network/__init__.py
🧬 Code Graph Analysis (1)
plugwise_usb/network/registry.py (3)
plugwise_usb/api.py (2)
NodeType(61-72)mac(316-317)plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/network/cache.py (3)
update_nodetypes(62-75)prune_cache(81-90)nodetypes(23-25)
⏰ 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 (54)
tests/test_usb.py (12)
504-504: LGTM: Debug logging enhancementGood addition for debugging node awake events. The log format correctly captures both MAC and event type.
567-568: LGTM: Proper test synchronizationGood addition to wait for scan completion before assertions. This properly synchronizes the test with the refactored asynchronous node discovery process.
694-698: LGTM: Updated discovery assertionsThe scan completion wait and updated node count assertions properly reflect the refactored discovery process. The change from joined_nodes count to discovered nodes length is consistent with the codebase changes.
770-771: LGTM: Consistent test synchronizationConsistent application of scan completion synchronization across test methods.
913-936: LGTM: Updated cache handling for SED nodesThe cache method updates properly reflect the new cache key naming scheme with
CACHE_SED_prefixes and the introduction of boolean cache values. The addition of_get_cache_as_boolmonkeypatch aligns with the cache restructuring.
972-982: LGTM: Updated configuration state testingThe shift from checking scheduling flags to testing
battery_config.dirtyproperly reflects the new configuration state management with dirty flags introduced in the refactoring.
1021-1021: LGTM: Updated validation boundaryThe maintenance interval validation limit change to 1500 reflects updated business logic constraints.
2109-2146: LGTM: Updated cache handling for Scan nodesThe cache method updates for Scan nodes properly implement the new
CACHE_SCAN_prefix naming scheme and boolean cache values, maintaining consistency with the SED node cache updates.
2178-2188: LGTM: Consistent motion configuration testingThe motion configuration dirty flag testing maintains consistency with the new configuration state management approach across different node types.
2311-2336: LGTM: Consistent Switch node cache updatesSwitch node cache updates follow the established pattern, properly using the appropriate cache key prefixes and boolean value support.
2414-2439: LGTM: Comprehensive cache setup for discovery testingThe comprehensive fake_cache method ensures all necessary cache values are available for proper testing of the node discovery process with the new cache structure.
2565-2565: LGTM: Node loading validationThe assertion properly validates that the expected node is loaded during the discovery process, reflecting the updated discovery behavior.
plugwise_usb/network/__init__.py (12)
80-80: LGTM! Simplified stragglers tracking.The change from a dictionary to a list for tracking registry stragglers is consistent with the MAC-only refactoring, removing the complexity of address-based tracking.
235-241: LGTM! Consistent MAC-only registration check.The updated call to
node_is_registeredwith MAC only is consistent with the refactored registry interface.
248-249: LGTM! Updated battery-powered node discovery.The method call now correctly uses MAC-only parameters, aligning with the refactored discovery interface.
280-288: LGTM! Simplified node rejoin handling.The rejoin logic now uses MAC-only operations, consistent with the registry refactoring. The logic correctly checks for successful registration update before triggering discovery.
334-335: LGTM! Simplified coordinator discovery.The discovery call now uses MAC-only parameters, removing unnecessary address handling.
368-368: LGTM! Updated network node type registration.The method call correctly uses MAC-only parameters for updating node types in the registry.
388-388: LGTM! Battery-powered node discovery simplified.The discovery call now uses MAC-only parameters, consistent with the refactored interface.
413-413: LGTM! Simplified node object creation and stragglers handling.The node creation and stragglers management now use MAC-only operations. The logic correctly appends the MAC to the stragglers list when node details cannot be retrieved.
Also applies to: 424-425
437-443: LGTM! Fixed list modification during iteration.The stragglers discovery logic now correctly:
- Creates a copy for safe iteration
- Uses MAC-only discovery calls
- Removes MACs from stragglers when discovery succeeds
- Uses appropriate list operations
This addresses the critical bug mentioned in previous reviews.
485-489: LGTM! Proper task management for load stragglers.The task creation logic correctly checks if a task is already running before creating a new one, preventing duplicate task execution.
498-512: LGTM! Enhanced start method with callback setup.The start method improvements:
- Accepts a
loadparameter for configuration- Properly sets up registry callbacks for node discovery and scan completion
- Correctly manages stragglers discovery task creation
- Uses appropriate task state checking
The callback setup aligns with the refactored registry interface.
517-517: LGTM! Proper parameter propagation.The
discover_nodesmethod correctly passes theloadparameter to thestartmethod, maintaining consistent behavior.plugwise_usb/network/registry.py (13)
9-9: LGTM! Clean constant definitions.The addition of
Finalimport and scanning interval constants improves code maintainability and type safety.Also applies to: 26-28
46-46: LGTM! Simplified registry structure.The change from dictionary to list simplifies the registry by focusing solely on MAC addresses, consistent with the broader refactoring.
108-110: LGTM! Clean callback registration.The scan completed callback registration is well-implemented with clear naming and documentation.
122-125: LGTM! Streamlined start method.The updated start method properly loads registrations from cache and creates the unified scanning task. The task creation approach aligns with the comment from bouwew about using create_task for background processing.
152-158: LGTM! Simplified registration check.The updated
node_is_registeredmethod correctly uses list membership checking, consistent with the new registry structure.
165-170: LGTM! Simplified node type update.The method correctly updates node types in cache using MAC addresses only, consistent with the refactoring.
171-177: LGTM! Clean list-based registration.The registration update method properly:
- Checks for empty or duplicate MACs
- Uses list append operation
- Returns boolean indicating if registration was new
The logic is clear and correct for the list-based approach.
178-184: LGTM! Proper removal with cache pruning.The removal method correctly:
- Removes MAC from the registry list
- Prunes cache to maintain consistency
- Uses proper list operations
The cache pruning ensures no stale entries remain.
185-212: LGTM! Unified scanning approach.The refactored scanning method effectively:
- Scans all addresses 0-63 in a unified approach
- Maintains local registry for cache pruning
- Properly invokes discovery callbacks
- Sets scan completion flag
- Uses proper sleep intervals
This consolidation simplifies the scanning logic compared to separate quick/full scan methods.
213-230: LGTM! Adaptive cache loading.The cache loading method effectively:
- Adapts scanning pace based on cached node count (< 4 nodes uses quick scan)
- Properly loads from cache with node type information
- Invokes discovery callbacks with correct parameters
- Handles scan completion callback
The adaptive approach optimizes performance based on cache content.
231-234: LGTM! Clean method delegation.The simplified
update_node_registrationmethod properly delegates to the main registration update method.
251-253: LGTM! Updated registration flow.The registration method correctly:
- Uses the updated registration method
- Invokes discovery callback on successful registration
The flow is consistent with the refactored approach.
278-280: LGTM! Updated unregistration flow.The unregistration method correctly uses the updated registry methods for checking registration status and removing entries.
plugwise_usb/nodes/sed.py (17)
5-5: LGTM! Clean import optimization.The import updates remove unused dependencies and focus on the required classes, improving code clarity.
Also applies to: 12-12
26-34: LGTM! Standardized cache key naming.The new
CACHE_SED_prefixed constants provide clear, consistent naming for cache keys, improving maintainability and reducing naming conflicts.
87-87: LGTM! Constructor aligned with refactoring.The super() call correctly omits the address parameter, consistent with the broader address removal refactoring.
93-93: LGTM! Simplified task scheduling.The simplified task scheduling flag aligns with the new configuration management approach.
145-178: LGTM! Excellent cache loading refactoring.The refactored
_load_from_cachemethod provides significant improvements:
- Returns boolean success flag for better error handling
- Sets reasonable defaults when cache entries are missing
- Properly marks configuration as dirty when defaults are used
- Consolidates all battery configuration into a single object
- Triggers immediate configuration update when dirty
This approach is much cleaner than the previous implementation.
179-223: LGTM! Consistent cache loading methods.The individual cache loading methods correctly:
- Use the new
CACHE_SED_prefixed keys- Return
Nonewhen cache entries are missing- Handle type conversion properly
- Maintain clean separation of concerns
The consistent approach makes the code more maintainable.
238-325: LGTM! Simplified setter methods with dirty flag.The refactored setter methods provide several improvements:
- Use
replace()for immutable updates of battery configuration- Mark configuration as dirty when changes are made
- Remove complex scheduling logic in favor of simple dirty tracking
- Maintain consistent error handling and validation
This approach is much cleaner and more predictable than deferred scheduling.
330-333: LGTM! Clean dirty flag exposure.The
dirtyproperty provides clean access to the configuration dirty state, enabling external monitoring of pending changes.
350-350: LGTM! Complete battery config state.The battery_config property correctly includes the dirty flag, providing complete configuration state information.
390-400: LGTM! Simplified configuration task.The
_configure_sed_taskmethod is now much cleaner:
- Simple dirty flag check
- Direct call to configuration method
- Clear success/failure handling
This is more straightforward than the previous implementation.
410-410: LGTM! Updated cache key usage.The awake timestamp caching correctly uses the new
CACHE_SED_AWAKE_TIMESTAMPkey.
423-424: LGTM! Simplified awake task execution.The awake task execution is now cleaner and more direct.
436-456: LGTM! Consistent awake reason caching.The awake reason handling correctly uses the new
CACHE_SED_AWAKE_REASONkey across all awake types, maintaining consistency.
486-486: LGTM! Updated maintenance interval caching.The maintenance interval detection correctly uses the new
CACHE_SED_MAINTENANCE_INTERVALkey across all code paths.Also applies to: 493-493, 501-502
544-554: LGTM! Streamlined awake tasks execution.The
_run_awake_tasksmethod is now much cleaner:
- Handles node info updates when scheduled
- Executes configuration updates when dirty
- Simple, clear logic flow
This is a significant improvement over complex task queuing.
555-592: LGTM! Improved SED configuration method.The
sed_configuremethod improvements:
- Uses consolidated battery configuration properties
- Clears dirty flag on successful configuration
- Triggers configuration update to save cache
- Maintains proper error handling and logging
The approach is cleaner and more reliable than the previous implementation.
594-620: LGTM! Comprehensive configuration update.The
_sed_configure_updatemethod effectively:
- Uses all new
CACHE_SED_prefixed keys- Saves complete battery configuration state including dirty flag
- Publishes feature updates to subscribers
- Uses proper async gathering for concurrent operations
The method ensures cache and subscribers stay synchronized with configuration changes.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/scan.py(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `none` means th...
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/scan.py
📚 Learning: in plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_colle...
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/scan.py
⏰ 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 (10)
plugwise_usb/nodes/scan.py (10)
35-42: Cache key naming convention looks good.The consistent use of
CACHE_SCAN_prefixes aligns well with the broader refactoring pattern mentioned in the summary. This standardization improves maintainability and reduces naming conflicts.
88-88: Constructor simplification is correct.Removing the
addressparameter from the parent constructor call aligns with the broader shift away from address-based node identification mentioned in the AI summary.
152-179: Dirty flag initialization and cache loading logic is well-implemented.The approach of initializing
dirty = Falseand setting it toTruewhen cache values are missing ensures proper configuration management when the cache is empty. The conditional call to_scan_configure_update()when dirty is set provides immediate configuration updates.
181-220: Cache loading methods have consistent null handling.All cache loading methods properly handle
Nonevalues and return appropriate defaults orNonewhere expected. The_motion_config_dirty_from_cache()method correctly defaults toTruewhen no cached value exists, which is the safe approach for ensuring configuration updates.
232-236: Clean dirty flag property implementation.The property provides direct access to the motion config's dirty state, which simplifies external access to the configuration status.
303-309: Configuration update pattern is consistent and correct.The use of
replace()withdirty=Truefollowed by_scan_configure_update()provides a clean pattern for tracking and applying configuration changes. This pattern is consistently applied across all configuration methods.
428-434: Awake task execution logic is sound.The
_run_awake_tasks()method properly calls the parent implementation and then handles motion configuration updates when the dirty flag is set. This ensures configuration changes are applied when the node becomes available.
436-443: Configuration task logic has early return optimization.The early return when
dirtyisFalseavoids unnecessary configuration requests, which is an efficient approach.
445-477: Scan configuration method is robust.The method properly handles different response types and includes appropriate logging for success, failure, and unexpected response cases. The dirty flag is correctly reset to
Falseonly upon successful configuration.
479-497: Cache update method is comprehensive.All configuration values are properly cached with appropriate string conversions, and the method includes both feature update notifications and cache persistence.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/nodes/scan.py(9 hunks)tests/test_usb.py(30 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise_usb/nodes/scan.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_colle...
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:
tests/test_usb.py
📚 Learning: in the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `none` means th...
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:
tests/test_usb.py
🪛 GitHub Actions: Latest commit
tests/test_usb.py
[error] 2236-2236: SyntaxError: Expected a statement at line 2236.
[error] 2236-2236: SyntaxError: Expected a statement at line 2236 (column 10).
🔇 Additional comments (1)
tests/test_usb.py (1)
504-504: Good addition for test observabilityThe debug log statement enhances test diagnostics by logging node MAC and event details during awake callbacks.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugwise_usb/nodes/scan.py (1)
136-146: Reset the update flag at the start of_load_defaultsto avoid stale scheduling.The flag
_sed_node_info_update_task_scheduledshould be reset at the start of the method to prevent carrying over from previous loads.async def _load_defaults(self) -> None: """Load default configuration settings.""" + self._sed_node_info_update_task_scheduled = False await super()._load_defaults()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/scan.py(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: in the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `none` means th...
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/scan.py
📚 Learning: in plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_colle...
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/scan.py
🔇 Additional comments (10)
plugwise_usb/nodes/scan.py (10)
86-86: LGTM - Constructor updated correctly for address parameter removal.The removal of the
addressparameter from the superclass call aligns with the broader refactor to eliminate address-based node handling.
28-34: LGTM - Improved cache key naming with consistent prefixes.The renaming to
CACHE_SCAN_*prefixes improves namespacing and consistency across the codebase.
148-177: LGTM - Proper dirty flag implementation with default handling.The dirty flag logic correctly:
- Tracks when defaults are used due to missing cache values
- Combines dirty flags using bitwise OR
- Triggers configuration updates when dirty
- Properly tracks super method success
179-204: LGTM - Proper None return pattern for missing cache values.Returning
Nonewhen cache values are missing allows the caller to properly handle defaults and dirty flag logic.
230-233: LGTM - Clean dirty flag property implementation.Simple and consistent property that provides access to the motion configuration dirty flag.
301-347: LGTM - Consistent configuration setter pattern with immediate updates.All three setter methods properly:
- Check for actual value changes before proceeding
- Use immutable dataclass updates with
replace()- Set the dirty flag when changes occur
- Trigger immediate configuration updates
369-424: LGTM - Motion state update properly uses dirty flags and new cache keys.The method correctly:
- Uses the new
CACHE_SCAN_*cache key constants- Sets dirty flags when configuration is modified
- Calls configuration update after changes
- Maintains proper motion detection logic
426-432: LGTM - Proper awake tasks implementation with dirty flag handling.The method follows the correct pattern by calling the super method first and handling dirty configuration updates when the node is awake.
434-472: LGTM - Robust scan configuration with proper dirty flag management.The configuration methods correctly:
- Guard against unnecessary work by checking dirty flags
- Handle different response types with appropriate logging
- Clear dirty flag only on successful configuration
- Persist changes through configuration update calls
474-492: LGTM - Clean configuration update method with proper cache management.The method properly updates all configuration cache values using the new cache key constants, publishes updates to subscribers, and saves the cache efficiently using
gather().
bouwew
force-pushed
the
mdi_CPRegistry
branch
2 times, most recently
from
31ae902 to
0e70089
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugwise_usb/network/registry.py (1)
146-149: create_task is appropriate here to avoid blocking startup.Spawning the full-scan as a background task aligns with the requirement to not block initialization when cache-driven discovery runs first. _registration_task keeps a handle for cancellation.
🧹 Nitpick comments (4)
plugwise_usb/network/registry.py (4)
46-46: Optional: use a set for _registry to ensure uniqueness under concurrency.Concurrent adds (manual register + scan) can race and generate duplicates in a list. A set avoids this and keeps checks O(1) without changing the external API.
- self._registry: list[str] = [] + # Track unique MACs internally to prevent duplicates under concurrent updates + self._registry: set[str] = set()- def registry(self) -> list[str]: - """Return list with mac's of all joined nodes.""" - return deepcopy(self._registry) + def registry(self) -> list[str]: + """Return list with MACs of all joined nodes.""" + # Preserve external list semantics; detach from internal set + return list(self._registry)- if mac == "" or mac in self._registry: + if mac == "" or mac in self._registry: return False - self._registry.append(mac) + self._registry.add(mac) return True- if mac in self._registry: - self._registry.remove(mac) + if mac in self._registry: + self._registry.discard(mac) if self._network_cache is not None: - await self._network_cache.prune_cache(self._registry) + await self._network_cache.prune_cache(list(self._registry))Note: membership checks elsewhere continue to work unchanged.
Also applies to: 93-95, 189-195, 196-202
176-182: Fix node_is_registered docstring to reflect return type and behavior.The function returns a boolean, not an address.
- def node_is_registered(self, mac: str) -> bool: - """Return the network registration address for given mac.""" + def node_is_registered(self, mac: str) -> bool: + """Return True if the given MAC is registered in the network registry."""
142-144: Nit: improve start() docstring wording.Current: “Initialize load the network registry.” Suggest clearer phrasing.
- """Initialize load the network registry.""" + """Initialize and load the network registry."""
26-28: Optional: clarify units in constant names.Consider suffixing with _DELAY_S to reflect seconds and improve readability.
-CIRCLEPLUS_SCANREQUEST_MAINTENANCE: Final = 10 -CIRCLEPLUS_SCANREQUEST_QUICK: Final = 0.1 +CIRCLEPLUS_SCANREQUEST_MAINTENANCE_DELAY_S: Final = 10 +CIRCLEPLUS_SCANREQUEST_QUICK_DELAY_S: Final = 0.1Follow-up: update usages at lines 51, 231, 234 accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/network/registry.py(7 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-09T16:24:49.853Z
Learnt from: dirixmjm
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/network/registry.py:49-52
Timestamp: 2025-08-09T16:24:49.853Z
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.
Applied to files:
plugwise_usb/network/registry.py
📚 Learning: 2025-08-09T16:24:49.853Z
Learnt from: dirixmjm
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/network/registry.py:49-52
Timestamp: 2025-08-09T16:24:49.853Z
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.
Applied to files:
plugwise_usb/network/registry.py
📚 Learning: 2025-08-09T16:24:49.853Z
Learnt from: dirixmjm
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/network/registry.py:49-52
Timestamp: 2025-08-09T16:24:49.853Z
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).
Applied to files:
plugwise_usb/network/registry.py
📚 Learning: 2025-08-07T17:50:29.163Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.163Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.
Applied to files:
plugwise_usb/network/registry.py
📚 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/network/registry.py
📚 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/network/registry.py
🧬 Code Graph Analysis (1)
plugwise_usb/network/registry.py (9)
plugwise_usb/api.py (2)
NodeType(61-72)mac(316-317)plugwise_usb/network/__init__.py (3)
registry(149-151)start(498-511)unregister_node(167-175)plugwise_usb/nodes/node.py (1)
mac(235-237)plugwise_usb/messages/__init__.py (1)
mac(53-57)plugwise_usb/messages/responses.py (1)
node_type(633-635)plugwise_usb/network/cache.py (3)
update_nodetypes(62-75)prune_cache(81-90)nodetypes(23-25)plugwise_usb/__init__.py (1)
unregister_node(333-340)plugwise_usb/helpers/util.py (1)
validate_mac(14-22)plugwise_usb/exceptions.py (1)
NodeError(24-25)
🔇 Additional comments (3)
plugwise_usb/network/registry.py (3)
123-139: Good: guarded discovery-callback wrapper keeps background tasks resilient.The _exec_node_discover_callback addition correctly guards against exceptions and re-raises CancelledError. This prevents a bad listener from crashing the scan.
223-226: Confirm semantics: scan_completed_callback is invoked after cache phase and after full scan.This callback fires twice (once at the end of load_registrations_from_cache and once after full scan). Verify downstream listeners expect multiple invocations; otherwise consider distinct callbacks for “cache phase completed” vs “full scan finished”.
Also applies to: 242-244
228-245: LGTM: cache preload and adaptive scan pace.
- Fast-scan when <4 cached nodes and steady pacing otherwise matches the PR intent.
- Using the guarded _exec_node_discover_callback is correct.
|
dirixmjm
dismissed
coderabbitai[bot]’s stale review
The suggested try/except needs further investigation and is considered outside the scope of this PR.
Improve C+ registry collection and node discovery
Improve handling of SED and Scan configuration data
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Documentation
Chores