Feature Request: add a lock to disable relay-switch-changes (energy devices only) #254
Conversation
![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
🧹 Nitpick comments (1)
plugwise_usb/nodes/node.py (1)
489-489: Remove whitespace from blank line.Static analysis detected whitespace on this blank line which should be cleaned up.
- +🧰 Tools
🪛 Ruff (0.11.9)
489-489: Blank line contains whitespace
Remove whitespace from blank line
(W293)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
plugwise_usb/nodes/circle.py(17 hunks)plugwise_usb/nodes/node.py(11 hunks)plugwise_usb/nodes/sed.py(4 hunks)pyproject.toml(1 hunks)tests/test_usb.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- plugwise_usb/nodes/circle.py
🧰 Additional context used
🪛 Ruff (0.11.9)
plugwise_usb/nodes/node.py
489-489: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/sed.py
600-600: await should be used within an async function
(PLE1142)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Check commit
🔇 Additional comments (21)
plugwise_usb/nodes/sed.py (2)
553-553: LGTM! Cache persistence improvement.Adding cache saving after processing awake responses ensures state persistence across device wake cycles.
764-772: Clean refactoring to match statement.The conversion from if-elif-else to match statement improves readability while maintaining identical logic. The pattern matches handle
INFO,BATTERY, and all other features correctly.tests/test_usb.py (11)
713-721: Good callback implementation for handling multiple features.The callback properly differentiates between
RELAYandRELAY_LOCKfeatures, setting results only forRELAYstates while acknowledgingRELAY_LOCKcallbacks without action. This is a clean approach for testing both features together.
781-782: Verify proper error testing for relay lock.The test correctly verifies that setting relay lock on an unloaded node raises a
NodeError, which is consistent with the existing pattern for theset_relaymethod above.
787-789: Correct initial state verification for relay lock.The test properly verifies that relay lock is initially set to false when not in cache, which aligns with expected default behavior for the feature.
793-793: Proper subscription setup for both relay features.The subscription correctly includes both
RELAYandRELAY_LOCKfeatures, ensuring the callback will be triggered for updates to either feature.
802-810: Comprehensive test coverage for relay lock functionality.This test segment provides excellent coverage of the relay lock feature:
- Activates relay lock and verifies state
- Tests that relay state changes are blocked when lock is active
- Properly raises
NodeErrorwhen attempting blocked operations- Disables relay lock and verifies state change
The test logic is sound and follows good testing practices.
461-461: Consistent feature inclusion in state requests.The addition of
RELAY_LOCKto the state request is consistent with the test's goal of verifying all supported features for the node.
504-504: Proper feature set expansion for relay lock support.The features list correctly includes
RELAY_LOCKalongside existing features, ensuring comprehensive testing of the node's capabilities.
525-525: Correct relay lock state verification.The assertion properly verifies that the relay lock state is initially off, which is the expected default behavior.
530-535: Comprehensive state request for multiple features.The state request includes both
RELAYandRELAY_LOCKfeatures alongside other supported features, providing complete test coverage for the node's functionality.
555-555: Consistent feature set verification.The feature set correctly includes
RELAY_LOCKin the expected features list, ensuring the test validates that the feature is properly supported by the node.
562-562: Proper final state verification for relay lock.The final assertion correctly verifies that the relay lock state remains off, completing the comprehensive test of the relay lock feature.
plugwise_usb/nodes/node.py (8)
26-26: LGTM: RelayLock import added correctly.The import follows the existing pattern and is properly placed alphabetically in the import list.
281-287: LGTM: Fixed feature check for relay_config property.The feature check has been correctly changed from
NodeFeature.RELAYtoNodeFeature.RELAY_INIT, which properly aligns with the relay configuration functionality.
299-304: LGTM: Fixed feature check for relay_state property.The feature check has been correctly changed from
NodeFeature.RELAY_INITtoNodeFeature.RELAY, which properly aligns with the relay state functionality. This completes the fix for the swapped feature checks.
305-311: LGTM: Relay lock property implemented correctly.The new
relay_lockproperty follows the established pattern with proper feature checking, decorator usage, and error handling. The return type and docstring are correct based on the past review feedback.
510-521: LGTM: Enhanced debug logging provides better visibility.The debug logging additions help with troubleshooting and monitoring node detail updates. The logging is appropriately structured across two debug statements for readability.
637-649: LGTM: Match statement refactoring improves consistency.The refactoring from if-elif statements to a match statement aligns this method with similar implementations in other node classes, improving code consistency and readability.
803-806: LGTM: Error message correctly references relay state.Based on past review feedback, the error message has been properly corrected to reference "relay state" instead of "relay-lock", accurately reflecting the method's purpose.
807-814: LGTM: Relay lock setter method implemented correctly.The new
set_relay_lockmethod follows the established pattern with proper feature checking, decorator usage, and error handling. It maintains consistency with other similar setter methods in the class.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (1)
969-972: Fix trailing whitespace.Static analysis detected trailing whitespace on line 970.
- # Change note: _curr_log_addr lags behind c_l_a__pointer so must be < instead of > + # Change note: _curr_log_addr lags behind c_l_a__pointer so must be < instead of >🧰 Tools
🪛 Ruff (0.11.9)
970-970: Trailing whitespace
Remove trailing whitespace
(W291)
plugwise_usb/nodes/node.py (1)
489-489: Remove whitespace from blank line.Static analysis detected whitespace in the blank line.
- +🧰 Tools
🪛 Ruff (0.11.9)
489-489: Blank line contains whitespace
Remove whitespace from blank line
(W293)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/nodes/circle.py(16 hunks)plugwise_usb/nodes/node.py(12 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/circle.py (6)
plugwise_usb/api.py (10)
RelayLock(176-179)relay_lock(381-385)save_cache(330-333)set_relay_lock(438-439)relay_state(388-392)NodeFeature(39-55)mac(278-279)node_info(254-255)get_state(411-415)available_state(270-271)plugwise_usb/nodes/node.py (14)
relay_lock(307-311)save_cache(705-719)_set_cache(689-703)set_relay_lock(808-814)_get_cache(660-664)relay_state(299-303)node_info_update(459-479)_load_from_cache(402-416)initialize(418-426)mac(236-238)node_info(231-233)_node_info_load_from_cache(481-497)get_state(627-650)available_state(97-102)plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/nodes/helpers/__init__.py (1)
raise_not_loaded(26-33)plugwise_usb/messages/responses.py (2)
relay_state(643-645)current_logaddress_pointer(638-640)plugwise_usb/nodes/helpers/subscription.py (1)
publish_feature_update_to_subscribers(51-64)
🪛 Ruff (0.11.9)
plugwise_usb/nodes/circle.py
970-970: Trailing whitespace
Remove trailing whitespace
(W291)
plugwise_usb/nodes/node.py
489-489: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🔇 Additional comments (16)
plugwise_usb/nodes/circle.py (9)
21-21: LGTM: Proper import addition for relay lock feature.The
RelayLockimport is correctly added to support the new relay lock functionality.
57-57: LGTM: Cache constant follows established naming pattern.The
CACHE_RELAY_LOCKconstant follows the established naming convention for cache keys.
89-89: LGTM: Proper initialization of relay lock state.The
_relay_lockattribute is correctly initialized as aRelayLockinstance following the same pattern as other state attributes.
181-184: LGTM: Clean property implementation for relay lock.The
relay_lockproperty provides direct access to the internal state, following the established pattern for other state properties.
645-647: LGTM: Proper relay lock enforcement.The relay lock check correctly prevents relay state changes when the lock is active, raising an appropriate
NodeErrorwith a clear message.
668-672: LGTM: Relay lock setter follows established pattern.The
set_relay_lockmethod correctly delegates to the internal_relay_update_lockmethod and returns the state, following the established pattern for state setters.
693-703: LGTM: Comprehensive relay lock cache handling.The relay lock cache loading logic correctly handles both cached and non-cached scenarios, with proper boolean conversion and fallback to False when not present in cache.
728-745: LGTM: Consistent relay lock update implementation.The
_relay_update_lockmethod follows the exact same pattern as_relay_update_state, ensuring consistent behavior for cache management, state updates, and subscriber notifications.
1192-1220: LGTM: Clean refactor to match statement with proper relay lock handling.The refactored
get_statemethod using amatchstatement improves readability and correctly handles theRELAY_LOCKfeature by returning the internal_relay_lockstate.plugwise_usb/nodes/node.py (7)
26-26: LGTM: Proper import addition for relay lock interface.The
RelayLockimport is correctly added to support the relay lock property interface in the base node class.
77-77: LGTM: Simplified NodeCache initialization.The NodeCache initialization is cleaner without the unnecessary empty string parameter.
281-286: LGTM: Corrected feature check for relay configuration.The
relay_configproperty now correctly checks forRELAY_INITfeature instead ofRELAY, which aligns with the relay initialization configuration purpose.
307-311: LGTM: Proper relay lock property implementation.The
relay_lockproperty correctly checks forRELAY_LOCKfeature support and raises appropriate exceptions, following the established pattern for feature-gated properties.
510-521: LGTM: Helpful debug logging for node details update.The debug logging provides valuable information for troubleshooting node update issues by logging all the key parameters being processed.
637-648: LGTM: Clean refactor to match statement.The refactored
get_statemethod using amatchstatement improves readability and maintainability while preserving the existing functionality.
808-814: LGTM: Proper relay lock setter interface.The
set_relay_lockmethod provides the correct interface with appropriate feature checking and error handling, following the established pattern for configuration methods.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
642-645: Consider improving the return value clarity when relay is lockedThe logic correctly blocks relay switching when locked, but returning
not statemight be confusing. Consider whether returning the current relay state or raising an exception would be clearer.if self._relay_lock.state: _LOGGER.debug("Relay switch blocked, relay is locked") - return not state + # Return current relay state since switch was blocked + return bool(self._relay_state.state)Alternatively, consider raising a specific exception to make it explicit that the operation was blocked:
if self._relay_lock.state: - _LOGGER.debug("Relay switch blocked, relay is locked") - return not state + raise NodeError("Relay switch blocked - relay is locked")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise_usb/nodes/circle.py(16 hunks)pyproject.toml(1 hunks)tests/test_usb.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pyproject.toml
- tests/test_usb.py
🧰 Additional context used
🪛 GitHub Check: codecov/patch
plugwise_usb/nodes/circle.py
[warning] 676-676: plugwise_usb/nodes/circle.py#L676
Added line #L676 was not covered by tests
[warning] 681-681: plugwise_usb/nodes/circle.py#L681
Added line #L681 was not covered by tests
[warning] 689-689: plugwise_usb/nodes/circle.py#L689
Added line #L689 was not covered by tests
[warning] 692-692: plugwise_usb/nodes/circle.py#L692
Added line #L692 was not covered by tests
[warning] 697-698: plugwise_usb/nodes/circle.py#L697-L698
Added lines #L697 - L698 were not covered by tests
[warning] 867-867: plugwise_usb/nodes/circle.py#L867
Added line #L867 was not covered by tests
[warning] 876-876: plugwise_usb/nodes/circle.py#L876
Added line #L876 was not covered by tests
[warning] 884-885: plugwise_usb/nodes/circle.py#L884-L885
Added lines #L884 - L885 were not covered by tests
[warning] 889-889: plugwise_usb/nodes/circle.py#L889
Added line #L889 was not covered by tests
[warning] 894-895: plugwise_usb/nodes/circle.py#L894-L895
Added lines #L894 - L895 were not covered by tests
[warning] 993-993: plugwise_usb/nodes/circle.py#L993
Added line #L993 was not covered by tests
[warning] 997-997: plugwise_usb/nodes/circle.py#L997
Added line #L997 was not covered by tests
[warning] 1191-1192: plugwise_usb/nodes/circle.py#L1191-L1192
Added lines #L1191 - L1192 were not covered by tests
[warning] 1207-1207: plugwise_usb/nodes/circle.py#L1207
Added line #L1207 was not covered by tests
[warning] 1209-1210: plugwise_usb/nodes/circle.py#L1209-L1210
Added lines #L1209 - L1210 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Run pytest using Python 3.13
- GitHub Check: Build and publish Python 🐍 distributions 📦 to TestPyPI
🔇 Additional comments (10)
plugwise_usb/nodes/circle.py (10)
21-21: LGTM - Clean import additionThe
RelayLockimport is properly integrated with the existing API imports.
57-57: LGTM - Consistent cache key namingThe
CACHE_RELAY_LOCKconstant follows the established naming pattern for cache keys.
89-89: LGTM - Proper attribute initializationThe
_relay_lockattribute is correctly initialized with proper type annotation.
181-184: LGTM - Well-implemented propertyThe
relay_lockproperty follows the established pattern and provides clean access to the relay lock state.
666-670: LGTM - Clean relay lock setter implementationThe
set_relay_lockmethod is well-implemented and follows the established async pattern.
674-703: LGTM - Cache loading logic is now correctThe relay and relay lock cache loading implementation looks solid:
- Boolean conversion logic is now correct with
== "True"- Proper if-else structure prevents duplicate state updates
- Result accumulation pattern is consistent with other cache loading methods
- Good debug logging for troubleshooting
The previous syntax and boolean logic issues have been properly addressed.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 676-676: plugwise_usb/nodes/circle.py#L676
Added line #L676 was not covered by tests
[warning] 681-681: plugwise_usb/nodes/circle.py#L681
Added line #L681 was not covered by tests
[warning] 689-689: plugwise_usb/nodes/circle.py#L689
Added line #L689 was not covered by tests
[warning] 692-692: plugwise_usb/nodes/circle.py#L692
Added line #L692 was not covered by tests
[warning] 697-698: plugwise_usb/nodes/circle.py#L697-L698
Added lines #L697 - L698 were not covered by tests
726-743: LGTM - Consistent relay lock update implementationThe
_relay_update_lockmethod follows the same pattern as_relay_update_statewith proper cache handling, state change detection, and feature publication.
802-802: LGTM - Proper feature integrationThe
NodeFeature.RELAY_LOCKis correctly added to the supported features tuple in both cache and non-cache loading paths.Also applies to: 843-843
956-964: LGTM - Current log address caching enhancementThe addition of current log address caching when not present in cache is a good improvement that will help with state persistence.
1189-1217: LGTM - Excellent refactoring to match statementThe conversion from if-elif chains to a match statement improves readability and maintainability. The relay lock case is properly handled, and the fallback to the superclass method is well-implemented.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1191-1192: plugwise_usb/nodes/circle.py#L1191-L1192
Added lines #L1191 - L1192 were not covered by tests
[warning] 1207-1207: plugwise_usb/nodes/circle.py#L1207
Added line #L1207 was not covered by tests
[warning] 1209-1210: plugwise_usb/nodes/circle.py#L1209-L1210
Added lines #L1209 - L1210 were not covered by tests
|
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores