Merge ASYNC-branch into MAIN #236
Conversation
|
""" WalkthroughThis update is a comprehensive refactor and modernization of the Plugwise USB integration. It transitions the codebase from a synchronous, thread-based design to a fully asynchronous, coroutine-based architecture, introducing new modules for connection management, networking, caching, and node handling. Numerous files were added, removed, or extensively rewritten. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Stick
participant Controller
participant Network
participant Node
User->>Stick: connect()
Stick->>Controller: connect_to_stick()
Controller-->>Stick: connection established
User->>Stick: initialize()
Stick->>Controller: initialize_stick()
Controller-->>Stick: stick initialized
User->>Stick: start_network()
Stick->>Network: start()
Network-->>Stick: network started
User->>Stick: discover_nodes()
Stick->>Network: discover_nodes()
Network->>Node: load()
Node-->>Network: node loaded
Network-->>Stick: nodes discovered
User->>Stick: subscribe_to_stick_events()
Stick-->>User: unsubscribe callback
User->>Stick: disconnect()
Stick->>Controller: disconnect_from_stick()
Controller-->>Stick: disconnected
Possibly related PRs
Suggested labels
Poem
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #236 +/- ##
===========================================
+ Coverage 35.14% 81.21% +46.06%
===========================================
Files 21 36 +15
Lines 2404 7363 +4959
===========================================
+ Hits 845 5980 +5135
+ Misses 1559 1383 -176 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🥳 #itshappening |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 43
🔭 Outside diff range comments (1)
scripts/python-venv.sh (1)
4-9:⚠️ Potential issueFix syntax in Bash array and for-loop quoting. The current loop uses
"${pyversions[@]};"which appends a semicolon to each element, causing the checks to fail. Also, semicolons are misplaced. Apply this diff:- pyversions=( 3.13 ) - for pv in "${pyversions[@]};"; do + pyversions=(3.13) + for pv in "${pyversions[@]}"; doThis ensures
${pyversions[@]}expands correctly and iterates over3.13.
🧹 Nitpick comments (44)
plugwise_usb/nodes/stealth.py (1)
6-6: Refine docstring capitalization
PEP 257 recommends that docstrings start with a capital letter. Consider changing to:"""Provides interface to the Plugwise Stealth nodes."""CODEOWNERS (1)
10-10: Use glob pattern for coverage of all files in the directory. The entry/plugwise_usb/matches only the directory itself. To ensure ownership applies to all nested files, replace it with/plugwise_usb/**.- /plugwise_usb/ @plugwise/plugwise-usb + /plugwise_usb/** @plugwise/plugwise-usbrequirements_test.txt (1)
7-9: Pin added test dependencies. To avoid unexpected breaks when new versions are released, consider specifying version constraints for these packages.# Example: pyserial-asyncio-fast==<version> aiofiles==<version> freezegun==<version>CHANGELOG.md (1)
3-17: Add release dates and clarify alpha sequencing. The new sections forv0.40.0 (a22)andv0.40.0 (a4)lack release dates and may confuse readers about the order of alpha releases. Consider:
- Appending dates (e.g.,
## v0.40.0 (a22) - 2025-05-01).- Ordering
a4beforea22(or noting whya22is listed first).This will improve traceability and readability.
plugwise_usb/nodes/helpers/cache.py (1)
22-39: State management methods are functional with minor optimization opportunity.The state management methods work correctly. However, the
remove_statemethod can be simplified:def remove_state(self, state: str) -> None: """Remove configuration state from cache.""" - if self._states.get(state) is not None: - self._states.pop(state) + self._states.pop(state, None)plugwise_usb/nodes/celsius.py (1)
3-4: Address the TODO comment for incomplete implementation.The TODO comment indicates this node implementation is incomplete. This could lead to runtime issues or unexpected behavior in production.
Would you like me to help complete the implementation or create an issue to track this task?
plugwise_usb/nodes/helpers/subscription.py (1)
22-29: Add missing docstring and fix formatting.The
__init__method is missing a docstring and there should be a blank line after the class docstring.Apply this diff to address the style issues:
class FeaturePublisher: """Base Class to call awaitable of subscription when event happens.""" + def __init__(self) -> None: + """Initialize the feature publisher.""" self._feature_update_subscribers: dict[ Callable[[], None], NodeFeatureSubscription, ] = {}🧰 Tools
🪛 Ruff (0.11.9)
23-23: 1 blank line required after class docstring
Insert 1 blank line after class docstring
(D204)
24-24: Missing docstring in
__init__(D107)
plugwise_usb/helpers/util.py (2)
13-22: Consider adding MAC address length validation.The MAC validation correctly checks for uppercase hex characters and parseability, but doesn't validate the expected length. Plugwise MAC addresses typically have a specific length.
Consider adding length validation:
def validate_mac(mac: str) -> bool: """Validate the supplied string is in a MAC address format.""" + if len(mac) != 16: # Adjust length as needed for Plugwise MAC format + return False if not re.match("^[A-F0-9]+$", mac): return False try: _ = int(mac, 16) except ValueError: return False return True
24-24: Fix type annotation spacing.There's inconsistent spacing in the return type annotation.
Apply this diff to fix the spacing:
-def version_to_model(version: str | None) -> tuple[str|None, str]: +def version_to_model(version: str | None) -> tuple[str | None, str]:plugwise_usb/connection/sender.py (1)
51-51: Remove trailing whitespace.Line contains trailing whitespace that should be removed.
- return self._processed_msgs - + return self._processed_msgs +🧰 Tools
🪛 Ruff (0.11.9)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/__init__.py (1)
17-69: Consider refactoring to use dictionary dispatch for better maintainability.The current if-elif chain is repetitive and could be simplified using a dictionary mapping of node types to classes.
+_NODE_TYPE_MAP = { + NodeType.CIRCLE_PLUS: PlugwiseCirclePlus, + NodeType.CIRCLE: PlugwiseCircle, + NodeType.SWITCH: PlugwiseSwitch, + NodeType.SENSE: PlugwiseSense, + NodeType.SCAN: PlugwiseScan, + NodeType.STEALTH: PlugwiseStealth, +} + def get_plugwise_node( mac: str, address: int, controller: StickController, loaded_callback: Callable[[NodeEvent, str], Awaitable[None]], node_type: NodeType, ) -> PlugwiseNode | None: """Return an initialized plugwise node class based on given the node type.""" - - if node_type == NodeType.CIRCLE_PLUS: - return PlugwiseCirclePlus( - mac, - address, - controller, - loaded_callback, - ) - if node_type == NodeType.CIRCLE: - return PlugwiseCircle( - mac, - address, - controller, - loaded_callback, - ) - if node_type == NodeType.SWITCH: - return PlugwiseSwitch( - mac, - address, - controller, - loaded_callback, - ) - if node_type == NodeType.SENSE: - return PlugwiseSense( - mac, - address, - controller, - loaded_callback, - ) - if node_type == NodeType.SCAN: - return PlugwiseScan( - mac, - address, - controller, - loaded_callback, - ) - if node_type == NodeType.STEALTH: - return PlugwiseStealth( - mac, - address, - controller, - loaded_callback, - ) - return None + node_class = _NODE_TYPE_MAP.get(node_type) + if node_class is None: + return None + return node_class(mac, address, controller, loaded_callback)plugwise_usb/nodes/helpers/firmware.py (1)
84-88: Improve consistency of comment placement.The comment placement is inconsistent with the pattern used in other firmware support dictionaries.
Apply this diff to align the comment placement with other dictionaries:
datetime(2011, 5, 13, 7, 21, 55, tzinfo=UTC): SupportedVersions(min=2.0, max=2.5), - datetime(2011, 11, 3, 13, 0, 56, tzinfo=UTC): SupportedVersions(min=2.0, max=2.6), # Legrand + datetime(2011, 11, 3, 13, 0, 56, tzinfo=UTC): SupportedVersions(min=2.0, max=2.6), datetime(2011, 6, 27, 8, 55, 44, tzinfo=UTC): SupportedVersions(min=2.0, max=2.5),plugwise_usb/network/cache.py (1)
55-56: Fix typo in comment.Apply this diff to fix the typo:
else: - # legacy data separator can by remove at next version + # legacy data separator can be removed at next version values = _data.split(";")plugwise_usb/connection/queue.py (3)
20-26: Remove unused RequestState dataclass.The
RequestStatedataclass is defined but never used in this file.If this dataclass is not used elsewhere, consider removing it to reduce code clutter:
-@dataclass -class RequestState: - """Node hardware information.""" - - session: bytes - zigbee_address: int - -
147-151: Extract queue depth threshold as a constant.The hardcoded value
3for queue depth threshold should be defined as a module-level constant for better maintainability.Add a constant at the module level:
_MAX_QUEUE_DEPTH = 3Then apply this diff:
- if self._stick.queue_depth > 3: + if self._stick.queue_depth > _MAX_QUEUE_DEPTH: await sleep(0.125) - if self._stick.queue_depth > 3: + if self._stick.queue_depth > _MAX_QUEUE_DEPTH: _LOGGER.warning("Awaiting plugwise responses %d", self._stick.queue_depth)
97-127: Consider improving test coverage.Static analysis indicates that several error handling paths (lines 97, 109, 111, 113) are not covered by tests. While the error handling logic appears correct, comprehensive test coverage would help ensure these edge cases work as expected.
Would you like me to help generate test cases to cover these error handling scenarios?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-97: plugwise_usb/connection/queue.py#L97
Added line #L97 was not covered by tests
[warning] 109-109: plugwise_usb/connection/queue.py#L109
Added line #L109 was not covered by tests
[warning] 111-111: plugwise_usb/connection/queue.py#L111
Added line #L111 was not covered by tests
[warning] 113-113: plugwise_usb/connection/queue.py#L113
Added line #L113 was not covered by testsplugwise_usb/connection/manager.py (1)
115-121: Consider using a single-line string or f-string for clarity.The error message string concatenation could be cleaner.
if self._receiver is None or not self._receiver.is_connected: raise StickError( - "Unable to subscribe to node response when receiver " + "is not loaded" + "Unable to subscribe to node response when receiver is not loaded" )plugwise_usb/nodes/helpers/counter.py (2)
134-140: Use more descriptive variable name for clarity.The variable name
node_eventis misleading as it's iterating over energy types, not node events.@calibration.setter def calibration(self, calibration: EnergyCalibration) -> None: """Energy calibration configuration.""" - for node_event in ENERGY_COUNTERS: - self._counters[node_event].calibration = calibration + for energy_type in ENERGY_COUNTERS: + self._counters[energy_type].calibration = calibration self._calibration = calibration
286-299: Add documentation for complex midnight reset logic.The midnight reset postponement logic is complex and would benefit from explanatory comments to help future maintainers understand the reasoning.
if self._energy_id in ENERGY_DAY_COUNTERS: - # Postpone the last_reset time-changes at day-end until a device pulsecounter resets + # Postpone the last_reset time-changes at day-end until a device pulsecounter resets. + # This ensures we don't miss pulses that occur between midnight and when the device + # actually resets its counter, preventing data loss during the transition period. if last_reset.hour == 0 and ( not pulse_collection.pulse_counter_reset and not self._midnight_reset_passed ): + # Keep yesterday's date as the reset time last_reset = (last_reset - timedelta(days=1)).replace( hour=0, minute=0, second=0, microsecond=0 ) else: + # Device counter has reset or we're past midnight transition if last_reset.hour == 0 and pulse_collection.pulse_counter_reset: self._midnight_reset_passed = True if last_reset.hour == 1 and self._midnight_reset_passed: self._midnight_reset_passed = False last_reset = last_reset.replace(hour=0, minute=0, second=0, microsecond=0)plugwise_usb/connection/__init__.py (1)
232-236: Add type annotation for boolean parameter.The
suppress_node_errorsparameter should have a type annotation.async def send( self, request: PlugwiseRequest, - suppress_node_errors=True, + suppress_node_errors: bool = True, ) -> PlugwiseResponse | None:plugwise_usb/messages/properties.py (1)
390-396: Remove unnecessary int() call.Static analysis correctly identified an unnecessary
int()call that should be a literal.def deserialize(self, val: bytes) -> None: """Convert data into integer value based on log address formatted data.""" if val == b"00000000": - self._value = int(0) + self._value = 0 return Int.deserialize(self, val) self._value = (self.value - LOGADDR_OFFSET) // 32🧰 Tools
🪛 Ruff (0.11.9)
393-393: Unnecessary
intcall (rewrite as a literal)Replace with integer literal
(UP018)
plugwise_usb/network/__init__.py (2)
302-302: Remove whitespace from blank line.# endregion - + # region - Coordinator🧰 Tools
🪛 Ruff (0.11.9)
302-302: Blank line contains whitespace
Remove whitespace from blank line
(W293)
321-327: Improve error message formatting.The error message uses string concatenation across multiple lines which is less readable.
- raise StickError( - "The zigbee network coordinator (Circle+/Stealth+) with mac " - + "'%s' did not respond to ping request. Make " - + "sure the Circle+/Stealth+ is within reach of the USB-stick !", - self._controller.mac_coordinator, - ) from err + raise StickError( + f"The zigbee network coordinator (Circle+/Stealth+) with mac " + f"'{self._controller.mac_coordinator}' did not respond to ping request. " + f"Make sure the Circle+/Stealth+ is within reach of the USB-stick!" + ) from errplugwise_usb/nodes/switch.py (1)
91-91: Add space after # in comment.- #endregion + # endregionplugwise_usb/nodes/helpers/pulses.py (6)
40-97: Consider refactoring to reduce class complexity.The
PulseCollectionclass has over 30 instance variables, which suggests high complexity. Consider:
- Grouping related state into sub-objects (e.g.,
ConsumptionState,ProductionState)- Using dataclasses for the grouped state
- Adding docstrings to explain the purpose of each variable group
This would improve maintainability and make the code easier to understand.
273-276: Fix whitespace formatting issues.def update_pulse_counter( self, pulses_consumed: int, pulses_produced: int, timestamp: datetime ) -> None: """Update pulse counter. - + Both device consumption and production counters reset after the beginning of a new hour. """🧰 Tools
🪛 Ruff (0.11.9)
274-274: Blank line contains whitespace
Remove whitespace from blank line
(W293)
289-291: Remove trailing whitespace.if ( self._pulses_production is not None - and self._pulses_production < pulses_produced + and self._pulses_production < pulses_produced ):🧰 Tools
🪛 Ruff (0.11.9)
290-290: Trailing whitespace
Remove trailing whitespace
(W291)
315-319: Fix whitespace formatting issues.def _update_rollover(self) -> None: """Update rollover states. - + When the last found timestamp is outside the interval `_last_log_timestamp` - to `_next_log_timestamp` the pulses should not be counted as part of the + to `_next_log_timestamp` the pulses should not be counted as part of the ongoing collection-interval. """🧰 Tools
🪛 Ruff (0.11.9)
316-316: Blank line contains whitespace
Remove whitespace from blank line
(W293)
318-318: Trailing whitespace
Remove trailing whitespace
(W291)
344-344: Fix docstring to use imperative mood.- """Helper function for _update_rollover().""" + """Detect rollover condition for _update_rollover()."""🧰 Tools
🪛 Ruff (0.11.9)
344-344: First line of docstring should be in imperative mood: "Helper function for _update_rollover()."
(D401)
1-1035: Consider breaking this module into smaller, focused components.This 1000+ line module handles many responsibilities:
- Log address calculations
- Pulse counting and collection
- Rollover detection
- Missing log detection
- Consumption/production state tracking
Consider splitting into:
PulseLogStorage: Handle log storage and retrievalPulseCalculator: Handle pulse calculations and intervalsLogValidator: Handle missing log detection and validationRolloverDetector: Handle rollover logicThis would improve testability and reduce cognitive load.
🧰 Tools
🪛 Ruff (0.11.9)
274-274: Blank line contains whitespace
Remove whitespace from blank line
(W293)
290-290: Trailing whitespace
Remove trailing whitespace
(W291)
316-316: Blank line contains whitespace
Remove whitespace from blank line
(W293)
318-318: Trailing whitespace
Remove trailing whitespace
(W291)
344-344: First line of docstring should be in imperative mood: "Helper function for _update_rollover()."
(D401)
plugwise_usb/nodes/sense.py (1)
82-88: Add error handling for subscription cleanupThe subscription cleanup could potentially raise an exception. Consider wrapping it in a try-except block to ensure the unload process continues even if cleanup fails.
Apply this diff to add error handling:
async def unload(self) -> None: """Unload node.""" self._loaded = False if self._sense_subscription is not None: - self._sense_subscription() + try: + self._sense_subscription() + except Exception: + _LOGGER.exception("Failed to cleanup sense subscription for node %s", self.mac) await super().unload()plugwise_usb/nodes/node.py (1)
415-422: Remove empty line in conditional blockThere's an empty line (420) in the middle of a conditional statement, which breaks the logical flow and could indicate missing code or a formatting issue.
Apply this diff to fix the formatting:
if self._available == available: if ( self._last_seen is not None and timestamp is not None and (timestamp - self._last_seen).seconds > 5 - ): self._last_seen = timestampplugwise_usb/api.py (1)
286-292: TODO comments need to be addressed.There are TODO comments indicating that
reconnect()anddisconnect()methods should be moved to node with subscription to stick events. This architectural decision should be finalized.Would you like me to help refactor these methods to use the subscription model mentioned in the TODO comments?
plugwise_usb/nodes/scan.py (2)
158-162: Simplify boolean conversion logic.The boolean conversion can be simplified using direct comparison.
-def _daylight_mode_from_cache(self) -> bool: - """Load awake duration from cache.""" - if (daylight_mode := self._get_cache(CACHE_SCAN_DAYLIGHT_MODE)) is not None: - if daylight_mode == "True": - return True - return False - return SCAN_DEFAULT_DAYLIGHT_MODE +def _daylight_mode_from_cache(self) -> bool: + """Load awake duration from cache.""" + if (daylight_mode := self._get_cache(CACHE_SCAN_DAYLIGHT_MODE)) is not None: + return daylight_mode == "True" + return SCAN_DEFAULT_DAYLIGHT_MODE
354-354: Consider using a lower log level for expected operations.If receiving switch group responses is normal operation, consider using
_LOGGER.infoor_LOGGER.debuginstead of_LOGGER.warning.- _LOGGER.warning("%s received %s", self.name, response) + _LOGGER.info("%s received %s", self.name, response)plugwise_usb/connection/receiver.py (2)
247-248: Remove underscore prefix from local variable.The underscore prefix is misleading for a local variable that's only used for logging.
- _raw_msg_data_data = msg_data[2:][: msg_data_length - 4] - _LOGGER.warning("Drop unknown message type %s", str(_raw_msg_data_data)) + raw_msg_data = msg_data[2:][: msg_data_length - 4] + _LOGGER.warning("Drop unknown message type %s", str(raw_msg_data))
504-509: Extract retry limit as a constant.The retry limit should be defined as a constant for better maintainability.
Add at the module level:
MAX_SUBSCRIPTION_RETRIES: Final = 10Then update the code:
- if node_response.retries > 10: + if node_response.retries > MAX_SUBSCRIPTION_RETRIES: _LOGGER.warning( - "No subscriber to handle %s after 10 retries", + "No subscriber to handle %s after %d retries", node_response, + MAX_SUBSCRIPTION_RETRIES, )plugwise_usb/messages/responses.py (3)
69-73: Track the TODO for response type validation.The TODO comment indicates that these response types need validation. This should be tracked properly to ensure it's not forgotten.
Would you like me to create an issue to track the validation of these response types?
783-802: Fix inconsistent indentation in log_data property.The else blocks have an extra space before them, making the indentation inconsistent.
if self.logdate1.value_set: log_data[1] = (self.logdate1.value, self.pulses1.value) else: - log_data[1] = (None, None) + log_data[1] = (None, None) if self.logdate2.value_set: log_data[2] = (self.logdate2.value, self.pulses2.value) else: - log_data[2] = (None, None) + log_data[2] = (None, None) if self.logdate3.value_set: log_data[3] = (self.logdate3.value, self.pulses3.value) else: - log_data[3] = (None, None) + log_data[3] = (None, None) if self.logdate4.value_set: log_data[4] = (self.logdate4.value, self.pulses4.value) else: - log_data[4] = (None, None) + log_data[4] = (None, None)
864-868: Remove unnecessary parentheses in switch_state property.The parentheses around the comparison are redundant.
@property def switch_state(self) -> bool: """Return state of switch (True = On, False = Off).""" - return (self._power_state.value != 0) + return self._power_state.value != 0plugwise_usb/__init__.py (3)
179-183: Simplify redundant port assignment logic.The None check and assignment logic can be simplified.
- if self._port is None: - self._port = port - if port != self._port: - self._port = port + self._port = port
201-206: Fix string concatenation in error message.Missing space between concatenated strings makes the error message hard to read.
if self._network is None or not self._network.is_running: raise StickError( "Cannot accept joining node" - + "without node discovery be activated. Call discover() first." + + " without node discovery be activated. Call discover() first." )
209-213: Add test coverage for error handling paths.The exception handling for
allow_join_requestsis not covered by tests. Consider adding test cases that simulateMessageErrorandNodeErrorconditions.Consider adding unit tests that mock the
_network.allow_join_requests()method to raise these exceptions and verify the error handling behavior.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 211-212: plugwise_usb/init.py#L211-L212
Added lines #L211 - L212 were not covered by testsplugwise_usb/nodes/sed.py (1)
233-236: Consider extracting validation ranges as constants.The validation ranges are hardcoded in multiple methods. Extracting them as module-level constants would improve maintainability and documentation.
Add these constants at the module level (after line 65):
# Validation ranges for SED configuration SED_AWAKE_DURATION_MIN = 1 SED_AWAKE_DURATION_MAX = 255 SED_CLOCK_INTERVAL_MIN = 1 SED_CLOCK_INTERVAL_MAX = 65535 SED_MAINTENANCE_INTERVAL_MIN = 1 SED_MAINTENANCE_INTERVAL_MAX = 1440 # 24 hours SED_SLEEP_DURATION_MIN = 1 SED_SLEEP_DURATION_MAX = 65535Then update the validation checks to use these constants:
- if seconds < 1 or seconds > 255: + if seconds < SED_AWAKE_DURATION_MIN or seconds > SED_AWAKE_DURATION_MAX: raise ValueError( - f"Invalid awake duration ({seconds}). It must be between 1 and 255 seconds." + f"Invalid awake duration ({seconds}). It must be between {SED_AWAKE_DURATION_MIN} and {SED_AWAKE_DURATION_MAX} seconds." )Also applies to: 263-266, 317-320, 350-353
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (55)
.github/workflows/merge.yml(1 hunks).github/workflows/verify.yml(5 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks)CHANGELOG.md(1 hunks)CODEOWNERS(1 hunks)plugwise_usb/__init__.py(1 hunks)plugwise_usb/api.py(1 hunks)plugwise_usb/connection/__init__.py(1 hunks)plugwise_usb/connection/manager.py(1 hunks)plugwise_usb/connection/queue.py(1 hunks)plugwise_usb/connection/receiver.py(1 hunks)plugwise_usb/connection/sender.py(1 hunks)plugwise_usb/connections/__init__.py(0 hunks)plugwise_usb/connections/serial.py(0 hunks)plugwise_usb/connections/socket.py(0 hunks)plugwise_usb/constants.py(2 hunks)plugwise_usb/controller.py(0 hunks)plugwise_usb/exceptions.py(1 hunks)plugwise_usb/helpers/__init__.py(1 hunks)plugwise_usb/helpers/cache.py(1 hunks)plugwise_usb/helpers/util.py(1 hunks)plugwise_usb/messages/__init__.py(1 hunks)plugwise_usb/messages/properties.py(1 hunks)plugwise_usb/messages/requests.py(2 hunks)plugwise_usb/messages/responses.py(4 hunks)plugwise_usb/network/__init__.py(1 hunks)plugwise_usb/network/cache.py(1 hunks)plugwise_usb/network/registry.py(1 hunks)plugwise_usb/nodes/__init__.py(1 hunks)plugwise_usb/nodes/celsius.py(1 hunks)plugwise_usb/nodes/circle.py(1 hunks)plugwise_usb/nodes/circle_plus.py(1 hunks)plugwise_usb/nodes/helpers/__init__.py(1 hunks)plugwise_usb/nodes/helpers/cache.py(1 hunks)plugwise_usb/nodes/helpers/counter.py(1 hunks)plugwise_usb/nodes/helpers/firmware.py(1 hunks)plugwise_usb/nodes/helpers/pulses.py(1 hunks)plugwise_usb/nodes/helpers/subscription.py(1 hunks)plugwise_usb/nodes/node.py(1 hunks)plugwise_usb/nodes/scan.py(1 hunks)plugwise_usb/nodes/sed.py(1 hunks)plugwise_usb/nodes/sense.py(1 hunks)plugwise_usb/nodes/stealth.py(1 hunks)plugwise_usb/nodes/switch.py(1 hunks)plugwise_usb/parser.py(0 hunks)plugwise_usb/util.py(0 hunks)pyproject.toml(7 hunks)requirements_commit.txt(1 hunks)requirements_test.txt(1 hunks)scripts/python-venv.sh(1 hunks)setup.cfg(0 hunks)setup.py(0 hunks)tests/bandit.yaml(0 hunks)tests/stick_test_data.py(1 hunks)
💤 Files with no reviewable changes (9)
- tests/bandit.yaml
- setup.py
- setup.cfg
- plugwise_usb/parser.py
- plugwise_usb/connections/serial.py
- plugwise_usb/connections/socket.py
- plugwise_usb/controller.py
- plugwise_usb/connections/init.py
- plugwise_usb/util.py
🧰 Additional context used
🧬 Code Graph Analysis (13)
plugwise_usb/nodes/helpers/__init__.py (2)
plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/nodes/node.py (2)
is_loaded(203-205)mac(230-232)
plugwise_usb/nodes/helpers/subscription.py (2)
plugwise_usb/api.py (2)
NodeFeature(39-54)features(230-231)plugwise_usb/nodes/node.py (1)
features(185-187)
plugwise_usb/nodes/celsius.py (5)
plugwise_usb/api.py (5)
NodeEvent(30-36)NodeFeature(39-54)load(250-251)is_battery_powered(234-235)mac(270-271)plugwise_usb/nodes/sed.py (3)
load(107-122)_load_from_cache(164-178)initialize(143-152)plugwise_usb/nodes/node.py (6)
load(365-367)is_battery_powered(198-200)mac(230-232)_load_from_cache(388-399)_setup_protocol(319-348)initialize(401-409)plugwise_usb/nodes/switch.py (2)
load(45-63)initialize(66-75)plugwise_usb/nodes/sense.py (2)
load(51-68)initialize(71-80)
plugwise_usb/nodes/helpers/firmware.py (1)
plugwise_usb/api.py (1)
NodeFeature(39-54)
plugwise_usb/nodes/helpers/counter.py (6)
plugwise_usb/api.py (3)
EnergyStatistics(210-222)mac(270-271)energy(331-335)plugwise_usb/exceptions.py (1)
EnergyError(12-13)plugwise_usb/nodes/helpers/__init__.py (1)
EnergyCalibration(14-20)plugwise_usb/nodes/helpers/pulses.py (16)
PulseCollection(49-1034)PulseLogRecord(41-46)collected_logs(99-106)add_empty_log(380-411)add_log(414-448)logs(109-123)update_pulse_counter(270-312)log_interval_consumption(141-143)log_interval_production(146-148)log_addresses_missing(757-759)log_rollover(151-153)recalculate_missing_log_addresses(450-454)production_logging(136-138)last_update(156-158)pulse_counter_reset(161-163)collected_pulses(165-213)plugwise_usb/nodes/node.py (2)
mac(230-232)energy(158-162)plugwise_usb/nodes/circle.py (1)
energy(111-113)
plugwise_usb/helpers/cache.py (4)
tests/test_usb.py (6)
os_path_join(1520-1522)makedirs(1541-1546)makedirs(1644-1649)aiofiles_os_remove(1532-1539)aiofiles_os_remove(1635-1642)exists(208-222)plugwise_usb/exceptions.py (1)
CacheError(8-9)plugwise_usb/network/__init__.py (1)
initialize_cache(115-119)plugwise_usb/network/registry.py (1)
initialize_cache(66-70)
plugwise_usb/connection/manager.py (10)
plugwise_usb/api.py (2)
StickEvent(12-19)mac(270-271)plugwise_usb/exceptions.py (1)
StickError(32-33)plugwise_usb/messages/requests.py (6)
PlugwiseRequest(61-336)seq_id(132-134)seq_id(137-146)resend(330-332)max_retries(315-317)max_retries(320-322)plugwise_usb/connection/receiver.py (7)
StickReceiver(82-514)processed_messages(142-144)correct_processed_messages(151-153)is_connected(147-149)subscribe_to_stick_events(303-320)subscribe_to_node_responses(401-438)close(167-171)plugwise_usb/connection/sender.py (5)
StickSender(33-157)processed_messages(48-50)start(52-66)write_request_to_port(68-144)stop(154-157)plugwise_usb/__init__.py (2)
is_connected(100-102)subscribe_to_stick_events(220-232)plugwise_usb/connection/__init__.py (4)
is_connected(57-59)subscribe_to_stick_events(122-136)_handle_stick_event(153-161)subscribe_to_messages(138-151)plugwise_usb/network/__init__.py (3)
_handle_stick_event(195-209)start(489-496)stop(510-520)plugwise_usb/connection/queue.py (3)
_handle_stick_event(56-61)start(45-54)stop(63-75)tests/test_usb.py (2)
subscribe_to_messages(234-250)close(163-165)
plugwise_usb/messages/properties.py (2)
plugwise_usb/exceptions.py (1)
MessageError(20-21)plugwise_usb/helpers/util.py (1)
int_to_uint(53-58)
plugwise_usb/network/registry.py (6)
plugwise_usb/api.py (5)
NodeType(57-68)mac(270-271)network_address(274-275)save_cache(322-325)clear_cache(319-320)plugwise_usb/exceptions.py (3)
CacheError(8-9)NodeError(24-25)StickError(32-33)plugwise_usb/helpers/util.py (1)
validate_mac(13-21)plugwise_usb/messages/requests.py (20)
NodeAddRequest(408-444)NodeRemoveRequest(853-882)PlugwiseRequest(61-336)response(125-129)send(358-367)send(380-389)send(431-433)send(469-478)send(505-514)send(535-546)send(559-568)send(581-590)send(614-623)send(660-669)send(712-721)send(768-777)send(801-810)send(841-850)send(873-882)send(905-914)plugwise_usb/messages/responses.py (4)
PlugwiseResponse(98-215)node_type(641-643)network_address(556-558)registered_mac(551-553)plugwise_usb/network/cache.py (6)
NetworkRegistrationCache(15-83)restore_cache(45-73)registrations(24-26)update_registration(75-83)save_cache(28-38)clear_cache(40-43)
plugwise_usb/nodes/helpers/pulses.py (6)
plugwise_usb/exceptions.py (1)
EnergyError(12-13)plugwise_usb/messages/responses.py (1)
offset(494-496)plugwise_usb/nodes/helpers/counter.py (8)
timestamp(171-179)is_consumption(230-232)collected_logs(66-68)log_rollover(125-127)last_update(274-276)direction(210-212)add_empty_log(70-72)log_addresses_missing(120-122)plugwise_usb/messages/__init__.py (1)
mac(52-56)plugwise_usb/api.py (1)
mac(270-271)plugwise_usb/nodes/node.py (1)
mac(230-232)
plugwise_usb/nodes/sense.py (7)
plugwise_usb/api.py (9)
NodeEvent(30-36)NodeFeature(39-54)mac(270-271)load(250-251)unload(253-254)temperature(387-391)humidity(338-342)get_state(396-400)features(230-231)plugwise_usb/connection/__init__.py (1)
StickController(31-259)plugwise_usb/exceptions.py (2)
MessageError(20-21)NodeError(24-25)plugwise_usb/messages/responses.py (3)
PlugwiseResponse(98-215)SenseReportResponse(929-943)firmware(636-638)plugwise_usb/nodes/sed.py (5)
NodeSED(70-767)load(107-122)initialize(143-152)unload(124-140)get_state(751-767)plugwise_usb/nodes/helpers/__init__.py (1)
raise_not_loaded(26-33)plugwise_usb/nodes/helpers/subscription.py (1)
publish_feature_update_to_subscribers(51-64)
plugwise_usb/messages/responses.py (4)
plugwise_usb/api.py (1)
NodeType(57-68)plugwise_usb/exceptions.py (1)
MessageError(20-21)plugwise_usb/messages/__init__.py (7)
PlugwiseMessage(22-109)Priority(14-20)identifier(47-49)mac_decoded(59-63)calculate_checksum(75-77)seq_id(37-39)seq_id(42-44)plugwise_usb/messages/properties.py (39)
Bytes(59-76)deserialize(25-27)deserialize(47-52)deserialize(67-69)deserialize(87-89)deserialize(113-118)deserialize(149-152)deserialize(178-180)deserialize(196-200)deserialize(228-237)deserialize(266-269)deserialize(292-294)deserialize(316-323)deserialize(345-352)deserialize(370-373)deserialize(390-396)Int(99-125)String(79-96)value(72-76)value(92-96)value(121-125)value(155-159)value(183-187)value(203-207)value(247-251)value(272-276)value(297-301)value(326-330)value(355-359)value(376-380)value(399-403)UnixTimestamp(162-187)DateTime(210-251)LogAddr(383-403)Float(362-380)RealClockTime(304-330)RealClockDate(333-359)Time(254-276)value_set(240-244)
plugwise_usb/connection/__init__.py (7)
plugwise_usb/api.py (2)
StickEvent(12-19)mac(270-271)plugwise_usb/exceptions.py (2)
NodeError(24-25)StickError(32-33)plugwise_usb/helpers/util.py (1)
version_to_model(24-38)plugwise_usb/messages/requests.py (21)
NodeInfoRequest(885-914)NodePingRequest(593-623)PlugwiseRequest(61-336)StickInitRequest(517-546)seq_id(132-134)seq_id(137-146)send(358-367)send(380-389)send(431-433)send(469-478)send(505-514)send(535-546)send(559-568)send(581-590)send(614-623)send(660-669)send(712-721)send(768-777)send(801-810)send(841-850)send(873-882)plugwise_usb/messages/responses.py (10)
NodeInfoResponse(582-662)NodePingResponse(358-394)PlugwiseResponse(98-215)StickInitResponse(411-457)network_id(446-448)network_online(451-453)mac_network_controller(440-443)hardware(631-633)retries(123-125)retries(128-130)plugwise_usb/connection/manager.py (8)
StickConnectionManager(23-196)is_connected(52-58)serial_path(47-49)setup_connection_to_stick(123-166)subscribe_to_stick_events(84-102)_handle_stick_event(70-82)subscribe_to_messages(104-121)disconnect_from_stick(184-196)plugwise_usb/connection/queue.py (6)
StickQueue(28-156)_handle_stick_event(56-61)start(45-54)is_running(41-43)stop(63-75)submit(77-126)
🪛 Ruff (0.11.9)
plugwise_usb/nodes/helpers/subscription.py
23-23: 1 blank line required after class docstring
Insert 1 blank line after class docstring
(D204)
24-24: Missing docstring in __init__
(D107)
plugwise_usb/connection/sender.py
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
tests/stick_test_data.py
1-1: Missing docstring in public module
(D100)
plugwise_usb/helpers/cache.py
62-62: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/connection/manager.py
40-40: Missing docstring in public method
(D102)
43-43: Missing docstring in public method
(D102)
plugwise_usb/messages/properties.py
393-393: Unnecessary int call (rewrite as a literal)
Replace with integer literal
(UP018)
plugwise_usb/connection/queue.py
79-79: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/helpers/pulses.py
274-274: Blank line contains whitespace
Remove whitespace from blank line
(W293)
290-290: Trailing whitespace
Remove trailing whitespace
(W291)
316-316: Blank line contains whitespace
Remove whitespace from blank line
(W293)
318-318: Trailing whitespace
Remove trailing whitespace
(W291)
344-344: First line of docstring should be in imperative mood: "Helper function for _update_rollover()."
(D401)
plugwise_usb/network/__init__.py
263-263: Blank line contains whitespace
Remove whitespace from blank line
(W293)
289-289: Blank line contains whitespace
Remove whitespace from blank line
(W293)
302-302: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/constants.py
34-34: Trailing whitespace
Remove trailing whitespace
(W291)
🪛 GitHub Check: codecov/patch
plugwise_usb/connection/queue.py
[warning] 97-97: plugwise_usb/connection/queue.py#L97
Added line #L97 was not covered by tests
[warning] 109-109: plugwise_usb/connection/queue.py#L109
Added line #L109 was not covered by tests
[warning] 111-111: plugwise_usb/connection/queue.py#L111
Added line #L111 was not covered by tests
[warning] 113-113: plugwise_usb/connection/queue.py#L113
Added line #L113 was not covered by tests
plugwise_usb/network/__init__.py
[warning] 149-150: plugwise_usb/network/init.py#L149-L150
Added lines #L149 - L150 were not covered by tests
[warning] 152-156: plugwise_usb/network/init.py#L152-L156
Added lines #L152 - L156 were not covered by tests
[warning] 164-167: plugwise_usb/network/init.py#L164-L167
Added lines #L164 - L167 were not covered by tests
[warning] 256-262: plugwise_usb/network/init.py#L256-L262
Added lines #L256 - L262 were not covered by tests
[warning] 264-264: plugwise_usb/network/init.py#L264
Added line #L264 was not covered by tests
[warning] 273-275: plugwise_usb/network/init.py#L273-L275
Added lines #L273 - L275 were not covered by tests
[warning] 277-279: plugwise_usb/network/init.py#L277-L279
Added lines #L277 - L279 were not covered by tests
[warning] 282-283: plugwise_usb/network/init.py#L282-L283
Added lines #L282 - L283 were not covered by tests
plugwise_usb/messages/responses.py
[warning] 1022-1022: plugwise_usb/messages/responses.py#L1022
Added line #L1022 was not covered by tests
plugwise_usb/messages/requests.py
[warning] 360-360: plugwise_usb/messages/requests.py#L360
Added line #L360 was not covered by tests
[warning] 382-382: plugwise_usb/messages/requests.py#L382
Added line #L382 was not covered by tests
[warning] 433-433: plugwise_usb/messages/requests.py#L433
Added line #L433 was not covered by tests
[warning] 507-507: plugwise_usb/messages/requests.py#L507
Added line #L507 was not covered by tests
[warning] 561-561: plugwise_usb/messages/requests.py#L561
Added line #L561 was not covered by tests
[warning] 583-583: plugwise_usb/messages/requests.py#L583
Added line #L583 was not covered by tests
[warning] 714-714: plugwise_usb/messages/requests.py#L714
Added line #L714 was not covered by tests
[warning] 770-770: plugwise_usb/messages/requests.py#L770
Added line #L770 was not covered by tests
[warning] 875-875: plugwise_usb/messages/requests.py#L875
Added line #L875 was not covered by tests
[warning] 964-964: plugwise_usb/messages/requests.py#L964
Added line #L964 was not covered by tests
[warning] 1073-1073: plugwise_usb/messages/requests.py#L1073
Added line #L1073 was not covered by tests
[warning] 1337-1337: plugwise_usb/messages/requests.py#L1337
Added line #L1337 was not covered by tests
[warning] 1408-1408: plugwise_usb/messages/requests.py#L1408
Added line #L1408 was not covered by tests
[warning] 1441-1441: plugwise_usb/messages/requests.py#L1441
Added line #L1441 was not covered by tests
plugwise_usb/__init__.py
[warning] 211-212: plugwise_usb/init.py#L211-L212
Added lines #L211 - L212 were not covered by tests
[warning] 358-361: plugwise_usb/init.py#L358-L361
Added lines #L358 - L361 were not covered by tests
[warning] 369-372: plugwise_usb/init.py#L369-L372
Added lines #L369 - L372 were not covered by tests
🔇 Additional comments (59)
.pre-commit-config.yaml (1)
10-10: Update default Python version to 3.13
The default language version for unspecified hooks was bumped from 3.12 to 3.13. Please verify that all CI environments and local development setups support Python 3.13 and that no hooks or scripts rely on 3.12-specific behavior.requirements_commit.txt (1)
1-1: Document requirements import source
The top comment now indicates that requirements are imported fromproject.dependenciesinstead ofsetup.py. Ensure the consuming scripts (e.g., CI workflows, developer tooling) have been updated to referenceproject.dependenciesaccordingly.plugwise_usb/helpers/__init__.py (1)
1-1: Approve module-level placeholder
A new helpers package entrypoint with a module-level docstring has been added as intended. Looks good to set up thehelpersnamespace..gitignore (1)
14-14: Ignore.venvdirectory
Adding.venvalongsidevenvensures common virtual environment folders are excluded. This aligns with the Python 3.13 venv conventions in the project.requirements_test.txt (1)
1-1: Approve revised comment header. Updating the comment to indicate that test dependencies are imported fromproject.dependenciesis accurate and enhances clarity..github/workflows/merge.yml (1)
16-16: Verify branch name in trigger. Confirm that the branch nameasyncmatches the actual async development branch. If the branch is named differently (e.g.,ASYNC-branch), update this section to ensure CI runs as expected.- - async + - ASYNC-branch.github/workflows/verify.yml (3)
7-7: LGTM! Cache version bump supports the async refactor.The cache version increment from 22 to 24 is appropriate given the extensive dependency and architecture changes in this async refactor.
51-52: Good cleanup of erroneous command prefixes.Removing the
uvprefix from pip install commands fixes what appears to be incorrect command syntax. These should now execute properly in the CI environment.Also applies to: 202-206
100-100: Ruff command syntax updated correctly.The changes align with updated Ruff CLI syntax:
- Commenting out the old
ruff checkcommand- Updating the fix command from
ruff check --fixtoruff --fixAlso applies to: 110-110
plugwise_usb/nodes/helpers/__init__.py (2)
13-21: Well-designed calibration dataclass.The
EnergyCalibrationdataclass provides a clean, type-safe structure for storing the four calibration parameters used by Plugwise Circle and Stealth devices.
26-33:✅ Verification successful
Decorator implementation follows best practices.
The
raise_not_loadeddecorator is well-implemented:
- Uses
@wrapsto preserve function metadata- Proper type annotations with TypeVar
- Clear error message including MAC address for debugging
Verify that the decorator assumptions are met by checking that target methods have
selfas first parameter withis_loadedandmacproperties:
🏁 Script executed:
#!/bin/bash # Description: Verify node classes have required properties for the decorator # Search for classes that might use this decorator ast-grep --pattern $'class $_ { $$$ def is_loaded($_) { $$$ } $$$ def mac($_) { $$$ } $$$ }' # Also search for @property versions rg -A 3 "def is_loaded|def mac" --type pyLength of output: 3087
Decorator assumptions verified: Target Node classes expose both
is_loadedandmacas properties, so theraise_not_loadeddecorator will behave as intended.• plugwise_usb/api.py – defines
@property def is_loaded(self) -> bool: … @property def mac(self) -> str: …• plugwise_usb/nodes/node.py – implements
@property def is_loaded(self) -> bool: … @property def mac(self) -> str: …No further changes required.
pyproject.toml (5)
7-7: Beta version appropriate for major refactor.Version "v0.40.0b2" correctly indicates this is a beta release during the async refactoring process.
29-33: Dependencies updated for async architecture.The dependency changes properly support the async refactor:
- Added
pyserial-asyncio-fastfor async serial communication- Added
aiofilesfor async file operations- Retained essential packages like
crcmodandsemver- Removed sync-only packages no longer needed
46-46: Package discovery and coverage source updated consistently.Both the setuptools package discovery pattern and coverage source have been correctly updated from
plugwise*toplugwise_usb*to match the package rename.Also applies to: 211-211
172-172: Appropriate async test configuration.Setting
asyncio_default_fixture_loop_scope = "session"is a good choice for async testing, allowing fixtures to persist across the entire test session for better performance.
2-2:❓ Verification inconclusive
Verify fixed setuptools version requirement.
The fixed setuptools version (80.3.1) should be verified for compatibility with the project's requirements and build environment.
🌐 Web query:
What is the latest stable version of setuptools and are there any known issues with version 80.3.1?💡 Result:
The latest stable version of setuptools appears to be 80.8.0, which was released on May 20, 2025, just a few days ago[1]. This represents a significant update from previous versions in the 80.x series.
Recent Setuptools Releases
The release timeline for recent versions shows:
- 80.8.0: Released May 20, 2025[1]
- 80.7.1: Released May 14, 2025[3]
- 80.7.0: Released May 14, 2025[3]
- 80.6.0: Released May 14, 2025[3]
Known Issues with Version 80.x Series
There are some important breaking changes and issues with the 80.x series that users should be aware of:
Removal of easy_install: Starting with version 80.0.0, setuptools has completely removed support for
easy_installwhich can disrupt existing workflows[7]. This is documented in the release history as "Removed support for the easy_install command including the sandbox module"[2].Develop Command Changes: The develop command no longer uses easy_install but instead defers execution to pip. Most options to the develop command have been dropped. This is described as "the final warning before the command is dropped completely in a few months"[2].
Compatibility Problems: Some organizations have specifically recommended pinning setuptools to versions below 80.0.0 to avoid these breaking changes. For instance, Palantir has advised users to pin to version <=79.0.1 to prevent the error "Unexpected failure in task condaDevelop"[7].
Removal of Legacy-Editable Installs: Version 79.0.0 (released on April 20, 2025) removed support for 'legacy-editable' installs[2].
While the search results don't specifically mention issues with version 80.3.1, the breaking changes introduced in the 80.x series affect all versions from 80.0.0 onward. Users with workflows that depend on
easy_installor legacy features should carefully consider these changes before upgrading.Citations:
- 1: https://pypi.org/project/setuptools/
- 2: https://setuptools.pypa.io/en/stable/history.html
- 3: https://github.com/pypa/setuptools/releases
- 4: https://www.piwheels.org/project/setuptools/
- 5: https://pydevtools.com/handbook/reference/setuptools/
- 6: https://pypi.org/project/setuptools/3.1/
- 7: https://community.palantir.com/t/important-update-on-setuptools-pinning-the-version-below-80-0-0/3872
- 8: https://pypi.org/project/setuptools/5.8/
- 9: https://setuptools.pypa.io/en/latest/history.html?highlight=py_limited_api
- 10: https://setuptools.pypa.io/en/latest/deprecated/functionalities.html
Review setuptools pin for compatibility
The project currently pins setuptools to 80.3.1 (pyproject.toml, line 2), but all 80.x releases introduce breaking changes—removed
easy_install, altereddevelopbehavior, and dropped legacy-editable installs.• If any build or development workflows rely on those legacy features, consider pinning to ≤ 79.0.1.
• Otherwise, upgrade to the latest stable 80.8.0 (released May 20, 2025) to pick up recent fixes.Please verify your CI/build pipelines handle these 80.x changes before locking in 80.3.1.
plugwise_usb/nodes/helpers/cache.py (2)
12-21: Well-structured cache class initialization.The
NodeCacheclass properly inherits fromPlugwiseCacheand uses MAC address-based file naming, which is logical for node-specific caching.
40-47: Cache save operation implemented correctly.The
save_cachemethod properly delegates to the parent class and includes helpful debug logging.plugwise_usb/exceptions.py (1)
8-46: Well-designed exception hierarchy for async architecture.The refactored exception hierarchy provides good granularity and aligns well with the asynchronous architecture. Each exception type has a clear purpose and follows Python best practices.
plugwise_usb/nodes/helpers/subscription.py (2)
31-49: Solid subscription management pattern.The subscription mechanism using a closure for unsubscribe is well-designed and thread-safe. The pattern allows clean subscription management without external state tracking.
51-64: Efficient concurrent callback execution.The use of
asyncio.gatherto run callbacks concurrently is efficient. Creating a list copy of subscribers before iteration prevents race conditions during subscription changes.plugwise_usb/helpers/util.py (3)
10-10: CRC function setup is correct for Plugwise protocol.The CRC configuration with polynomial 0x11021, no reversal, zero initialization, and no XOR output aligns with Plugwise message integrity requirements.
24-38: Robust version to model mapping with good fallback logic.The function handles multiple fallback strategies for version parsing, including substring extraction and byte order reversal. This provides good resilience for hardware version variations.
43-58: Correct two's complement conversion implementation.Both
uint_to_intandint_to_uintfunctions correctly implement two's complement conversion using bit manipulation. The parameterized bit length (octals << 2) provides flexibility for different data sizes.plugwise_usb/connection/sender.py (1)
111-123:❌ Incorrect review comment
Review the logic for handling ACCEPT responses with existing seq_id.
The current logic raises an error when receiving an ACCEPT response if the request already has a
seq_id. This seems counterintuitive - typically an ACCEPT response indicates success.Is this checking for duplicate sequence IDs or preventing re-assignment? Please verify this logic is correct.
If this is intended behavior, consider adding a comment explaining why an ACCEPT with existing seq_id is an error condition.
🏁 Script executed:
#!/bin/bash set -e # Locate the `write_request_to_port` definition rg --color never -n "def write_request_to_port" -n .Length of output: 179
🏁 Script executed:
#!/bin/bash set -e # Display lines 90-140 to understand the context around the ACCEPT logic sed -n '90,140p' plugwise_usb/connection/sender.pyLength of output: 2457
🏁 Script executed:
#!/bin/bash set -e # Locate StickResponseType definition rg --color never -n "enum class StickResponseType" -n .Length of output: 66
🏁 Script executed:
#!/bin/bash set -e # Locate the definition of PlugwiseRequest to inspect seq_id usage rg --color never -n "class PlugwiseRequest" -n .Length of output: 138
🏁 Script executed:
#!/bin/bash set -e # Find and display the StickResponseType definition and context file=$(rg --color never -l "class StickResponseType" . | head -n1) echo "Found StickResponseType in: $file" sed -n '1,200p' "$file"Length of output: 6493
Ignore the ACCEPT‐logic inversion warning
The code intentionally treats a second ACCEPT on the same request as an error: on first receipt (seq_id is None) it assignsseq_idand subscribes; any subsequent ACCEPT (whenseq_idis already set) rightly indicates a duplicate and is flagged. No change is needed here.Likely an incorrect or invalid review comment.
plugwise_usb/nodes/helpers/firmware.py (1)
1-171: Well-structured firmware compatibility definitions.The module provides a clean and comprehensive mapping of firmware versions to supported protocol ranges for various Plugwise node types. The use of
NamedTuplefor version ranges andFinalfor immutability ensures type safety and prevents accidental modifications.plugwise_usb/messages/__init__.py (5)
14-20: LGTM! Well-designed priority enum.The Priority enum correctly defines message priority levels with CANCEL having the highest priority (0) for immediate processing.
22-64: Well-structured class with proper encapsulation.Good use of private attributes with property accessors, proper error handling for unset MAC addresses, and timezone-aware timestamps.
65-77: Correct message serialization implementation.The method properly constructs messages with identifier, MAC (if set), arguments, checksum, and frame delimiters.
79-93: Correct priority-based comparison logic.The comparison methods properly implement priority ordering where lower numeric values represent higher priority (CANCEL=0 > HIGH=1 > MEDIUM=2 > LOW=3).
103-109: Correct implementation of__le__method.The less than or equal comparison correctly handles priority, sequence ID, and timestamp comparisons.
plugwise_usb/connection/manager.py (3)
123-167: Well-implemented connection setup with proper error handling.The method correctly handles serial connection setup, timeout management, and state initialization.
168-183: Robust message writing with comprehensive error handling.Good implementation with clear error messages and proper validation of request state and sender availability.
184-196: Proper resource cleanup in disconnect method.The method correctly handles unsubscription, stops the sender, closes the receiver, and resets the connection state.
plugwise_usb/nodes/circle_plus.py (2)
24-79: Well-structured async node loading implementation.The method properly handles cache loading, online status verification, node info retrieval, and protocol setup with appropriate error handling and logging.
112-121: Proper clock reset implementation.The clock reset logic correctly sends a set request with current UTC time and validates the response.
plugwise_usb/nodes/helpers/counter.py (2)
17-48: LGTM! Well-structured enum and constants.The
EnergyTypeenum and grouping constants provide a clear and type-safe way to categorize energy measurements.
240-267: Consider the implications of returning negative energy values.The method can return negative energy values when pulses are negative. While the code handles the calculation correctly, negative energy might be unexpected for API consumers. Consider documenting this behavior or ensuring it aligns with the expected use cases.
Would you like me to verify if negative energy values are expected and properly handled by the consumers of this API?
plugwise_usb/connection/__init__.py (1)
110-262: Well-structured asynchronous controller implementation.The
StickControllerclass provides a clean abstraction over the connection manager and queue, with proper error handling and state management throughout.plugwise_usb/messages/properties.py (2)
13-404: Well-designed property type system.The property classes provide a comprehensive and extensible framework for message serialization/deserialization with consistent error handling and clear abstractions.
🧰 Tools
🪛 Ruff (0.11.9)
393-393: Unnecessary
intcall (rewrite as a literal)Replace with integer literal
(UP018)
136-153:⚠️ Potential issueFix incorrect static method usage.
The
negativemethod is defined as static but called as an instance method. This will cause a runtime error.- @staticmethod - def negative(val: int, octals: int) -> int: + def negative(self, val: int, octals: int) -> int: """Compute the 2's compliment of int value val for negative values.""" bits = octals << 2 if (val & (1 << (bits - 1))) != 0: val = val - (1 << bits) return val def serialize(self) -> bytes: """Return current string formatted integer value into an iterable list of bytes.""" fmt = "%%0%dX" % self.length return bytes(fmt % int_to_uint(self._raw_value, self.length), UTF8) def deserialize(self, val: bytes) -> None: """Convert current string formatted value into integer value.""" - # TODO: negative is not initialized! 20220405 self._value = self.negative(int(val, 16), self.length)Alternatively, if it should remain static, call it correctly:
def deserialize(self, val: bytes) -> None: """Convert current string formatted value into integer value.""" - # TODO: negative is not initialized! 20220405 - self._value = self.negative(int(val, 16), self.length) + self._value = SInt.negative(int(val, 16), self.length)Likely an incorrect or invalid review comment.
plugwise_usb/network/__init__.py (5)
35-75: LGTM! Well-structured class initialization.The
StickNetworkclass is well-designed with proper initialization of instance variables and clear separation of concerns. Good use of type hints throughout.
78-120: Clean implementation of cache management properties.The cache properties are well-implemented with proper propagation of settings to both the register and individual nodes. Good validation in
initialize_cache.
456-481: Excellent async implementation for concurrent node loading.The
_load_discovered_nodesmethod efficiently loads multiple nodes concurrently usinggather()and properly tracks results. Good debug logging throughout.
489-521: Well-designed network lifecycle management.The start/stop methods properly manage resources:
- Proper task cancellation in
stop()- Correct order of operations (unsubscribe before unloading nodes)
- Clean async/await usage throughout
540-569: Clean event subscription pattern implementation.The node event subscription mechanism is well-designed with:
- Proper callback registration/deregistration pattern
- Type-safe callback signatures
- Efficient event filtering and distribution
plugwise_usb/nodes/switch.py (3)
32-64: Excellent async refactoring of the switch node.The constructor and
loadmethod are well-implemented:
- Proper initialization of switch-specific state
- Clear async loading pattern with cache support
- Appropriate feature configuration for switch nodes
93-139: Well-designed switch state management.The switch state handling is comprehensive:
- Proper message validation
- Efficient concurrent updates using
gather()- Smart state change detection to avoid redundant updates
- Appropriate cache persistence and subscriber notifications
140-163: Clean implementation of state retrieval.The
get_statemethod properly:
- Validates requested features are supported
- Handles switch-specific state directly
- Delegates other features to the parent class
- Ensures availability is always included
plugwise_usb/nodes/helpers/pulses.py (2)
20-38: Well-implemented address calculation with rollover handling.The
calc_log_addressfunction correctly handles:
- Negative and positive offsets
- Address boundary rollover (0 to LOGADDR_MAX-1)
- Slot adjustments within 1-4 range
165-214: Well-implemented pulse collection logic.The
collected_pulsesmethod properly:
- Validates production logging is enabled when needed
- Handles null cases gracefully
- Provides comprehensive debug logging
- Returns absolute values for energy statistics
plugwise_usb/constants.py (1)
1-91: Well-structured constants module aligned with async architecture.The cleanup and reorganization of constants is appropriate for the new asynchronous design. The removal of detailed protocol constants and focus on foundational parameters supports the modular architecture introduced in this PR.
🧰 Tools
🪛 Ruff (0.11.9)
34-34: Trailing whitespace
Remove trailing whitespace
(W291)
plugwise_usb/api.py (1)
1-680: Excellent API design with comprehensive typing.The introduction of this strongly-typed API module provides a solid foundation for the asynchronous architecture. The use of enums, dataclasses, and Protocol definitions creates a clear contract for node implementations while maintaining type safety throughout the codebase.
plugwise_usb/nodes/circle.py (1)
1-1130: Excellent async refactor with comprehensive error handling.The transformation of the Circle node class to a fully asynchronous implementation is well-executed. The code properly handles:
- Asynchronous relay control and power monitoring
- Energy log retrieval with rollover handling
- Proper cache integration
- Comprehensive error handling and logging
- Task management for background operations
The implementation aligns perfectly with the new async architecture introduced in this PR.
plugwise_usb/messages/responses.py (3)
3-24: LGTM! Well-organized imports.The imports are properly structured with
__future__annotations for better type hint support, and the relative imports follow Python best practices.
26-43: Well-structured constants with clear naming.The constants are properly grouped and follow the Python naming convention. The use of
Finaltype hints ensures immutability.
137-202: Robust message deserialization with comprehensive validation.The
deserializemethod properly validates all aspects of the message structure including header, footer, checksum, identifier, and length. The error messages are descriptive and helpful for debugging.plugwise_usb/nodes/sed.py (2)
35-65: Well-defined constants with clear documentation.The SED-specific constants are properly organized with appropriate default values and helpful comments explaining their purpose.
495-553: Comprehensive awake state management with proper async handling.The
_awake_responsemethod properly handles different awake types, manages state updates, schedules tasks, and maintains cache consistency. The implementation correctly uses async patterns and handles edge cases like duplicate messages.
| async def restore_cache(self) -> bool: | ||
| """Load the previously store state information.""" | ||
| data: dict[str, str] = await self.read_cache() | ||
| self._states.clear() | ||
| for key, value in data.items(): | ||
| self._states[key] = value | ||
| return True |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling to restore_cache method.
The restore_cache method always returns True regardless of whether the cache read operation succeeds. Consider handling potential failures:
async def restore_cache(self) -> bool:
"""Load the previously store state information."""
- data: dict[str, str] = await self.read_cache()
- self._states.clear()
- for key, value in data.items():
- self._states[key] = value
- return True
+ try:
+ data: dict[str, str] = await self.read_cache()
+ self._states.clear()
+ for key, value in data.items():
+ self._states[key] = value
+ return True
+ except Exception:
+ return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def restore_cache(self) -> bool: | |
| """Load the previously store state information.""" | |
| data: dict[str, str] = await self.read_cache() | |
| self._states.clear() | |
| for key, value in data.items(): | |
| self._states[key] = value | |
| return True | |
| async def restore_cache(self) -> bool: | |
| """Load the previously store state information.""" | |
| try: | |
| data: dict[str, str] = await self.read_cache() | |
| self._states.clear() | |
| for key, value in data.items(): | |
| self._states[key] = value | |
| return True | |
| except Exception: | |
| return False |
🤖 Prompt for AI Agents
In plugwise_usb/nodes/helpers/cache.py around lines 53 to 59, the restore_cache
method always returns True without handling errors from the read_cache call.
Modify the method to catch exceptions that may occur during the cache read
operation, handle them appropriately (e.g., logging the error), and return False
if the operation fails. This ensures the method accurately reflects success or
failure of restoring the cache.
| CELSIUS_FEATURES: Final = ( | ||
| NodeFeature.INFO, | ||
| NodeFeature.TEMPERATURE, | ||
| NodeFeature.HUMIDITY, | ||
| ) |
There was a problem hiding this comment.
Feature constant defined but not fully utilized.
The CELSIUS_FEATURES constant includes HUMIDITY but this feature is not passed to _setup_protocol on line 42, creating an inconsistency.
Apply this diff to use the defined constant consistently:
- self._setup_protocol(
- CELSIUS_FIRMWARE_SUPPORT,
- (NodeFeature.INFO, NodeFeature.TEMPERATURE),
- )
+ self._setup_protocol(
+ CELSIUS_FIRMWARE_SUPPORT,
+ CELSIUS_FEATURES,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CELSIUS_FEATURES: Final = ( | |
| NodeFeature.INFO, | |
| NodeFeature.TEMPERATURE, | |
| NodeFeature.HUMIDITY, | |
| ) | |
| # in plugwise_usb/nodes/celsius.py, around line 42 | |
| # previously: | |
| # self._setup_protocol( | |
| # CELSIUS_FIRMWARE_SUPPORT, | |
| # (NodeFeature.INFO, NodeFeature.TEMPERATURE), | |
| # ) | |
| # updated to use the full feature set constant | |
| self._setup_protocol( | |
| CELSIUS_FIRMWARE_SUPPORT, | |
| CELSIUS_FEATURES, | |
| ) |
🤖 Prompt for AI Agents
In plugwise_usb/nodes/celsius.py around lines 16 to 20 and line 42, the
CELSIUS_FEATURES constant includes NodeFeature.HUMIDITY but this feature is not
passed to the _setup_protocol method, causing inconsistency. To fix this, update
the call to _setup_protocol on line 42 to use the CELSIUS_FEATURES constant
instead of a partial or different set of features, ensuring all features defined
in CELSIUS_FEATURES are consistently utilized.
| self._loaded = True | ||
| self._setup_protocol( | ||
| CELSIUS_FIRMWARE_SUPPORT, | ||
| (NodeFeature.INFO, NodeFeature.TEMPERATURE), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move protocol setup before initialization.
The protocol setup should occur before setting _loaded = True to ensure proper feature configuration, consistent with other node implementations.
Apply this diff to move the protocol setup:
+ self._setup_protocol(
+ CELSIUS_FIRMWARE_SUPPORT,
+ CELSIUS_FEATURES,
+ )
self._loaded = True
- self._setup_protocol(
- CELSIUS_FIRMWARE_SUPPORT,
- (NodeFeature.INFO, NodeFeature.TEMPERATURE),
- )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self._loaded = True | |
| self._setup_protocol( | |
| CELSIUS_FIRMWARE_SUPPORT, | |
| (NodeFeature.INFO, NodeFeature.TEMPERATURE), | |
| ) | |
| self._setup_protocol( | |
| CELSIUS_FIRMWARE_SUPPORT, | |
| CELSIUS_FEATURES, | |
| ) | |
| self._loaded = True |
🤖 Prompt for AI Agents
In plugwise_usb/nodes/celsius.py around lines 39 to 43, move the call to
self._setup_protocol before setting self._loaded = True. This ensures the
protocol and features are properly configured before marking the node as loaded,
maintaining consistency with other node implementations.
| if self._cache_enabled: | ||
| _LOGGER.debug( | ||
| "Load Celsius node %s from cache", self._node_info.mac | ||
| ) | ||
| if await self._load_from_cache(): | ||
| pass | ||
|
|
There was a problem hiding this comment.
Incomplete cache loading logic.
The cache loading logic is incomplete compared to other node implementations. When cache loading succeeds, it just passes without proper setup. Additionally, there's no fallback to _load_defaults() when cache is disabled.
Looking at the pattern from plugwise_usb/nodes/sense.py (lines 51-68) and plugwise_usb/nodes/switch.py (lines 45-63), apply this diff to fix the cache loading:
if self._cache_enabled:
_LOGGER.debug(
"Load Celsius node %s from cache", self._node_info.mac
)
- if await self._load_from_cache():
- pass
+ await self._load_from_cache()
+ else:
+ self._load_defaults()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if self._cache_enabled: | |
| _LOGGER.debug( | |
| "Load Celsius node %s from cache", self._node_info.mac | |
| ) | |
| if await self._load_from_cache(): | |
| pass | |
| if self._cache_enabled: | |
| _LOGGER.debug( | |
| "Load Celsius node %s from cache", self._node_info.mac | |
| ) | |
| await self._load_from_cache() | |
| else: | |
| self._load_defaults() |
🤖 Prompt for AI Agents
In plugwise_usb/nodes/celsius.py around lines 32 to 38, the cache loading logic
is incomplete because it only passes when cache loading succeeds and lacks a
fallback to _load_defaults() when cache is disabled. To fix this, update the
code to properly initialize the node when cache loading succeeds instead of
passing, and add a fallback call to _load_defaults() when cache is not enabled,
following the pattern used in plugwise_usb/nodes/sense.py (lines 51-68) and
plugwise_usb/nodes/switch.py (lines 45-63).
| request.assign_error( | ||
| BaseException( | ||
| StickError( | ||
| f"USB-Stick did not respond within {STICK_TIME_OUT} seconds after writing {request}" | ||
| ) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid wrapping StickError in BaseException.
The code wraps StickError exceptions in BaseException, which is unnecessary and may complicate error handling. Consider assigning the error directly.
- request.assign_error(
- BaseException(
- StickError(
- f"USB-Stick did not respond within {STICK_TIME_OUT} seconds after writing {request}"
- )
- )
- )
+ request.assign_error(
+ StickError(
+ f"USB-Stick did not respond within {STICK_TIME_OUT} seconds after writing {request}"
+ )
+ )This same pattern appears in lines 112-116, 129-133, and 136-140. Apply similar changes there as well.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| request.assign_error( | |
| BaseException( | |
| StickError( | |
| f"USB-Stick did not respond within {STICK_TIME_OUT} seconds after writing {request}" | |
| ) | |
| ) | |
| ) | |
| request.assign_error( | |
| StickError( | |
| f"USB-Stick did not respond within {STICK_TIME_OUT} seconds after writing {request}" | |
| ) | |
| ) |
🤖 Prompt for AI Agents
In plugwise_usb/connection/sender.py around lines 98 to 104, the code wraps
StickError inside BaseException when assigning errors, which is unnecessary and
complicates error handling. Remove the BaseException wrapper and assign the
StickError directly to request.assign_error. Apply the same fix to similar
patterns found in lines 112-116, 129-133, and 136-140 by removing the
BaseException wrapper and assigning StickError directly.
| if identifier == NODE_REJOIN_ID: | ||
| return NodeRejoinResponse() |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Add test coverage for NodeRejoinResponse case.
Static analysis indicates that this branch is not covered by tests. Since NODE_REJOIN_ID is already checked earlier for the special sequence ID case, this might be unreachable code or needs specific test coverage.
🏁 Script executed:
#!/bin/bash
# Check if NODE_REJOIN_ID can be reached with a non-special sequence ID
echo "Checking for NODE_REJOIN_ID usage patterns..."
rg -A 5 -B 5 "NODE_REJOIN_ID" --type py
# Check test coverage for get_message_object
echo -e "\nChecking test files for get_message_object coverage..."
fd -e py -x grep -l "get_message_object" {} \; | head -20Length of output: 4128
🏁 Script executed:
#!/bin/bash
set -e
# Show get_message_object implementation around lines 960-1030
sed -n '930,1100p' plugwise_usb/messages/responses.pyLength of output: 3488
Deduplicate or cover the NodeRejoinResponse branch
The identifier == NODE_REJOIN_ID check at lines 1021–1022 is never reached in practice because the sequence‐ID check for REJOIN_RESPONSE_SEQ_ID already returns NodeRejoinResponse. You should either remove this redundant branch or add tests to explicitly cover both ways of creating a NodeRejoinResponse.
• In plugwise_usb/messages/responses.py:
– Sequence check (≈ line 960):
python if seq_id == REJOIN_RESPONSE_SEQ_ID: return NodeRejoinResponse()
– Redundant identifier check (lines 1021–1022):
diff - if identifier == NODE_REJOIN_ID: - return NodeRejoinResponse()
• Add unit tests to assert that:
- A frame with
seq_id == REJOIN_RESPONSE_SEQ_IDreturns aNodeRejoinResponse. - (If you keep it) A frame with
identifier == NODE_REJOIN_IDand a differentseq_idalso returnsNodeRejoinResponse.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 1022-1022: plugwise_usb/messages/responses.py#L1022
Added line #L1022 was not covered by tests
🤖 Prompt for AI Agents
In plugwise_usb/messages/responses.py around lines 1021 to 1022, the check for
identifier == NODE_REJOIN_ID is redundant because the sequence ID check for
REJOIN_RESPONSE_SEQ_ID already returns NodeRejoinResponse, making this branch
unreachable. To fix this, either remove the redundant identifier check branch
entirely or keep it and add unit tests to cover both cases: one where seq_id ==
REJOIN_RESPONSE_SEQ_ID returns NodeRejoinResponse, and another where identifier
== NODE_REJOIN_ID with a different seq_id also returns NodeRejoinResponse.
| """Initialize Stick.""" | ||
| self._loop = get_running_loop() | ||
| self._loop.set_debug(True) | ||
| self._controller = StickController() |
There was a problem hiding this comment.
Remove or make debug mode conditional.
Setting debug mode on the event loop can impact performance in production. This should be controlled by a configuration parameter or environment variable.
def __init__(
self, port: str | None = None, cache_enabled: bool = True
) -> None:
"""Initialize Stick."""
self._loop = get_running_loop()
- self._loop.set_debug(True)
+ # Only enable debug mode in development
+ # self._loop.set_debug(True)
self._controller = StickController()🤖 Prompt for AI Agents
In plugwise_usb/__init__.py around lines 57 to 60, the event loop debug mode is
always enabled, which can degrade production performance. Modify the code to set
debug mode conditionally based on a configuration parameter or environment
variable. Retrieve this setting at runtime and only call
self._loop.set_debug(True) if debugging is enabled, otherwise leave it off.
There was a problem hiding this comment.
The output for https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&pullRequest=236&id=plugwise_python-plugwise-usb shows some valuable ones - specifically the 3 blocking 'same return' ones (and a couple wrt detecting TODOs). It's a long list though so I would just check the top ones to be safe.
For now/future not fix the The complexity ones obviously
The 'merge if-statements' is a nice refactor ones we are in main
The 'define error/logging as constant' can be done later
Same for the better exceptions (or noqa'ing them)
Quicklist
- plugwise_usb/nodes/helpers/pulses.py L939 doesn't do anything
- plugwise_usb/network/init.py L211 always returns true?
- plugwise_usb/nodes/node.py L405 always returns true?
- plugwise_usb/nodes/sed.py L495 always returns true?
- tests/test_usb.py L2632 state is defined but not used (so could be just an await)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (19)
plugwise_usb/connection/__init__.py (1)
131-133: Remove unnecessary None check.The
_managerattribute is always initialized in__init__and never set toNone, making this check redundant.- if self._manager is None: - raise StickError("Connect to stick before subscribing to events") return self._manager.subscribe_to_stick_events(plugwise_usb/nodes/helpers/pulses.py (2)
821-828:⚠️ Potential issueFix missing return value - critical bug.
The method
_logs_missingshould returnlist[int] | Nonebut line 827 has a barereturnstatement.Based on the context and the warning message, it appears you want to return an empty list to indicate no missing logs in this error condition:
if last_address < first_address and (first_address - last_address < 1000): _LOGGER.warning( "The Circle %s does not overwrite old logged data, please reset the Circle's energy-logs via Source", self._mac, ) - return + return []
784-903: 🛠️ Refactor suggestionRefactor complex method for better maintainability.
The
_logs_missingmethod is over 100 lines with deeply nested logic. Consider breaking it down into smaller methods:
- Extract validation logic (lines 786-812) into
_validate_log_references()- Extract missing-in-range logic (lines 838-856) into
_find_missing_in_range()- Extract calculated missing logic (lines 885-902) into
_calculate_missing_before_first()This would improve readability, testability, and maintainability.
plugwise_usb/nodes/sense.py (2)
51-69:⚠️ Potential issueFix inconsistent state when initialization fails.
The node is marked as loaded before verifying that initialization succeeds. If initialization fails, the node remains in an inconsistent state.
Apply this diff to fix the state consistency:
if self._cache_enabled: _LOGGER.debug("Load Sense node %s from cache", self._node_info.mac) if await self._load_from_cache(): - self._loaded = True self._setup_protocol( SENSE_FIRMWARE_SUPPORT, (NodeFeature.INFO, NodeFeature.TEMPERATURE, NodeFeature.HUMIDITY), ) if await self.initialize(): + self._loaded = True await self._loaded_callback(NodeEvent.LOADED, self.mac) return True _LOGGER.debug("Load of Sense node %s failed", self._node_info.mac) return False
91-116:⚠️ Potential issueFix return value logic to properly indicate successful processing.
The method only returns
Truewhen humidity is processed, ignoring cases where only temperature is valid. This could lead to incorrect behavior.Apply this diff to fix the return logic:
async def _sense_report(self, response: PlugwiseResponse) -> bool: """Process sense report message to extract current temperature and humidity values.""" if not isinstance(response, SenseReportResponse): raise MessageError( f"Invalid response message type ({response.__class__.__name__}) received, expected SenseReportResponse" ) await self._available_update_state(True, response.timestamp) + processed = False if response.temperature.value != 65535: self._temperature = int( SENSE_TEMPERATURE_MULTIPLIER * (response.temperature.value / 65536) - SENSE_TEMPERATURE_OFFSET ) await self.publish_feature_update_to_subscribers( NodeFeature.TEMPERATURE, self._temperature ) + processed = True if response.humidity.value != 65535: self._humidity = int( SENSE_HUMIDITY_MULTIPLIER * (response.humidity.value / 65536) - SENSE_HUMIDITY_OFFSET ) await self.publish_feature_update_to_subscribers( NodeFeature.HUMIDITY, self._humidity ) - return True - return False + processed = True + return processedplugwise_usb/nodes/node.py (1)
616-631:⚠️ Potential issueAdd error handling for datetime parsing
The datetime parsing could fail with
ValueErrorif the cached data is corrupted or in an unexpected format.Apply this diff to add error handling:
def _get_cache_as_datetime(self, setting: str) -> datetime | None: """Retrieve value of specified setting from cache memory and return it as datetime object.""" if (timestamp_str := self._get_cache(setting)) is not None: data = timestamp_str.split("-") if len(data) == 6: - return datetime( - year=int(data[0]), - month=int(data[1]), - day=int(data[2]), - hour=int(data[3]), - minute=int(data[4]), - second=int(data[5]), - tzinfo=UTC, - ) + try: + return datetime( + year=int(data[0]), + month=int(data[1]), + day=int(data[2]), + hour=int(data[3]), + minute=int(data[4]), + second=int(data[5]), + tzinfo=UTC, + ) + except ValueError: + _LOGGER.warning( + "Invalid datetime format in cache for setting %s: %s", + setting, + timestamp_str, + ) return Noneplugwise_usb/nodes/scan.py (3)
172-173:⚠️ Potential issueUse
total_seconds()for time comparisonThe
.secondsattribute doesn't include days component. This could cause incorrect motion state if the time difference exceeds 24 hours.and (motion_timestamp := self._motion_timestamp_from_cache()) is not None - and (datetime.now(tz=UTC) - motion_timestamp).seconds < self._reset_timer_from_cache() * 60 + and (datetime.now(tz=UTC) - motion_timestamp).total_seconds() < self._reset_timer_from_cache() * 60 ):
381-381:⚠️ Potential issueUse
total_seconds()for accurate time difference calculationThe
.secondsattribute only returns the seconds component (0-86399) and doesn't include days. Use.total_seconds()for accurate calculations.- reset_timer = (timestamp - self._reset_timer_motion_on).seconds + reset_timer = int((timestamp - self._reset_timer_motion_on).total_seconds())
467-472: 🛠️ Refactor suggestionExtract magic numbers as constants
The sensitivity values should be defined as constants for better maintainability and clarity.
Add these constants at the module level:
# Sensitivity values for motion sensor configuration SENSITIVITY_HIGH_VALUE = 20 # 0x14 SENSITIVITY_MEDIUM_VALUE = 30 # 0x1E SENSITIVITY_OFF_VALUE = 255 # 0xFFThen update the method:
- # Default to medium: - sensitivity_value = 30 # b'1E' - if sensitivity_level == MotionSensitivity.HIGH: - sensitivity_value = 20 # b'14' - if sensitivity_level == MotionSensitivity.OFF: - sensitivity_value = 255 # b'FF' + # Map sensitivity level to value + sensitivity_map = { + MotionSensitivity.HIGH: SENSITIVITY_HIGH_VALUE, + MotionSensitivity.MEDIUM: SENSITIVITY_MEDIUM_VALUE, + MotionSensitivity.OFF: SENSITIVITY_OFF_VALUE, + } + sensitivity_value = sensitivity_map.get(sensitivity_level, SENSITIVITY_MEDIUM_VALUE)plugwise_usb/messages/requests.py (3)
1234-1234: Use debug level for diagnostic loggingThis appears to be diagnostic information rather than a warning condition.
- _LOGGER.warning("NodeSleepConfigRequest result: %s", result) + _LOGGER.debug("NodeSleepConfigRequest result: %s", result)
203-208:⚠️ Potential issueFix incorrect state in stop_response_timeout method
The method sets
_waiting_for_response = Truewhen stopping the timeout, but it should beFalsesince we're no longer waiting.def stop_response_timeout(self) -> None: """Stop timeout for node response.""" - self._waiting_for_response = True + self._waiting_for_response = False if self._response_timeout is not None: self._response_timeout.cancel()
1239-1241:⚠️ Potential issueFix error message to match expected response type
The error message says "expected NodeAckResponse" but the method signature and isinstance check expect "NodeResponse".
raise MessageError( - f"Invalid response message. Received {result.__class__.__name__}, expected NodeAckResponse" + f"Invalid response message. Received {result.__class__.__name__}, expected NodeResponse" )plugwise_usb/messages/responses.py (1)
1013-1014:⚠️ Potential issueRemove or test the redundant NodeRejoinResponse branch.
The check for
identifier == NODE_REJOIN_IDis unreachable because messages withseq_id == REJOIN_RESPONSE_SEQ_IDare already handled at lines 959-960. Either remove this redundant branch or add tests to cover both paths if they represent different scenarios.Apply this diff to remove the redundant branch:
- if identifier == NODE_REJOIN_ID: - return NodeRejoinResponse()plugwise_usb/nodes/circle.py (4)
535-536:⚠️ Potential issueRemove duplicate cache retrieval.
The
cache_datavariable is assigned but not used, and_get_cacheis called again in the if statement.Apply this diff to fix the redundant code:
- cache_data = self._get_cache(CACHE_ENERGY_COLLECTION) if (cache_data := self._get_cache(CACHE_ENERGY_COLLECTION)) is None:
735-737:⚠️ Potential issueRemove duplicate await statement.
The
set_clock_request.send()is awaited twice, which will send the request twice.Apply this diff to fix the duplicate await:
- node_response: NodeResponse | None = await set_clock_request.send() if (node_response := await set_clock_request.send()) is None:
817-822:⚠️ Potential issueFix misleading log message.
The log message incorrectly states that loading energy_log_records failed when it actually succeeded.
Apply this diff to fix the log message:
if await self._energy_log_records_load_from_cache(): - _LOGGER.warning( - "Node %s failed to load energy_log_records from cache", + _LOGGER.debug( + "Node %s successfully loaded energy_log_records from cache", self._mac_in_str, )
830-835:⚠️ Potential issueFix misleading log message.
The log message incorrectly states that loading relay_init state failed when it actually succeeded.
Apply this diff to clarify the log message:
if await self._relay_init_load_from_cache(): _LOGGER.debug( - "Node %s failed to load relay_init state from cache", + "Node %s successfully loaded relay_init state from cache", self._mac_in_str, )plugwise_usb/network/registry.py (2)
124-137: Missing cache_restored flag updateThe method checks
self._cache_restoredbut never sets it toTrueafter loading, which could result in the cache being loaded multiple times.Apply this diff to fix the cache loading logic:
async def load_registry_from_cache(self) -> None: """Load network registry from cache.""" if self._network_cache is None: _LOGGER.error( "Unable to restore network registry because cache is not initialized" ) return if self._cache_restored: return for address, registration in self._network_cache.registrations.items(): mac, node_type = registration if self._registry.get(address) is None: self._registry[address] = (mac, node_type) + self._cache_restored = True
258-263: Fix string formatting errorThe error message uses incorrect string formatting syntax.
Apply this diff to fix the string formatting:
request = NodeAddRequest(self._send_to_controller, bytes(mac, UTF8), True) try: await request.send() except StickError as exc: - raise NodeError("{exc}") from exc + raise NodeError(f"{exc}") from exc
🧹 Nitpick comments (4)
plugwise_usb/nodes/helpers/counter.py (1)
285-298: Complex midnight reset logic may benefit from additional documentation.The logic for postponing day-end reset times until a device pulse counter resets is intricate. Consider adding more detailed comments explaining the rationale and edge cases, particularly around the
_midnight_reset_passedflag behavior.if self._energy_id in ENERGY_DAY_COUNTERS: - # Postpone the last_reset time-changes at day-end until a device pulsecounter resets + # Postpone the last_reset time-changes at day-end until a device pulsecounter resets. + # This ensures energy calculations align with the device's internal reset cycle, + # preventing discrepancies when the device and this code reset at slightly different times. if last_reset.hour == 0 and ( not pulse_collection.pulse_counter_reset and not self._midnight_reset_passed ):plugwise_usb/nodes/helpers/pulses.py (2)
274-274: Fix whitespace and formatting issues.Static analysis detected formatting issues that should be cleaned up.
def update_pulse_counter( self, pulses_consumed: int, pulses_produced: int, timestamp: datetime ) -> None: """Update pulse counter. - + Both device consumption and production counters reset after the beginning of a new hour. """ if ( self._pulses_production is not None - and self._pulses_production < pulses_produced + and self._pulses_production < pulses_produced ): def _update_rollover(self) -> None: """Update rollover states. - + When the last found timestamp is outside the interval `_last_log_timestamp` - to `_next_log_timestamp` the pulses should not be counted as part of the + to `_next_log_timestamp` the pulses should not be counted as part of the ongoing collection-interval. """Also applies to: 290-290, 316-316, 318-318
🧰 Tools
🪛 Ruff (0.11.9)
274-274: Blank line contains whitespace
Remove whitespace from blank line
(W293)
344-344: Fix docstring to use imperative mood.The first line of the docstring should be in imperative mood according to PEP 257.
- """Helper function for _update_rollover().""" + """Detect rollover based on timestamp comparisons."""🧰 Tools
🪛 Ruff (0.11.9)
344-344: First line of docstring should be in imperative mood: "Helper function for _update_rollover()."
(D401)
plugwise_usb/network/__init__.py (1)
266-266: Remove whitespace from blank linesMultiple blank lines contain trailing whitespace which should be removed.
Apply this diff to fix the whitespace issues:
if result: return True - + return False ... return True - + return False ... # endregion - + # region - CoordinatorAlso applies to: 289-289, 302-302
🧰 Tools
🪛 Ruff (0.11.9)
266-266: 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 (14)
plugwise_usb/connection/__init__.py(1 hunks)plugwise_usb/messages/requests.py(2 hunks)plugwise_usb/messages/responses.py(4 hunks)plugwise_usb/network/__init__.py(1 hunks)plugwise_usb/network/registry.py(1 hunks)plugwise_usb/nodes/celsius.py(1 hunks)plugwise_usb/nodes/circle.py(1 hunks)plugwise_usb/nodes/helpers/counter.py(1 hunks)plugwise_usb/nodes/helpers/pulses.py(1 hunks)plugwise_usb/nodes/node.py(1 hunks)plugwise_usb/nodes/scan.py(1 hunks)plugwise_usb/nodes/sed.py(1 hunks)plugwise_usb/nodes/sense.py(1 hunks)plugwise_usb/nodes/switch.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise_usb/nodes/celsius.py
🧰 Additional context used
🧬 Code Graph Analysis (6)
plugwise_usb/nodes/helpers/counter.py (6)
plugwise_usb/api.py (3)
EnergyStatistics(210-222)mac(270-271)energy(331-335)plugwise_usb/exceptions.py (1)
EnergyError(12-13)plugwise_usb/nodes/helpers/__init__.py (1)
EnergyCalibration(14-20)plugwise_usb/nodes/helpers/pulses.py (15)
PulseCollection(49-1032)PulseLogRecord(41-46)collected_logs(99-106)add_empty_log(380-411)add_log(414-448)logs(109-123)update_pulse_counter(270-312)log_interval_consumption(141-143)log_interval_production(146-148)log_addresses_missing(757-759)log_rollover(151-153)recalculate_missing_log_addresses(450-454)last_update(156-158)pulse_counter_reset(161-163)collected_pulses(165-213)plugwise_usb/nodes/node.py (2)
mac(230-232)energy(158-162)plugwise_usb/nodes/circle.py (1)
energy(111-113)
plugwise_usb/messages/requests.py (6)
plugwise_usb/exceptions.py (5)
MessageError(20-21)NodeError(24-25)NodeTimeout(28-29)StickError(32-33)StickTimeout(40-41)plugwise_usb/messages/responses.py (16)
CircleEnergyLogsResponse(737-798)CirclePlusScanResponse(536-562)NodeAckResponse(896-918)NodeFeaturesResponse(865-877)NodeInfoResponse(582-654)NodeResponse(243-270)NodeSpecificResponse(313-327)PlugwiseResponse(98-215)StickResponse(218-240)StickResponseType(45-53)ack_id(133-135)retries(123-125)retries(128-130)network_address(556-558)log_address(770-772)relay_state(643-645)plugwise_usb/messages/__init__.py (8)
PlugwiseMessage(22-109)Priority(14-20)mac(52-56)mac_decoded(59-63)seq_id(37-39)seq_id(42-44)serialize(65-72)calculate_checksum(75-77)plugwise_usb/connection/__init__.py (1)
send(231-242)tests/test_usb.py (1)
send(252-258)plugwise_usb/messages/properties.py (27)
serialize(21-23)serialize(43-45)serialize(108-111)serialize(144-147)serialize(170-176)serialize(287-290)serialize(386-388)Int(99-125)DateTime(210-251)Time(254-276)LogAddr(383-403)String(79-96)RealClockTime(304-330)RealClockDate(333-359)value(72-76)value(92-96)value(121-125)value(155-159)value(183-187)value(203-207)value(247-251)value(272-276)value(297-301)value(326-330)value(355-359)value(376-380)value(399-403)
plugwise_usb/nodes/switch.py (9)
plugwise_usb/api.py (10)
NodeEvent(30-36)NodeFeature(39-54)mac(270-271)load(250-251)unload(253-254)switch(380-384)name(242-243)get_state(396-400)features(230-231)available_state(262-263)plugwise_usb/connection/__init__.py (1)
StickController(31-258)plugwise_usb/exceptions.py (2)
MessageError(20-21)NodeError(24-25)plugwise_usb/messages/responses.py (4)
NodeSwitchGroupResponse(836-863)PlugwiseResponse(98-215)firmware(628-630)switch_state(857-859)plugwise_usb/nodes/sed.py (6)
load(107-122)_load_from_cache(166-180)_load_defaults(156-164)initialize(143-154)unload(124-140)get_state(752-768)plugwise_usb/nodes/helpers/__init__.py (1)
raise_not_loaded(26-33)plugwise_usb/nodes/node.py (12)
mac(230-232)load(365-367)_load_from_cache(388-399)_setup_protocol(319-348)initialize(401-409)unload(602-608)switch(301-305)_available_update_state(411-440)name(213-217)get_state(580-600)features(185-187)available_state(99-104)plugwise_usb/nodes/scan.py (7)
load(82-106)_load_from_cache(142-156)_load_defaults(129-140)initialize(109-120)_switch_group(345-359)unload(122-126)get_state(546-569)plugwise_usb/nodes/helpers/subscription.py (1)
publish_feature_update_to_subscribers(51-64)
plugwise_usb/nodes/sed.py (7)
plugwise_usb/api.py (12)
BatteryConfig(103-121)NodeFeature(39-54)NodeInfo(125-138)mac(270-271)is_battery_powered(234-235)load(250-251)unload(253-254)name(242-243)battery_config(428-439)node_info(246-247)get_state(396-400)features(230-231)plugwise_usb/exceptions.py (2)
MessageError(20-21)NodeError(24-25)plugwise_usb/messages/requests.py (18)
NodeSleepConfigRequest(1185-1245)response(125-129)send(365-374)send(387-396)send(438-440)send(476-485)send(512-521)send(542-553)send(566-575)send(588-597)send(621-630)send(667-676)send(719-728)send(775-784)send(808-817)send(848-857)send(880-889)send(912-921)plugwise_usb/messages/responses.py (6)
NodeAwakeResponse(801-833)NodeAwakeResponseType(87-95)NodeInfoResponse(582-654)NodeResponseType(56-72)PlugwiseResponse(98-215)awake_type(827-829)plugwise_usb/nodes/helpers/__init__.py (1)
raise_not_loaded(26-33)plugwise_usb/nodes/node.py (20)
PlugwiseBaseNode(55-774)mac(230-232)is_battery_powered(198-200)load(365-367)_load_from_cache(388-399)initialize(401-409)unload(602-608)name(213-217)clock_sync(118-124)_get_cache(610-614)_get_cache_as_datetime(616-630)battery_config(108-114)node_info_update(442-462)node_info(225-227)skip_update(662-672)_set_cache(632-645)_available_update_state(411-440)ping_update(549-567)get_state(580-600)features(185-187)plugwise_usb/nodes/helpers/subscription.py (1)
publish_feature_update_to_subscribers(51-64)
plugwise_usb/nodes/circle.py (7)
plugwise_usb/api.py (16)
EnergyStatistics(210-222)NodeFeature(39-54)NodeInfo(125-138)PowerStatistics(152-157)RelayConfig(161-171)RelayState(175-179)mac(270-271)energy(331-335)power(359-363)relay(366-370)relay_config(455-465)name(242-243)relay_state(373-377)load(250-251)get_state(396-400)features(230-231)plugwise_usb/exceptions.py (2)
FeatureError(16-17)NodeError(24-25)plugwise_usb/messages/requests.py (25)
CircleClockGetRequest(1009-1028)CircleClockSetRequest(731-784)CircleEnergyLogsRequest(1130-1164)CirclePowerUsageRequest(657-676)CircleRelayInitStateRequest(1458-1491)CircleRelaySwitchRequest(787-817)EnergyCalibrationRequest(924-943)NodeInfoRequest(892-921)send(365-374)send(387-396)send(438-440)send(476-485)send(512-521)send(542-553)send(566-575)send(588-597)send(621-630)send(667-676)send(719-728)send(775-784)send(808-817)send(848-857)send(880-889)send(912-921)response(125-129)plugwise_usb/nodes/helpers/__init__.py (3)
EnergyCalibration(14-20)raise_not_loaded(26-33)decorated(29-32)plugwise_usb/nodes/helpers/counter.py (17)
EnergyCounters(52-178)energy(234-265)energy_statistics(104-106)consumption_interval(109-111)production_interval(114-116)timestamp(170-178)calibration(129-131)calibration(134-138)calibration(219-221)calibration(224-226)add_pulse_stats(92-101)log_rollover(124-126)log_addresses_missing(119-121)update(140-167)update(277-317)add_empty_log(70-72)add_pulse_log(74-86)plugwise_usb/nodes/node.py (12)
PlugwiseBaseNode(55-774)mac(230-232)energy(158-162)power(267-271)relay(283-287)relay_config(291-297)name(213-217)relay_state(275-279)skip_update(662-672)load(365-367)get_state(580-600)features(185-187)plugwise_usb/nodes/helpers/subscription.py (1)
publish_feature_update_to_subscribers(51-64)
plugwise_usb/messages/responses.py (4)
plugwise_usb/api.py (1)
NodeType(57-68)plugwise_usb/exceptions.py (1)
MessageError(20-21)plugwise_usb/messages/__init__.py (7)
PlugwiseMessage(22-109)Priority(14-20)identifier(47-49)mac_decoded(59-63)calculate_checksum(75-77)seq_id(37-39)seq_id(42-44)plugwise_usb/messages/properties.py (39)
Bytes(59-76)deserialize(25-27)deserialize(47-52)deserialize(67-69)deserialize(87-89)deserialize(113-118)deserialize(149-152)deserialize(178-180)deserialize(196-200)deserialize(228-237)deserialize(266-269)deserialize(292-294)deserialize(316-323)deserialize(345-352)deserialize(370-373)deserialize(390-396)Int(99-125)String(79-96)value(72-76)value(92-96)value(121-125)value(155-159)value(183-187)value(203-207)value(247-251)value(272-276)value(297-301)value(326-330)value(355-359)value(376-380)value(399-403)UnixTimestamp(162-187)DateTime(210-251)LogAddr(383-403)Float(362-380)RealClockTime(304-330)RealClockDate(333-359)Time(254-276)value_set(240-244)
🪛 Ruff (0.11.9)
plugwise_usb/network/__init__.py
266-266: Blank line contains whitespace
Remove whitespace from blank line
(W293)
289-289: Blank line contains whitespace
Remove whitespace from blank line
(W293)
302-302: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/helpers/pulses.py
274-274: Blank line contains whitespace
Remove whitespace from blank line
(W293)
290-290: Trailing whitespace
Remove trailing whitespace
(W291)
316-316: Blank line contains whitespace
Remove whitespace from blank line
(W293)
318-318: Trailing whitespace
Remove trailing whitespace
(W291)
344-344: First line of docstring should be in imperative mood: "Helper function for _update_rollover()."
(D401)
🔇 Additional comments (2)
plugwise_usb/nodes/sense.py (1)
1-143: Well-executed async refactoring!The conversion to async/await patterns is comprehensive and follows best practices. The integration with the new
StickControllerarchitecture and the event subscription system is clean and well-structured.plugwise_usb/messages/requests.py (1)
61-344: Well-designed async request framework!The new
PlugwiseRequestbase class provides a robust foundation for asynchronous request/response handling with:
- Proper Future management for awaitable responses
- Comprehensive timeout handling
- Clean subscription/unsubscription patterns
- Consistent error propagation
- Retry logic support
This is a significant improvement over the previous synchronous design.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (6)
plugwise_usb/nodes/helpers/pulses.py (1)
784-903: 🛠️ Refactor suggestionRefactor complex method for better maintainability.
The
_logs_missingmethod is over 100 lines with deeply nested logic. Consider breaking it down into smaller methods as previously suggested.plugwise_usb/nodes/sense.py (1)
60-67:⚠️ Potential issueFix inconsistent state when initialization fails.
The node is marked as loaded before checking if initialization succeeds. If initialization fails, the node remains in an inconsistent state.
plugwise_usb/network/registry.py (2)
124-139: Missing cache_restored flag updateThe method checks
self._cache_restoredbut never sets it toTrueafter loading, which could result in the cache being loaded multiple times.Apply this diff to fix the cache loading logic:
async def load_registry_from_cache(self) -> None: """Load network registry from cache.""" if self._network_cache is None: _LOGGER.error( "Unable to restore network registry because cache is not initialized" ) return if self._cache_restored: return for address, registration in self._network_cache.registrations.items(): mac, node_type = registration if self._registry.get(address) is None: self._registry[address] = (mac, node_type) + self._cache_restored = True
264-264: Fix string formatting errorThe error message uses incorrect string formatting syntax.
Apply this diff to fix the string formatting:
request = NodeAddRequest(self._send_to_controller, bytes(mac, UTF8), True) try: await request.send() except StickError as exc: - raise NodeError(f"{exc}") from exc + raise NodeError(f"{exc}") from excplugwise_usb/network/__init__.py (2)
263-263: Remove trailing whitespaceLine 263 contains whitespace on a blank line.
Apply this diff to fix the whitespace:
if result: return True - + return False
289-289: Remove trailing whitespaceLine 289 contains whitespace on a blank line.
Apply this diff to fix the whitespace:
return True - + return False
🧹 Nitpick comments (8)
plugwise_usb/nodes/switch.py (1)
85-85: Fix inconsistent region comment syntax.The region comments have inconsistent formatting - space after
#in start but not in end.- # region Properties + #region PropertiesAlso applies to: 93-93
plugwise_usb/nodes/helpers/pulses.py (1)
1-1033: Consider breaking down this large module.At over 1000 lines, this module handles many responsibilities. Consider splitting into separate modules for:
- Pulse log records and basic operations
- Missing log calculation logic
- Timestamp and interval management
This would improve maintainability and testability.
plugwise_usb/nodes/sense.py (1)
120-120: Remove trailing whitespace.report_received = True - + return report_received🧰 Tools
🪛 Ruff (0.11.9)
120-120: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/network/registry.py (1)
205-205: Consider reducing the 10-second sleep durationThe 10-second sleep between each registration during full scan could make the process take over 10 minutes to complete for all 64 addresses, which may significantly delay network initialization.
Consider reducing this to a more reasonable duration (e.g., 1-2 seconds) or making it configurable:
if not quick: - await sleep(10) + await sleep(1) # Reduced from 10 secondsplugwise_usb/network/__init__.py (1)
147-157: Clarify return value semanticsThe method returns
Falsewhenaccept_join_requestis disabled but raises an exception on actual registration errors. This inconsistency could confuse callers about when to expect exceptions vs boolean returns.Consider either:
- Always raising exceptions for all failure cases (recommended), or
- Documenting the return value semantics clearly in the docstring
Option 1 (recommended):
async def register_node(self, mac: str) -> bool: """Register node to Plugwise network.""" if not self.accept_join_request: - return False + raise NodeError("Node registration is currently disabled") try: await self._register.register_node(mac) except NodeError as exc: raise NodeError(f"{exc}") from exc return Trueplugwise_usb/nodes/node.py (1)
419-420: Avoid unnecessary int conversion for time comparisonThe
int()conversion on the time difference is unnecessary and could cause precision loss. The comparison can work directly with the float value.and timestamp is not None - and int((timestamp - self._last_seen).total_seconds()) > 5 + and (timestamp - self._last_seen).total_seconds() > 5plugwise_usb/nodes/scan.py (1)
52-52: Remove trailing whitespace.-SENSITIVITY_MEDIUM_VALUE = 30 # 0x1E +SENSITIVITY_MEDIUM_VALUE = 30 # 0x1E🧰 Tools
🪛 Ruff (0.11.9)
52-52: Trailing whitespace
Remove trailing whitespace
(W291)
plugwise_usb/nodes/circle.py (1)
817-822: Consider adjusting the log level for loaded energy records.While the log message has been updated to indicate failure (addressing the previous confusion), using WARNING level for a cache load failure during initialization might be too severe. Consider using DEBUG or INFO level instead, as this appears to be a non-critical initialization step that can proceed without cached data.
- if await self._energy_log_records_load_from_cache(): - _LOGGER.warning( - "Node %s failed to load energy_log_records from cache", - self._mac_in_str, - ) + if not await self._energy_log_records_load_from_cache(): + _LOGGER.debug( + "Node %s could not load energy_log_records from cache", + self._mac_in_str, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
plugwise_usb/messages/requests.py(2 hunks)plugwise_usb/network/__init__.py(1 hunks)plugwise_usb/network/registry.py(1 hunks)plugwise_usb/nodes/circle.py(1 hunks)plugwise_usb/nodes/helpers/pulses.py(1 hunks)plugwise_usb/nodes/node.py(1 hunks)plugwise_usb/nodes/scan.py(1 hunks)plugwise_usb/nodes/sed.py(1 hunks)plugwise_usb/nodes/sense.py(1 hunks)plugwise_usb/nodes/switch.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
plugwise_usb/nodes/helpers/pulses.py (6)
plugwise_usb/exceptions.py (1)
EnergyError(12-13)plugwise_usb/messages/responses.py (1)
offset(494-496)plugwise_usb/nodes/helpers/counter.py (8)
timestamp(170-178)is_consumption(229-231)collected_logs(66-68)log_rollover(124-126)last_update(273-275)direction(209-211)add_empty_log(70-72)log_addresses_missing(119-121)plugwise_usb/nodes/node.py (1)
mac(230-232)plugwise_usb/messages/__init__.py (1)
mac(52-56)plugwise_usb/api.py (1)
mac(270-271)
plugwise_usb/nodes/switch.py (8)
plugwise_usb/api.py (10)
NodeEvent(30-36)NodeFeature(39-54)mac(270-271)load(250-251)unload(253-254)switch(380-384)name(242-243)get_state(396-400)features(230-231)available_state(262-263)plugwise_usb/exceptions.py (2)
MessageError(20-21)NodeError(24-25)plugwise_usb/messages/responses.py (4)
NodeSwitchGroupResponse(836-863)PlugwiseResponse(98-215)firmware(628-630)switch_state(857-859)plugwise_usb/nodes/sed.py (6)
load(107-122)_load_from_cache(166-180)_load_defaults(156-164)initialize(143-154)unload(124-140)get_state(752-768)plugwise_usb/nodes/helpers/__init__.py (1)
raise_not_loaded(26-33)plugwise_usb/nodes/node.py (12)
mac(230-232)load(365-367)_load_from_cache(388-399)_setup_protocol(319-348)initialize(401-409)unload(602-608)switch(301-305)_available_update_state(411-440)name(213-217)get_state(580-600)features(185-187)available_state(99-104)plugwise_usb/nodes/scan.py (7)
load(87-111)_load_from_cache(147-161)_load_defaults(134-145)initialize(114-125)_switch_group(350-364)unload(127-131)get_state(555-578)plugwise_usb/nodes/helpers/subscription.py (1)
publish_feature_update_to_subscribers(51-64)
plugwise_usb/nodes/circle.py (10)
plugwise_usb/api.py (20)
EnergyStatistics(210-222)NodeEvent(30-36)NodeFeature(39-54)NodeInfo(125-138)PowerStatistics(152-157)RelayConfig(161-171)RelayState(175-179)mac(270-271)energy(331-335)power(359-363)relay(366-370)relay_config(455-465)relay_state(373-377)set_relay(405-421)load(250-251)is_online(283-284)unload(253-254)set_relay_init(625-641)get_state(396-400)features(230-231)plugwise_usb/connection/__init__.py (1)
StickController(31-258)plugwise_usb/exceptions.py (2)
FeatureError(16-17)NodeError(24-25)plugwise_usb/messages/requests.py (25)
CircleClockGetRequest(1009-1028)CircleClockSetRequest(731-784)CircleEnergyLogsRequest(1130-1164)CirclePowerUsageRequest(657-676)CircleRelayInitStateRequest(1458-1491)CircleRelaySwitchRequest(787-817)EnergyCalibrationRequest(924-943)NodeInfoRequest(892-921)send(365-374)send(387-396)send(438-440)send(476-485)send(512-521)send(542-553)send(566-575)send(588-597)send(621-630)send(667-676)send(719-728)send(775-784)send(808-817)send(848-857)send(880-889)send(912-921)response(125-129)plugwise_usb/messages/responses.py (12)
NodeInfoResponse(582-654)NodeResponseType(56-72)firmware(628-630)relay_state(643-645)gain_a(674-676)gain_b(679-681)off_noise(689-691)off_tot(684-686)offset(494-496)log_address(770-772)ack_id(133-135)hardware(623-625)plugwise_usb/nodes/helpers/__init__.py (3)
EnergyCalibration(14-20)raise_not_loaded(26-33)decorated(29-32)plugwise_usb/nodes/helpers/counter.py (12)
EnergyCounters(52-178)energy(234-265)timestamp(170-178)calibration(129-131)calibration(134-138)calibration(219-221)calibration(224-226)log_rollover(124-126)log_addresses_missing(119-121)update(140-167)update(277-317)add_empty_log(70-72)plugwise_usb/nodes/helpers/pulses.py (6)
PulseLogRecord(41-46)calc_log_address(20-37)log_rollover(151-153)log_addresses_missing(757-759)add_empty_log(380-411)logs(109-123)plugwise_usb/nodes/node.py (16)
PlugwiseBaseNode(55-781)mac(230-232)energy(158-162)power(267-271)relay(283-287)relay_config(291-297)relay_state(275-279)set_relay(746-752)skip_update(669-679)load(365-367)initialize(401-409)is_online(542-547)unload(602-608)set_relay_init(755-761)get_state(580-600)features(185-187)plugwise_usb/nodes/helpers/subscription.py (1)
publish_feature_update_to_subscribers(51-64)
plugwise_usb/network/registry.py (5)
plugwise_usb/api.py (5)
NodeType(57-68)mac(270-271)network_address(274-275)save_cache(322-325)clear_cache(319-320)plugwise_usb/exceptions.py (3)
CacheError(8-9)NodeError(24-25)StickError(32-33)plugwise_usb/helpers/util.py (1)
validate_mac(13-21)plugwise_usb/messages/requests.py (21)
CirclePlusScanRequest(820-857)NodeAddRequest(415-451)NodeRemoveRequest(860-889)PlugwiseRequest(61-343)response(125-129)send(365-374)send(387-396)send(438-440)send(476-485)send(512-521)send(542-553)send(566-575)send(588-597)send(621-630)send(667-676)send(719-728)send(775-784)send(808-817)send(848-857)send(880-889)send(912-921)plugwise_usb/network/cache.py (6)
NetworkRegistrationCache(15-83)restore_cache(45-73)registrations(24-26)update_registration(75-83)save_cache(28-38)clear_cache(40-43)
🪛 Ruff (0.11.9)
plugwise_usb/nodes/sense.py
120-120: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/scan.py
52-52: Trailing whitespace
Remove trailing whitespace
(W291)
🔇 Additional comments (13)
plugwise_usb/nodes/switch.py (2)
87-92: Verify handling of uninitialized switch state.The property returns
Falsewhen_switch_stateisNone. Please verify this is the intended behavior rather than raising an error or returning a different value for uninitialized state.
45-160: Well-structured async implementation!The switch node implementation properly:
- Manages subscriptions lifecycle
- Handles cache loading and saving
- Uses async/await patterns consistently
- Publishes state updates to subscribers
- Includes proper error handling with MessageError
plugwise_usb/nodes/sense.py (1)
93-122: Good fix for the return value logic!The use of
report_receivedflag properly tracks whether any valid data was processed, addressing the previous concern about incomplete return logic.🧰 Tools
🪛 Ruff (0.11.9)
120-120: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/scan.py (3)
177-177: Correct use oftotal_seconds()for time difference calculation.The implementation properly uses
total_seconds()to get the full duration including days, which addresses the previous review feedback.
386-386: Correct use oftotal_seconds()for reset timer calculation.The implementation properly uses
total_seconds()for accurate time difference calculation, which addresses the previous review feedback.
472-478: Well-structured sensitivity mapping implementation.The sensitivity values are properly defined as module-level constants and the mapping uses a clean dictionary approach with a sensible default, which addresses the previous review feedback about magic numbers.
plugwise_usb/messages/requests.py (3)
203-208: Correct implementation ofstop_response_timeoutmethod.The method correctly sets
_waiting_for_response = Falsewhen stopping the timeout, which properly reflects that the system is no longer waiting for a response. This addresses the previous review feedback.
1234-1234: Appropriate use of debug logging level.Using debug level for diagnostic information is correct, which addresses the previous review feedback about not using warning level for non-warning conditions.
1239-1241: Correct error message in exception.The error message correctly states "expected NodeResponse" which matches the isinstance check and method signature. This addresses the previous review feedback about the mismatched error message.
plugwise_usb/nodes/circle.py (4)
533-540: Efficient cache retrieval implementation.The method correctly uses the walrus operator to retrieve and check the cache value in a single expression, avoiding duplicate
_get_cachecalls. This addresses the previous review feedback about redundant code.
735-744: Clean async/await implementation without duplication.The code correctly awaits
set_clock_request.send()once within the if statement condition, avoiding duplicate await calls. This addresses the previous review feedback.
823-827: Correct log message for successful relay state loading.The log message correctly indicates success when the relay state is loaded from cache, which addresses the previous review feedback about the misleading message.
830-835: Correct log message for successful relay init state loading.The log message correctly indicates success when the relay init state is loaded from cache, which addresses the previous review feedback about the misleading message.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
plugwise_usb/nodes/helpers/pulses.py (1)
983-1037:⚠️ Potential issueFix incorrect hardcoded interval in consumption-only case.
Line 1008 uses a hardcoded
timedelta(hours=1)instead of the calculatedcalc_interval_consvariable.Apply this fix:
while expected_timestamp < target: address, slot = calc_log_address(address, slot, 1) - expected_timestamp += timedelta(hours=1) + expected_timestamp += calc_interval_cons if address not in addresses: addresses.append(address)
♻️ Duplicate comments (3)
plugwise_usb/nodes/helpers/pulses.py (3)
784-903: Refactor complex method for better maintainability.The
_logs_missingmethod is over 100 lines with deeply nested logic. Consider breaking it down into smaller methods as suggested in the previous review.
821-828: Fix missing return value - critical bug.The method should return
list[int] | Nonebut line 827 has a barereturnstatement. Should return an empty list as suggested in the previous review.
915-922: Fix critical logic error in while condition.The OR condition on line 919 will cause a KeyError as noted in the previous review. Change to AND to prevent accessing non-existent logs.
🧹 Nitpick comments (3)
plugwise_usb/nodes/sed.py (1)
240-242: Fix whitespace on blank line.Remove the whitespace from the blank line.
if self._battery_config.awake_duration == seconds: return False - self._new_battery_config = replace(🧰 Tools
🪛 Ruff (0.11.9)
242-242: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/helpers/pulses.py (2)
49-98: Consider grouping related instance variables for better organization.The initialization contains many instance variables that could be better organized by grouping related fields together using dataclasses or named tuples. This would improve readability and maintainability.
For example, you could create helper dataclasses:
@dataclass class LogReference: address: int | None = None slot: int | None = None timestamp: datetime | None = None @dataclass class LogBoundaries: first: LogReference = field(default_factory=LogReference) last: LogReference = field(default_factory=LogReference) first_empty: LogReference = field(default_factory=LogReference) last_empty: LogReference = field(default_factory=LogReference)This would reduce the number of individual instance variables and make the code more maintainable.
99-107: Simplify the log counting logic.The method can be simplified using a generator expression:
@property def collected_logs(self) -> int: """Total collected logs.""" - counter = 0 if self._logs is None: - return counter - for address in self._logs: - counter += len(self._logs[address]) - return counter + return 0 + return sum(len(slots) for slots in self._logs.values())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise_usb/nodes/helpers/pulses.py(1 hunks)plugwise_usb/nodes/sed.py(1 hunks)plugwise_usb/nodes/switch.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
plugwise_usb/nodes/sed.py
242-242: Blank line contains whitespace
Remove whitespace from blank line
(W293)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check commit
- GitHub Check: Check commit
🔇 Additional comments (37)
plugwise_usb/nodes/switch.py (10)
2-21: LGTM! Well-structured imports for async architecture.The imports are properly organized and include all necessary components for the async refactor:
- Future annotations for better type hinting
- Asyncio components for concurrent operations
- Proper typing imports
- Relevant API, connection, and message response imports
25-27: LGTM! Appropriate cache constants.The Final type annotation for cache constants follows best practices and provides clear naming for switch state and timestamp caching.
32-43: LGTM! Well-designed async constructor.The constructor properly:
- Accepts the required StickController and async callback
- Initializes subscription management (
_switch_subscription)- Sets up switch state tracking (
_switch_state)- Maintains clean separation of concerns
44-63: LGTM! Comprehensive async load implementation.The load method properly handles:
- Early return for already loaded nodes
- Cache-based or default initialization
- Protocol setup with correct features
- Async initialization with proper callback handling
- Appropriate error logging
64-77: LGTM! Proper async initialization with subscription management.The initialize method correctly:
- Uses the
@raise_not_loadeddecorator for safety- Sets up message subscription with proper cleanup reference
- Calls parent initialization
- Follows good async patterns
78-83: LGTM! Clean subscription cleanup.The unload method properly cleans up the switch subscription before calling the parent unload, preventing potential memory leaks.
86-91: LGTM! Safe property implementation.The switch property correctly:
- Uses
@raise_not_loadeddecorator for safety- Returns boolean conversion of internal state
- Handles None state appropriately (converts to False)
94-105: LGTM! Robust async message handling.The
_switch_groupmethod demonstrates excellent async patterns:
- Proper type validation with clear error messages
- Concurrent processing using
gather()for availability and state updates- Appropriate error handling with MessageError
106-134: LGTM! Simplified and corrected state update logic.The state update logic has been significantly improved from past versions:
- Clear debug logging for state transitions
- Simplified state comparison logic (addresses past review feedback)
- Proper cache management
- Concurrent publishing and cache saving using
gather()This addresses the complexity issues mentioned in past reviews and removes the bug where
self._switchwas incorrectly compared instead ofself._switch_state.
135-158: LGTM! Comprehensive feature state management.The
get_statemethod provides robust implementation:
- Proper feature validation against supported features
- Specific handling for SWITCH feature
- Delegation to parent class for other features
- Automatic inclusion of AVAILABLE state
- Good error handling with descriptive messages
plugwise_usb/nodes/sed.py (10)
1-67: LGTM! Well-organized imports and constants.The imports are properly structured for the async refactor, and the constants are well-defined with clear naming conventions and appropriate use of
Finaltype hints.
70-106: LGTM! Well-structured async initialization.The class properly inherits from
PlugwiseBaseNodeand correctly initializes all async primitives including locks, tasks, and futures. The battery configuration setup is well-organized.
107-155: LGTM! Comprehensive async lifecycle management.The load/unload/initialize methods are well-implemented with proper async patterns. The unload method does an excellent job of cleaning up all resources including futures, tasks, subscriptions, and pending tasks in the queue.
225-372: Great job fixing the configuration logic!All the configuration setter methods now correctly handle the case when no change is needed, returning
Falsewithout updating_new_battery_config. This addresses all the past review comments about inconsistent behavior.🧰 Tools
🪛 Ruff (0.11.9)
242-242: Blank line contains whitespace
Remove whitespace from blank line
(W293)
448-486: Excellent fix for the change detection logic!The
change_requiredvariable is now correctly initialized toFalseand only set toTruewhen actual changes are detected. This ensures the configuration is only applied when necessary.
637-659: Perfect implementation of async lock management!Both
_send_tasksandschedule_task_when_awakenow correctly use async context managers (async with) for lock management, ensuring the lock is always properly released even if an exception occurs.
660-713: Great fix for the duplicate request issue!The method now correctly makes a single call to
request.send()and properly handles the response, eliminating the critical bug of sending duplicate requests.
496-598: LGTM! Robust awake response handling.The implementation includes excellent debouncing logic to prevent duplicate processing, comprehensive maintenance interval detection with proper validation margins, and appropriate state/cache updates for different awake types.
599-636: LGTM! Well-implemented awake timer with proper timeout handling.The awake timer correctly monitors for maintenance messages with a reasonable timeout margin (2.1x the interval), properly marks the node as unavailable when messages are missed, and handles exceptions appropriately.
749-767: LGTM! Clean state retrieval implementation.The
get_statemethod properly handles the supported features (INFO and BATTERY) and correctly delegates other features to the parent class, with appropriate error handling for unsupported features.plugwise_usb/nodes/helpers/pulses.py (17)
1-18: LGTM!The imports and constants are well-organized and appropriate for the module's functionality.
20-38: LGTM!The
calc_log_addressfunction correctly handles log address calculations with proper rollover logic for both negative and positive offsets.
40-48: LGTM!The
PulseLogRecorddataclass is well-structured for storing pulse log data.
109-124: LGTM!The
logsproperty correctly returns logs in reverse chronological order while filtering out entries older than 24 hours.
125-164: LGTM!All properties are well-implemented with appropriate None handling and clear purposes.
165-214: LGTM!The
collected_pulsesmethod correctly calculates pulse totals with proper handling of consumption/production modes and returns absolute values for energy statistics.
215-269: LGTM!The method correctly aggregates pulses from logs with appropriate filtering and validation.
314-379: LGTM!The rollover detection logic correctly identifies when pulse counters have rolled over based on timestamp comparisons.
380-412: LGTM!The method correctly manages empty log references and triggers necessary recalculations.
413-449: LGTM!The
add_logmethod properly handles log addition with all necessary updates and validations.
450-455: LGTM!Simple and effective method for triggering missing log recalculation.
456-495: LGTM!The method correctly handles log record addition with duplicate detection and old log filtering.
496-538: LGTM!The method correctly implements the complex logic for determining log direction based on timestamp pairing.
539-607: LGTM!The interval calculation logic correctly determines log intervals and updates expected timestamps.
608-616: LGTM!Simple and effective log existence check.
617-755: LGTM!All reference update methods are well-implemented with consistent patterns and proper state management.
926-982: LGTM!The method correctly calculates missing addresses with proper handling of both consumption-only and production scenarios.
|
you are on (fire) probeer je te volgen maar de branches vliegen me om de oren |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugwise_usb/nodes/helpers/pulses.py (1)
52-97: Consider refactoring to reduce class complexity.The
PulseCollectionclass initializes 38 instance variables, which suggests high complexity and potential maintainability issues. Consider grouping related variables into dedicated dataclasses or sub-objects, such as:
LogReferencesfor first/last log trackingLogIntervalsfor interval-related dataPulseCountersfor pulse trackingThis would improve code organization and make the class easier to understand and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/helpers/pulses.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
plugwise_usb/nodes/helpers/pulses.py (1)
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#236
File: plugwise_usb/nodes/helpers/pulses.py:270-313
Timestamp: 2025-05-24T09:02:22.887Z
Learning: In the Plugwise USB library, production pulse values (self._pulses_production, pulses_produced) are stored as negative numbers, while consumption pulse values (self._pulses_consumption, pulses_consumed) are stored as positive numbers. This means pulse counter reset detection uses different comparison operators: `>` for positive consumption values and `<` for negative production values.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check commit
- GitHub Check: Check commit
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
plugwise_usb/__init__.py (1)
57-60:⚠️ Potential issueRemove or make debug mode conditional.
Setting debug mode on the event loop can impact performance in production. This should be controlled by a configuration parameter or environment variable.
def __init__( self, port: str | None = None, cache_enabled: bool = True ) -> None: """Initialize Stick.""" self._loop = get_running_loop() - self._loop.set_debug(True) + # Only enable debug mode in development + # self._loop.set_debug(True) self._controller = StickController()
🧹 Nitpick comments (4)
plugwise_usb/messages/properties.py (4)
40-42: Consider using a more flexible type annotation.The explicit union type listing all property classes may require maintenance when new property types are added. Consider using a protocol or base type for better extensibility.
- self.contents: list[ - String | Int | SInt | UnixTimestamp | Year2k | IntDec | Float | LogAddr - ] = [] + self.contents: list[BaseType] = []
152-153: Address the TODO comment and verify negative number handling.The TODO comment indicates uncertainty about the
negativemethod initialization. The logic appears correct for two's complement conversion, but this should be verified and the TODO resolved.I can help verify this logic against test cases or remove the TODO if the implementation is confirmed correct. The
negativemethod appears to correctly handle two's complement conversion, which is the inverse of theint_to_uintutility function.
394-394: Remove unnecessary int() call.Static analysis correctly identified an unnecessary
int()call.- self._value = int(0) + self._value = 0🧰 Tools
🪛 Ruff (0.11.9)
394-394: Unnecessary
intcall (rewrite as a literal)Replace with integer literal
(UP018)
314-314: Consider documenting the time component order.The contents are ordered as
[second, minute, hour]which might be counterintuitive. Consider adding a comment explaining this ordering if it's intentional for the protocol.- self.contents += [self.second, self.minute, self.hour] + # Protocol order: second, minute, hour + self.contents += [self.second, self.minute, self.hour]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/__init__.py(1 hunks)plugwise_usb/messages/properties.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugwise_usb/messages/properties.py (3)
plugwise_usb/exceptions.py (1)
MessageError(20-21)plugwise_usb/helpers/util.py (1)
int_to_uint(53-58)plugwise_usb/messages/responses.py (1)
deserialize(137-201)
🪛 Ruff (0.11.9)
plugwise_usb/messages/properties.py
394-394: Unnecessary int call (rewrite as a literal)
Replace with integer literal
(UP018)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Process test coverage
- GitHub Check: Run mypy
- GitHub Check: Run pytest using Python 3.13
🔇 Additional comments (1)
plugwise_usb/messages/properties.py (1)
1-12: LGTM! Well-structured imports and error handling.The imports are appropriate and the
DESERIALIZE_ERRORconstant provides consistent error handling across all property types.
| def serialize(self) -> bytes: | ||
| """Return current value into an iterable list of bytes.""" | ||
| return bytes(self._raw_value, UTF8) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making serialize method abstract or more generic.
The default UTF8 encoding in the base serialize method may not be appropriate for all subclasses (e.g., Bytes class handles binary data). Consider making this method abstract or providing a more generic implementation.
def serialize(self) -> bytes:
"""Return current value into an iterable list of bytes."""
- return bytes(self._raw_value, UTF8)
+ raise NotImplementedError("Subclasses must implement serialize method")🤖 Prompt for AI Agents
In plugwise_usb/messages/properties.py around lines 22 to 24, the serialize
method currently encodes _raw_value using UTF8, which may not suit all
subclasses like Bytes that handle binary data. To fix this, make the serialize
method abstract in the base class to force subclasses to implement their own
serialization logic, or provide a more generic implementation that can be
overridden as needed. This ensures each subclass handles serialization
appropriately without assuming UTF8 encoding.
| class Float(BaseType): | ||
| """Float value property.""" | ||
|
|
||
| def __init__(self, value: float, length: int = 4) -> None: | ||
| """Initialize float value property.""" | ||
| super().__init__(value, length) | ||
| self._value: float | None = None | ||
|
|
||
| def deserialize(self, val: bytes) -> None: | ||
| """Convert data into float value.""" | ||
| hex_val = binascii.unhexlify(val) | ||
| self._value = float(struct.unpack("!f", hex_val)[0]) | ||
|
|
||
| @property | ||
| def value(self) -> float: | ||
| """Return converted float value.""" | ||
| if self._value is None: | ||
| raise DESERIALIZE_ERROR | ||
| return self._value | ||
|
|
There was a problem hiding this comment.
Float class missing serialize method.
The Float class doesn't implement a serialize method, which will cause it to use the inherited UTF8 encoding from BaseType, leading to incorrect serialization.
class Float(BaseType):
"""Float value property."""
def __init__(self, value: float, length: int = 4) -> None:
"""Initialize float value property."""
super().__init__(value, length)
self._value: float | None = None
+ def serialize(self) -> bytes:
+ """Return current float value as hex-encoded bytes."""
+ packed = struct.pack("!f", self._raw_value)
+ return binascii.hexlify(packed).upper()
def deserialize(self, val: bytes) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Float(BaseType): | |
| """Float value property.""" | |
| def __init__(self, value: float, length: int = 4) -> None: | |
| """Initialize float value property.""" | |
| super().__init__(value, length) | |
| self._value: float | None = None | |
| def deserialize(self, val: bytes) -> None: | |
| """Convert data into float value.""" | |
| hex_val = binascii.unhexlify(val) | |
| self._value = float(struct.unpack("!f", hex_val)[0]) | |
| @property | |
| def value(self) -> float: | |
| """Return converted float value.""" | |
| if self._value is None: | |
| raise DESERIALIZE_ERROR | |
| return self._value | |
| class Float(BaseType): | |
| """Float value property.""" | |
| def __init__(self, value: float, length: int = 4) -> None: | |
| """Initialize float value property.""" | |
| super().__init__(value, length) | |
| self._value: float | None = None | |
| def serialize(self) -> bytes: | |
| """Return current float value as hex-encoded bytes.""" | |
| packed = struct.pack("!f", self._raw_value) | |
| return binascii.hexlify(packed).upper() | |
| def deserialize(self, val: bytes) -> None: | |
| """Convert data into float value.""" | |
| hex_val = binascii.unhexlify(val) | |
| self._value = float(struct.unpack("!f", hex_val)[0]) | |
| @property | |
| def value(self) -> float: | |
| """Return converted float value.""" | |
| if self._value is None: | |
| raise DESERIALIZE_ERROR | |
| return self._value |
🤖 Prompt for AI Agents
In plugwise_usb/messages/properties.py around lines 363 to 382, the Float class
lacks a serialize method, causing it to incorrectly use the BaseType's UTF8
serialization. Implement a serialize method that converts the float value to
bytes using struct.pack with the appropriate format, then hexlify the result to
produce the correct serialized byte representation.
| Int.deserialize(self, val) | ||
| self._value = (self.value - LOGADDR_OFFSET) // 32 |
There was a problem hiding this comment.
Fix logic error in LogAddr deserialization.
The code calls Int.deserialize(self, val) but then immediately accesses self.value, which would be the result of the Int deserialization, not the current instance's value.
Int.deserialize(self, val)
- self._value = (self.value - LOGADDR_OFFSET) // 32
+ if self._value is not None:
+ self._value = (self._value - LOGADDR_OFFSET) // 32📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Int.deserialize(self, val) | |
| self._value = (self.value - LOGADDR_OFFSET) // 32 | |
| Int.deserialize(self, val) | |
| if self._value is not None: | |
| self._value = (self._value - LOGADDR_OFFSET) // 32 |
🤖 Prompt for AI Agents
In plugwise_usb/messages/properties.py at lines 396 to 397, the deserialization
logic incorrectly uses Int.deserialize(self, val) but then accesses self.value,
which refers to the Int deserialization result rather than the current
instance's value. To fix this, assign the result of Int.deserialize(val) to a
variable and use that variable for the calculation instead of self.value,
ensuring the correct value is used for the _value assignment.
| class Bytes(BaseType): | ||
| """Bytes based property.""" | ||
|
|
||
| def __init__(self, value: bytes | None, length: int) -> None: | ||
| """Initialize bytes based property.""" | ||
| super().__init__(value, length) | ||
| self._value: bytes | None = None | ||
|
|
||
| def deserialize(self, val: bytes) -> None: | ||
| """Set current value.""" | ||
| self._value = val | ||
|
|
||
| @property | ||
| def value(self) -> bytes: | ||
| """Return bytes value.""" | ||
| if self._value is None: | ||
| raise DESERIALIZE_ERROR | ||
| return self._value | ||
|
|
There was a problem hiding this comment.
Critical: Bytes class needs to override serialize method.
The Bytes class inherits the serialize method from BaseType, which attempts UTF8 encoding. This will fail for binary data.
class Bytes(BaseType):
"""Bytes based property."""
def __init__(self, value: bytes | None, length: int) -> None:
"""Initialize bytes based property."""
super().__init__(value, length)
self._value: bytes | None = None
+ def serialize(self) -> bytes:
+ """Return current bytes value."""
+ if self._raw_value is None:
+ raise MessageError("Unable to serialize None bytes value")
+ return self._raw_value
def deserialize(self, val: bytes) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class Bytes(BaseType): | |
| """Bytes based property.""" | |
| def __init__(self, value: bytes | None, length: int) -> None: | |
| """Initialize bytes based property.""" | |
| super().__init__(value, length) | |
| self._value: bytes | None = None | |
| def deserialize(self, val: bytes) -> None: | |
| """Set current value.""" | |
| self._value = val | |
| @property | |
| def value(self) -> bytes: | |
| """Return bytes value.""" | |
| if self._value is None: | |
| raise DESERIALIZE_ERROR | |
| return self._value | |
| class Bytes(BaseType): | |
| """Bytes based property.""" | |
| def __init__(self, value: bytes | None, length: int) -> None: | |
| """Initialize bytes based property.""" | |
| super().__init__(value, length) | |
| self._value: bytes | None = None | |
| def serialize(self) -> bytes: | |
| """Return current bytes value.""" | |
| if self._raw_value is None: | |
| raise MessageError("Unable to serialize None bytes value") | |
| return self._raw_value | |
| def deserialize(self, val: bytes) -> None: | |
| """Set current value.""" | |
| self._value = val | |
| @property | |
| def value(self) -> bytes: | |
| """Return bytes value.""" | |
| if self._value is None: | |
| raise DESERIALIZE_ERROR | |
| return self._value |
🤖 Prompt for AI Agents
In plugwise_usb/messages/properties.py around lines 60 to 78, the Bytes class
currently inherits the serialize method from BaseType, which uses UTF8 encoding
unsuitable for binary data. To fix this, override the serialize method in the
Bytes class to directly return the bytes value without encoding, ensuring binary
data is handled correctly during serialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
plugwise_usb/__init__.py (3)
288-288: Remove whitespace from blank line.Static analysis detected whitespace on a blank line which should be removed for code cleanliness.
- +🧰 Tools
🪛 Ruff (0.11.9)
288-288: Blank line contains whitespace
Remove whitespace from blank line
(W293)
54-65: Consider making cache folder validation more robust.The initialization looks good overall, but the cache folder handling could be more defensive.
Consider validating the cache folder path if provided:
def __init__( self, port: str | None = None, cache_enabled: bool = True ) -> None: """Initialize Stick.""" self._loop = get_running_loop() self._loop.set_debug(True) self._controller = StickController() self._network: StickNetwork | None = None self._cache_enabled = cache_enabled self._port = port - self._cache_folder: str = "" + self._cache_folder: str = "" + # TODO: Add cache folder validation if needed
257-271: Consider adding timeout parameter to setup method.The
setupmethod orchestrates multiple async operations but lacks timeout control, which could lead to hanging operations in network issues.Consider adding an optional timeout parameter:
async def setup( - self, discover: bool = True, load: bool = True + self, discover: bool = True, load: bool = True, timeout: float | None = None ) -> None: """Fully connect, initialize USB-Stick and discover all connected nodes.""" + # Consider wrapping operations with asyncio.wait_for(timeout) if timeout is provided
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/__init__.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
plugwise_usb/__init__.py
288-288: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🔇 Additional comments (5)
plugwise_usb/__init__.py (5)
169-181: LGTM: Port setter logic has been simplified.The redundant logic from the previous implementation has been correctly addressed. The current implementation properly validates connection state and directly assigns the port value.
26-48: Well-designed validation decorators.The
raise_not_connectedandraise_not_initializeddecorators provide clean separation of concerns and consistent error handling across the async methods. The use ofTypeVarand proper type casting ensures type safety.
218-247: Excellent async event subscription design.The event subscription methods provide a clean async interface with proper type hints for callbacks and return unsubscribe functions. The error handling in
subscribe_to_node_eventsappropriately validates network state.
347-366: Robust node management with proper error handling.The
register_nodeandunregister_nodemethods demonstrate good async patterns with proper exception handling and error re-raising with context. The type-specific error conversions provide clear error semantics.
194-203:⚠️ Potential issueFix string concatenation formatting in error messages.
Multiple error messages in this method have inconsistent spacing in string concatenation that could result in malformed text.
Apply this diff to fix the formatting:
if not self._controller.is_connected: raise StickError( "Cannot accept joining node" - + " without an active USB-Stick connection." + + " without an active USB-Stick connection." ) if self._network is None or not self._network.is_running: raise StickError( "Cannot accept joining node" - + " without node discovery be activated. Call discover() first." + + " without node discovery being activated. Call discover() first." )Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (28)
plugwise_usb/connection/sender.py (1)
98-140: 🛠️ Refactor suggestionAvoid wrapping StickError in BaseException.
The code unnecessarily wraps
StickErrorexceptions inBaseException, which complicates error handling. This issue was already identified in previous reviews.Apply these changes to all four occurrences:
- request.assign_error( - BaseException( - StickError( - f"USB-Stick did not respond within {STICK_TIME_OUT} seconds after writing {request}" - ) - ) - ) + request.assign_error( + StickError( + f"USB-Stick did not respond within {STICK_TIME_OUT} seconds after writing {request}" + ) + )Apply the same pattern to lines 112-116, 129-133, and 136-140.
tests/stick_test_data.py (1)
1-5: Add module docstring and simplify import.🧰 Tools
🪛 Ruff (0.11.9)
1-1: Missing docstring in public module
(D100)
plugwise_usb/helpers/cache.py (4)
62-62: Remove trailing whitespace.🧰 Tools
🪛 Ruff (0.11.9)
62-62: Blank line contains whitespace
Remove whitespace from blank line
(W293)
79-79: Fix docstring formatting.
119-122: Fix error message in read_cache method.
143-152: Use continue instead of break for invalid lines.plugwise_usb/messages/__init__.py (1)
95-101: Fix incorrect comparison in__ge__method.The
__ge__method still incorrectly uses<instead of<=for sequence ID comparison, as previously identified.plugwise_usb/connection/queue.py (1)
79-79: Remove trailing whitespace.The blank line still contains whitespace as previously identified.
🧰 Tools
🪛 Ruff (0.11.9)
79-79: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/connection/manager.py (1)
39-44: Add null checks to prevent AttributeError.The
queue_depthproperty andcorrect_received_messagesmethod still access_senderand_receiverwithout verifying they are not None, as previously identified.🧰 Tools
🪛 Ruff (0.11.9)
40-40: Missing docstring in public method
(D102)
43-43: Missing docstring in public method
(D102)
plugwise_usb/nodes/circle_plus.py (1)
101-111: Fix incorrect clock drift check logic.The condition on lines 102-104 still uses the incorrect logic that will always evaluate to true, as previously identified.
plugwise_usb/connection/__init__.py (1)
131-133: Remove unnecessary None check.The
_managerattribute is always initialized in__init__and never set toNone, making this check redundant.- if self._manager is None: - raise StickError("Connect to stick before subscribing to events") return self._manager.subscribe_to_stick_events(plugwise_usb/messages/properties.py (4)
22-24: Consider making serialize method abstract or more generic.The default UTF8 encoding in the base
serializemethod may not be appropriate for all subclasses (e.g.,Bytesclass handles binary data). Consider making this method abstract or providing a more generic implementation.def serialize(self) -> bytes: """Return current value into an iterable list of bytes.""" - return bytes(self._raw_value, UTF8) + raise NotImplementedError("Subclasses must implement serialize method")
60-78: Critical: Bytes class needs to override serialize method.The
Bytesclass inherits theserializemethod fromBaseType, which attempts UTF8 encoding. This will fail for binary data.class Bytes(BaseType): """Bytes based property.""" def __init__(self, value: bytes | None, length: int) -> None: """Initialize bytes based property.""" super().__init__(value, length) self._value: bytes | None = None + def serialize(self) -> bytes: + """Return current bytes value.""" + if self._raw_value is None: + raise MessageError("Unable to serialize None bytes value") + return self._raw_value def deserialize(self, val: bytes) -> None:🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 76-76: plugwise_usb/messages/properties.py#L76
Added line #L76 was not covered by tests
363-382: Float class missing serialize method.The
Floatclass doesn't implement aserializemethod, which will cause it to use the inherited UTF8 encoding fromBaseType, leading to incorrect serialization.class Float(BaseType): """Float value property.""" def __init__(self, value: float, length: int = 4) -> None: """Initialize float value property.""" super().__init__(value, length) self._value: float | None = None + def serialize(self) -> bytes: + """Return current float value as hex-encoded bytes.""" + packed = struct.pack("!f", self._raw_value) + return binascii.hexlify(packed).upper() def deserialize(self, val: bytes) -> None:🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 380-380: plugwise_usb/messages/properties.py#L380
Added line #L380 was not covered by tests
396-397: Fix logic error in LogAddr deserialization.The code calls
Int.deserialize(self, val)but then immediately accessesself.value, which would be the result of the Int deserialization, not the current instance's value.Int.deserialize(self, val) - self._value = (self.value - LOGADDR_OFFSET) // 32 + if self._value is not None: + self._value = (self._value - LOGADDR_OFFSET) // 32plugwise_usb/nodes/helpers/pulses.py (3)
821-827: Fix missing return value in error condition.The method should return a value of type
list[int] | Nonebut line 827 has a barereturnstatement without a value.Based on the warning message context, you should return
Noneto indicate that missing log calculation cannot proceed:if last_address < first_address and (first_address - last_address < 1000): _LOGGER.warning( "The Circle %s does not overwrite old logged data, please reset the Circle's energy-logs via Source", self._mac, ) - return + return None
915-927: Add defensive programming check after loop.After the while loop that decrements through log addresses, the code accesses
self._logs[address][slot]without verifying the log exists at line 924, which could cause a KeyError if the loop exits with a non-existent log.Add an existence check before accessing the log:
while ( self._log_exists(address, slot) and self._logs[address][slot].timestamp == last_known_timestamp ): address, slot = calc_log_address(address, slot, -1) + if not self._log_exists(address, slot): + return timedelta(hours=1) # Default duration + if self._logs[address][slot].timestamp == last_known_timestamp: return timedelta(hours=1) return self._logs[address][slot].timestamp - last_known_timestamp
784-902: Consider refactoring complex method for better maintainability.The
_logs_missingmethod is over 100 lines with deeply nested logic, making it difficult to maintain and test. Consider breaking it down into smaller, focused methods:
- Extract validation logic (lines 786-812) into
_validate_log_references()- Extract missing-in-range logic (lines 838-856) into
_find_missing_in_range()- Extract calculated missing logic (lines 885-902) into
_calculate_missing_before_first()This would improve readability, testability, and maintainability.
plugwise_usb/nodes/sense.py (1)
51-70: Fix inconsistent state management during loading.The node is marked as loaded (
self._loaded = Trueon line 60) before checking ifinitialize()succeeds. If initialization fails, the node remains in an inconsistent state where it's marked as loaded but not properly initialized.Move the assignment to after successful initialization:
if self._cache_enabled: _LOGGER.debug("Loading Sense node %s from cache", self._node_info.mac) if await self._load_from_cache(): - self._loaded = True self._setup_protocol( SENSE_FIRMWARE_SUPPORT, (NodeFeature.INFO, NodeFeature.TEMPERATURE, NodeFeature.HUMIDITY), ) if await self.initialize(): + self._loaded = True await self._loaded_callback(NodeEvent.LOADED, self.mac) return Trueplugwise_usb/network/registry.py (1)
124-139: Add missing cache_restored flag update.The method checks
self._cache_restoredbut never sets it toTrueafter loading, which could result in the cache being loaded multiple times.async def load_registry_from_cache(self) -> None: """Load network registry from cache.""" if self._network_cache is None: _LOGGER.error( "Unable to restore network registry because cache is not initialized" ) return if self._cache_restored: return for address, registration in self._network_cache.registrations.items(): mac, node_type = registration if self._registry.get(address) is None: self._registry[address] = (mac, node_type) + self._cache_restored = Trueplugwise_usb/network/__init__.py (2)
147-156: Add test coverage for node registration method.The
register_nodemethod lacks test coverage according to static analysis. This is a critical network management function that should be thoroughly tested.Consider adding unit tests to cover:
- Successful node registration when
accept_join_requestis True- Registration rejection when
accept_join_requestis False- Error handling for
NodeErrorexceptions
162-171: Add test coverage for node unregistration method.The
unregister_nodemethod lacks test coverage according to static analysis. This critical functionality should be tested to ensure proper error handling and cleanup.Consider adding unit tests to cover:
- Successful node unregistration with proper cleanup
- Error handling for non-existent MAC addresses
- Proper node unloading and removal from registry
plugwise_usb/constants.py (1)
34-34: Remove trailing whitespace (duplicate issue).This trailing whitespace issue was already identified in a previous review but hasn't been addressed yet.
🧰 Tools
🪛 Ruff (0.11.9)
34-34: Trailing whitespace
Remove trailing whitespace
(W291)
plugwise_usb/api.py (2)
559-559: Fix documentation: parameter is minutes, not seconds.The ValueError documentation incorrectly refers to "seconds value" when the parameter is "minutes".
Apply this diff to fix the documentation:
- ValueError: When the seconds value is out of range. + ValueError: When the minutes value is out of range.
602-602: Fix documentation: parameter is minutes, not seconds.The ValueError documentation incorrectly refers to "seconds value" when the parameter is "minutes".
Apply this diff to fix the documentation:
- ValueError: When the seconds value is out of range. + ValueError: When the minutes value is out of range.plugwise_usb/messages/responses.py (1)
1013-1014: Deduplicate or cover the NodeRejoinResponse branchThe
identifier == NODE_REJOIN_IDcheck at lines 1013–1014 is never reached in practice because the sequence‐ID check forREJOIN_RESPONSE_SEQ_IDalready returnsNodeRejoinResponse. You should either remove this redundant branch or add tests to explicitly cover both ways of creating aNodeRejoinResponse.Apply this diff to remove the redundant check:
- if identifier == NODE_REJOIN_ID: - return NodeRejoinResponse()Or add unit tests to cover both cases if there's a legitimate scenario for the identifier-based check.
plugwise_usb/__init__.py (1)
57-60: Remove or make debug mode conditional.Setting debug mode on the event loop can impact performance in production. This should be controlled by a configuration parameter or environment variable.
plugwise_usb/nodes/circle.py (1)
816-822:⚠️ Potential issueFix misleading log message.
The log message incorrectly states "failed to load energy_log_records" when the condition indicates success (returns True).
if await self._energy_log_records_load_from_cache(): - _LOGGER.warning( - "Node %s failed to load energy_log_records from cache", + _LOGGER.debug( + "Node %s successfully loaded energy_log_records from cache", self._mac_in_str, )
🧹 Nitpick comments (17)
plugwise_usb/exceptions.py (1)
8-45: Improve docstring consistency and descriptiveness across exception classes.The exception docstrings are inconsistent in style and informativeness. Some merely repeat the class name while others provide meaningful descriptions. Additionally, there's inconsistent use of plural forms.
Consider making all docstrings more descriptive and consistent:
class CacheError(PlugwiseException): - """Cache error.""" + """Error accessing or managing the cache.""" class EnergyError(PlugwiseException): - """Energy error.""" + """Error calculating or retrieving energy data.""" class FeatureError(PlugwiseException): - """Feature error.""" + """Error accessing or using node features.""" class MessageError(PlugwiseException): - """Message errors.""" + """Error processing or validating messages.""" class NodeError(PlugwiseException): """Node failed to execute request.""" class NodeTimeout(PlugwiseException): """No response from node.""" class StickError(PlugwiseException): """Error at USB stick connection.""" class StickFailed(PlugwiseException): """USB stick failed to accept request.""" class StickTimeout(PlugwiseException): """Response timed out from USB-Stick.""" class SubscriptionError(PlugwiseException): - """Subscription Errors.""" + """Error managing event subscriptions."""plugwise_usb/nodes/helpers/subscription.py (2)
22-29: Fix formatting issues flagged by static analysis.The class has minor formatting issues that should be addressed for consistency.
Apply these formatting fixes:
class FeaturePublisher: """Base Class to call awaitable of subscription when event happens.""" + def __init__(self) -> None: + """Initialize the feature publisher.""" self._feature_update_subscribers: dict[ Callable[[], None], NodeFeatureSubscription, ] = {}🧰 Tools
🪛 Ruff (0.11.9)
23-23: 1 blank line required after class docstring
Insert 1 blank line after class docstring
(D204)
24-24: Missing docstring in
__init__(D107)
51-64: Simplify the callback list check for better readability.The condition
if len(callback_list) > 0:can be made more Pythonic.- if len(callback_list) > 0: + if callback_list: await gather(*callback_list)plugwise_usb/connection/sender.py (1)
47-51: Remove trailing whitespace.Line 51 contains trailing whitespace that should be removed.
def processed_messages(self) -> int: """Return the number of processed messages.""" return self._processed_msgs - +🧰 Tools
🪛 Ruff (0.11.9)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/__init__.py (2)
27-67: Add trailing commas for consistency.Apply this diff to add missing trailing commas:
return PlugwiseCirclePlus( mac, address, controller, loaded_callback, - ) + ) if node_type == NodeType.CIRCLE: return PlugwiseCircle( mac, address, controller, loaded_callback, - ) + ) if node_type == NodeType.SWITCH: return PlugwiseSwitch( mac, address, controller, loaded_callback, - ) + ) if node_type == NodeType.SENSE: return PlugwiseSense( mac, address, controller, loaded_callback, - ) + ) if node_type == NodeType.SCAN: return PlugwiseScan( mac, address, controller, loaded_callback, - ) + ) if node_type == NodeType.STEALTH: return PlugwiseStealth( mac, address, controller, loaded_callback, - ) + )
68-68: Consider logging when returning None for unsupported node types.Returning None silently for unsupported node types could make debugging difficult. Consider adding a warning log to help identify issues.
Add logging before returning None:
+import logging + +_LOGGER = logging.getLogger(__name__) + from ..api import NodeEvent, NodeType, PlugwiseNode # ... rest of imports ... def get_plugwise_node( # ... parameters ... ) -> PlugwiseNode | None: """Return an initialized plugwise node class based on given the node type.""" # ... existing code ... + _LOGGER.warning("Unsupported node type: %s", node_type) return Nonetests/stick_test_data.py (2)
16-26: Modernize string formatting from % style to f-strings.The code uses old-style % formatting which is less readable than modern f-strings.
Update to use f-strings:
LOG_TIMESTAMPS = {} _one_hour = timedelta(hours=1) for x in range(168): delta_month = hour_timestamp - hour_timestamp.replace(day=1, hour=0) LOG_TIMESTAMPS[x] = ( - bytes(("%%0%dX" % 2) % (hour_timestamp.year - 2000), pw_constants.UTF8) - + bytes(("%%0%dX" % 2) % hour_timestamp.month, pw_constants.UTF8) - + bytes( - ("%%0%dX" % 4) - % int((delta_month.days * 1440) + (delta_month.seconds / 60)), - pw_constants.UTF8, - ) + bytes(f"{hour_timestamp.year - 2000:02X}", pw_constants.UTF8) + + bytes(f"{hour_timestamp.month:02X}", pw_constants.UTF8) + + bytes( + f"{int((delta_month.days * 1440) + (delta_month.seconds / 60)):04X}", + pw_constants.UTF8, + ) ) hour_timestamp -= _one_hour
597-603: Modernize datetime formatting to use f-strings.Update the datetime formatting:
b"003A" # msg_id + b"0098765432101234" # mac - + bytes(("%%0%dd" % 2) % utc_now.second, pw_constants.UTF8) - + bytes(("%%0%dd" % 2) % utc_now.minute, pw_constants.UTF8) - + bytes(("%%0%dd" % 2) % utc_now.hour, pw_constants.UTF8) - + bytes(("%%0%dd" % 2) % utc_now.weekday(), pw_constants.UTF8) - + bytes(("%%0%dd" % 2) % utc_now.day, pw_constants.UTF8) - + bytes(("%%0%dd" % 2) % utc_now.month, pw_constants.UTF8) - + bytes(("%%0%dd" % 2) % (utc_now.year - 2000), pw_constants.UTF8), + + bytes(f"{utc_now.second:02d}", pw_constants.UTF8) + + bytes(f"{utc_now.minute:02d}", pw_constants.UTF8) + + bytes(f"{utc_now.hour:02d}", pw_constants.UTF8) + + bytes(f"{utc_now.weekday():02d}", pw_constants.UTF8) + + bytes(f"{utc_now.day:02d}", pw_constants.UTF8) + + bytes(f"{utc_now.month:02d}", pw_constants.UTF8) + + bytes(f"{utc_now.year - 2000:02d}", pw_constants.UTF8),plugwise_usb/nodes/helpers/counter.py (1)
277-298: Add documentation for the midnight reset logic.The midnight transition handling for day counters is complex but necessary. Consider adding a detailed docstring or inline comments explaining:
- Why the reset time adjustment is postponed until the device pulse counter resets
- The state transitions of the
_midnight_reset_passedflag- Example scenarios where this logic prevents calculation errors
plugwise_usb/messages/properties.py (2)
152-153: Remove or clarify outdated TODO comment.The TODO comment states "negative is not initialized" but
negativeis defined as a static method at line 138. This comment appears to be outdated or incorrect.def deserialize(self, val: bytes) -> None: """Convert current string formatted value into integer value.""" - # TODO: negative is not initialized! 20220405 self._value = self.negative(int(val, 16), self.length)
393-395: Simplify integer literal.Use
0directly instead ofint(0).if val == b"00000000": - self._value = int(0) + self._value = 0 return🧰 Tools
🪛 Ruff (0.11.9)
394-394: Unnecessary
intcall (rewrite as a literal)Replace with integer literal
(UP018)
plugwise_usb/nodes/sense.py (1)
120-120: Remove whitespace from blank line.Line 120 contains trailing whitespace that should be removed for cleaner formatting.
) report_received = True - + return report_received🧰 Tools
🪛 Ruff (0.11.9)
120-120: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/network/registry.py (1)
182-225: Optimize network scanning with better sleep management.The current implementation uses fixed sleep intervals (0.1s for quick scan, 10s for full scan) which may not be optimal for all network conditions. Consider implementing adaptive delays based on network response times or making the delays configurable.
await sleep(0.1) if not quick: - await sleep(10) + await sleep(min(10, max(1, response_time * 10))) # Adaptive delayplugwise_usb/nodes/scan.py (1)
52-52: Remove trailing whitespace.There's trailing whitespace at the end of this line that should be removed for consistency.
Apply this diff to fix the formatting issue:
-SENSITIVITY_MEDIUM_VALUE = 30 # 0x1E +SENSITIVITY_MEDIUM_VALUE = 30 # 0x1E🧰 Tools
🪛 Ruff (0.11.9)
52-52: Trailing whitespace
Remove trailing whitespace
(W291)
plugwise_usb/__init__.py (1)
288-288: Remove trailing whitespace from blank line.- +🧰 Tools
🪛 Ruff (0.11.9)
288-288: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/circle.py (1)
104-109: Simplify calibrated property.The property can be simplified to directly return the boolean expression.
@property def calibrated(self) -> bool: """State of calibration.""" - if self._calibration is not None: - return True - return False + return self._calibration is not Noneplugwise_usb/nodes/sed.py (1)
242-242: Remove trailing whitespace from blank line.- +🧰 Tools
🪛 Ruff (0.11.9)
242-242: 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 (55)
.github/workflows/merge.yml(1 hunks).github/workflows/verify.yml(5 hunks).gitignore(1 hunks).pre-commit-config.yaml(1 hunks)CHANGELOG.md(1 hunks)CODEOWNERS(1 hunks)plugwise_usb/__init__.py(1 hunks)plugwise_usb/api.py(1 hunks)plugwise_usb/connection/__init__.py(1 hunks)plugwise_usb/connection/manager.py(1 hunks)plugwise_usb/connection/queue.py(1 hunks)plugwise_usb/connection/receiver.py(1 hunks)plugwise_usb/connection/sender.py(1 hunks)plugwise_usb/connections/__init__.py(0 hunks)plugwise_usb/connections/serial.py(0 hunks)plugwise_usb/connections/socket.py(0 hunks)plugwise_usb/constants.py(2 hunks)plugwise_usb/controller.py(0 hunks)plugwise_usb/exceptions.py(1 hunks)plugwise_usb/helpers/__init__.py(1 hunks)plugwise_usb/helpers/cache.py(1 hunks)plugwise_usb/helpers/util.py(1 hunks)plugwise_usb/messages/__init__.py(1 hunks)plugwise_usb/messages/properties.py(1 hunks)plugwise_usb/messages/requests.py(2 hunks)plugwise_usb/messages/responses.py(4 hunks)plugwise_usb/network/__init__.py(1 hunks)plugwise_usb/network/cache.py(1 hunks)plugwise_usb/network/registry.py(1 hunks)plugwise_usb/nodes/__init__.py(1 hunks)plugwise_usb/nodes/celsius.py(1 hunks)plugwise_usb/nodes/circle.py(1 hunks)plugwise_usb/nodes/circle_plus.py(1 hunks)plugwise_usb/nodes/helpers/__init__.py(1 hunks)plugwise_usb/nodes/helpers/cache.py(1 hunks)plugwise_usb/nodes/helpers/counter.py(1 hunks)plugwise_usb/nodes/helpers/firmware.py(1 hunks)plugwise_usb/nodes/helpers/pulses.py(1 hunks)plugwise_usb/nodes/helpers/subscription.py(1 hunks)plugwise_usb/nodes/node.py(1 hunks)plugwise_usb/nodes/scan.py(1 hunks)plugwise_usb/nodes/sed.py(1 hunks)plugwise_usb/nodes/sense.py(1 hunks)plugwise_usb/nodes/stealth.py(1 hunks)plugwise_usb/nodes/switch.py(1 hunks)plugwise_usb/parser.py(0 hunks)plugwise_usb/util.py(0 hunks)pyproject.toml(7 hunks)requirements_commit.txt(1 hunks)requirements_test.txt(1 hunks)scripts/python-venv.sh(1 hunks)setup.cfg(0 hunks)setup.py(0 hunks)tests/bandit.yaml(0 hunks)tests/stick_test_data.py(1 hunks)
💤 Files with no reviewable changes (9)
- tests/bandit.yaml
- setup.cfg
- setup.py
- plugwise_usb/connections/socket.py
- plugwise_usb/parser.py
- plugwise_usb/connections/serial.py
- plugwise_usb/controller.py
- plugwise_usb/connections/init.py
- plugwise_usb/util.py
✅ Files skipped from review due to trivial changes (2)
- requirements_commit.txt
- .github/workflows/merge.yml
🚧 Files skipped from review as they are similar to previous changes (18)
- plugwise_usb/nodes/stealth.py
- plugwise_usb/helpers/init.py
- scripts/python-venv.sh
- .gitignore
- .pre-commit-config.yaml
- requirements_test.txt
- CODEOWNERS
- .github/workflows/verify.yml
- CHANGELOG.md
- plugwise_usb/nodes/helpers/init.py
- plugwise_usb/helpers/util.py
- plugwise_usb/nodes/helpers/firmware.py
- pyproject.toml
- plugwise_usb/nodes/helpers/cache.py
- plugwise_usb/nodes/celsius.py
- plugwise_usb/network/cache.py
- plugwise_usb/nodes/switch.py
- plugwise_usb/connection/receiver.py
🧰 Additional context used
🧠 Learnings (1)
plugwise_usb/nodes/helpers/pulses.py (1)
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#236
File: plugwise_usb/nodes/helpers/pulses.py:270-313
Timestamp: 2025-05-24T09:02:22.887Z
Learning: In the Plugwise USB library, production pulse values (self._pulses_production, pulses_produced) are stored as negative numbers, while consumption pulse values (self._pulses_consumption, pulses_consumed) are stored as positive numbers. This means pulse counter reset detection uses different comparison operators: `>` for positive consumption values and `<` for negative production values.
🧬 Code Graph Analysis (7)
plugwise_usb/helpers/cache.py (4)
tests/test_usb.py (6)
os_path_join(1520-1522)makedirs(1541-1546)makedirs(1644-1649)aiofiles_os_remove(1532-1539)aiofiles_os_remove(1635-1642)exists(208-222)plugwise_usb/exceptions.py (1)
CacheError(8-9)plugwise_usb/network/__init__.py (1)
initialize_cache(115-119)plugwise_usb/network/registry.py (1)
initialize_cache(66-70)
plugwise_usb/connection/__init__.py (7)
plugwise_usb/api.py (2)
StickEvent(12-19)mac(270-271)plugwise_usb/exceptions.py (2)
NodeError(24-25)StickError(32-33)plugwise_usb/helpers/util.py (1)
version_to_model(24-38)plugwise_usb/messages/requests.py (21)
NodeInfoRequest(892-921)NodePingRequest(600-630)PlugwiseRequest(61-343)StickInitRequest(524-553)seq_id(132-134)seq_id(137-146)send(365-374)send(387-396)send(438-440)send(476-485)send(512-521)send(542-553)send(566-575)send(588-597)send(621-630)send(667-676)send(719-728)send(775-784)send(808-817)send(848-857)send(880-889)plugwise_usb/messages/responses.py (11)
NodeInfoResponse(582-654)NodePingResponse(358-394)PlugwiseResponse(98-215)StickInitResponse(411-457)network_id(446-448)network_online(451-453)mac_network_controller(440-443)firmware(628-630)hardware(623-625)retries(123-125)retries(128-130)plugwise_usb/connection/manager.py (8)
StickConnectionManager(23-196)is_connected(52-58)serial_path(47-49)setup_connection_to_stick(123-166)subscribe_to_stick_events(84-102)_handle_stick_event(70-82)subscribe_to_messages(104-121)disconnect_from_stick(184-196)plugwise_usb/connection/queue.py (6)
StickQueue(28-156)_handle_stick_event(56-61)start(45-54)is_running(41-43)stop(63-75)submit(77-126)
plugwise_usb/nodes/helpers/pulses.py (6)
plugwise_usb/exceptions.py (1)
EnergyError(12-13)plugwise_usb/messages/responses.py (1)
offset(494-496)plugwise_usb/nodes/helpers/counter.py (8)
timestamp(170-178)is_consumption(229-231)collected_logs(66-68)log_rollover(124-126)last_update(273-275)direction(209-211)add_empty_log(70-72)log_addresses_missing(119-121)plugwise_usb/nodes/node.py (1)
mac(230-232)plugwise_usb/messages/__init__.py (1)
mac(52-56)plugwise_usb/api.py (1)
mac(270-271)
plugwise_usb/nodes/helpers/counter.py (4)
plugwise_usb/api.py (3)
EnergyStatistics(210-222)mac(270-271)energy(331-335)plugwise_usb/exceptions.py (1)
EnergyError(12-13)plugwise_usb/nodes/helpers/__init__.py (1)
EnergyCalibration(14-20)plugwise_usb/nodes/helpers/pulses.py (15)
PulseCollection(49-1040)PulseLogRecord(41-46)collected_logs(99-106)add_empty_log(380-411)add_log(414-448)logs(109-123)update_pulse_counter(270-312)log_interval_consumption(141-143)log_interval_production(146-148)log_rollover(151-153)recalculate_missing_log_addresses(450-454)production_logging(136-138)last_update(156-158)pulse_counter_reset(161-163)collected_pulses(165-213)
plugwise_usb/__init__.py (15)
plugwise_usb/api.py (14)
NodeEvent(30-36)PlugwiseNode(225-680)StickEvent(12-19)cache_enabled(312-313)cache_enabled(316-317)cache_folder(296-297)cache_folder(300-301)name(242-243)clear_cache(319-320)load(250-251)cache_folder_create(304-305)cache_folder_create(308-309)mac(270-271)disconnect(291-292)plugwise_usb/connection/__init__.py (13)
StickController(31-258)is_connected(57-59)is_initialized(50-54)firmware_stick(62-64)hardware_stick(67-69)mac_stick(72-78)mac_coordinator(81-90)network_online(102-108)network_id(93-99)subscribe_to_stick_events(122-136)connect_to_stick(110-120)initialize_stick(162-203)disconnect_from_stick(251-258)plugwise_usb/exceptions.py (4)
MessageError(20-21)NodeError(24-25)StickError(32-33)SubscriptionError(44-45)plugwise_usb/network/__init__.py (20)
StickNetwork(35-568)cache_enabled(79-81)cache_enabled(84-90)cache_folder(93-95)cache_folder(98-103)nodes(134-138)registry(141-143)is_running(129-131)allow_join_requests(524-538)clear_cache(158-160)subscribe_to_node_events(540-558)discover_nodes(498-508)cache_folder_create(106-108)cache_folder_create(111-113)initialize_cache(115-119)start(489-496)discover_network_coordinator(304-337)register_node(147-156)unregister_node(162-170)stop(510-520)plugwise_usb/nodes/circle.py (3)
decorated(65-68)load(745-803)initialize(838-874)plugwise_usb/nodes/helpers/__init__.py (1)
decorated(29-32)plugwise_usb/connection/receiver.py (2)
is_connected(147-149)subscribe_to_stick_events(303-320)plugwise_usb/nodes/node.py (12)
cache_enabled(147-149)cache_enabled(152-154)cache_folder(127-129)cache_folder(132-134)name(213-217)clear_cache(383-386)load(365-367)initialize(401-409)cache_folder_create(137-139)cache_folder_create(142-144)mac(230-232)disconnect(356-359)plugwise_usb/nodes/helpers/cache.py (1)
clear_cache(48-51)plugwise_usb/network/cache.py (1)
clear_cache(40-43)plugwise_usb/nodes/switch.py (2)
load(44-62)initialize(65-76)plugwise_usb/nodes/scan.py (2)
load(87-111)initialize(114-125)plugwise_usb/nodes/sense.py (2)
load(51-70)initialize(73-84)plugwise_usb/nodes/celsius.py (1)
load(26-48)plugwise_usb/nodes/circle_plus.py (1)
load(24-79)
plugwise_usb/messages/properties.py (3)
plugwise_usb/exceptions.py (1)
MessageError(20-21)plugwise_usb/helpers/util.py (1)
int_to_uint(53-58)plugwise_usb/messages/responses.py (1)
deserialize(137-201)
plugwise_usb/nodes/helpers/subscription.py (2)
plugwise_usb/api.py (2)
NodeFeature(39-54)features(230-231)plugwise_usb/nodes/node.py (1)
features(185-187)
🪛 Ruff (0.11.9)
plugwise_usb/connection/sender.py
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/helpers/cache.py
62-62: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/constants.py
34-34: Trailing whitespace
Remove trailing whitespace
(W291)
plugwise_usb/nodes/sense.py
120-120: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/connection/queue.py
79-79: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/__init__.py
288-288: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/nodes/scan.py
52-52: Trailing whitespace
Remove trailing whitespace
(W291)
plugwise_usb/nodes/sed.py
242-242: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/connection/manager.py
40-40: Missing docstring in public method
(D102)
43-43: Missing docstring in public method
(D102)
plugwise_usb/messages/properties.py
394-394: Unnecessary int call (rewrite as a literal)
Replace with integer literal
(UP018)
plugwise_usb/nodes/helpers/subscription.py
23-23: 1 blank line required after class docstring
Insert 1 blank line after class docstring
(D204)
24-24: Missing docstring in __init__
(D107)
tests/stick_test_data.py
1-1: Missing docstring in public module
(D100)
🪛 GitHub Check: codecov/patch
plugwise_usb/connection/queue.py
[warning] 97-97: plugwise_usb/connection/queue.py#L97
Added line #L97 was not covered by tests
[warning] 109-109: plugwise_usb/connection/queue.py#L109
Added line #L109 was not covered by tests
[warning] 111-111: plugwise_usb/connection/queue.py#L111
Added line #L111 was not covered by tests
[warning] 113-113: plugwise_usb/connection/queue.py#L113
Added line #L113 was not covered by tests
plugwise_usb/messages/requests.py
[warning] 286-286: plugwise_usb/messages/requests.py#L286
Added line #L286 was not covered by tests
[warning] 293-293: plugwise_usb/messages/requests.py#L293
Added line #L293 was not covered by tests
[warning] 298-298: plugwise_usb/messages/requests.py#L298
Added line #L298 was not covered by tests
[warning] 300-300: plugwise_usb/messages/requests.py#L300
Added line #L300 was not covered by tests
[warning] 306-306: plugwise_usb/messages/requests.py#L306
Added line #L306 was not covered by tests
[warning] 308-308: plugwise_usb/messages/requests.py#L308
Added line #L308 was not covered by tests
[warning] 367-367: plugwise_usb/messages/requests.py#L367
Added line #L367 was not covered by tests
plugwise_usb/__init__.py
[warning] 209-210: plugwise_usb/init.py#L209-L210
Added lines #L209 - L210 were not covered by tests
[warning] 322-322: plugwise_usb/init.py#L322
Added line #L322 was not covered by tests
[warning] 334-334: plugwise_usb/init.py#L334
Added line #L334 was not covered by tests
[warning] 342-342: plugwise_usb/init.py#L342
Added line #L342 was not covered by tests
[warning] 352-355: plugwise_usb/init.py#L352-L355
Added lines #L352 - L355 were not covered by tests
[warning] 363-366: plugwise_usb/init.py#L363-L366
Added lines #L363 - L366 were not covered by tests
plugwise_usb/messages/properties.py
[warning] 76-76: plugwise_usb/messages/properties.py#L76
Added line #L76 was not covered by tests
[warning] 96-96: plugwise_usb/messages/properties.py#L96
Added line #L96 was not covered by tests
[warning] 125-125: plugwise_usb/messages/properties.py#L125
Added line #L125 was not covered by tests
[warning] 159-159: plugwise_usb/messages/properties.py#L159
Added line #L159 was not covered by tests
[warning] 207-207: plugwise_usb/messages/properties.py#L207
Added line #L207 was not covered by tests
[warning] 244-244: plugwise_usb/messages/properties.py#L244
Added line #L244 was not covered by tests
[warning] 251-251: plugwise_usb/messages/properties.py#L251
Added line #L251 was not covered by tests
[warning] 276-276: plugwise_usb/messages/properties.py#L276
Added line #L276 was not covered by tests
[warning] 301-301: plugwise_usb/messages/properties.py#L301
Added line #L301 was not covered by tests
[warning] 330-330: plugwise_usb/messages/properties.py#L330
Added line #L330 was not covered by tests
[warning] 359-359: plugwise_usb/messages/properties.py#L359
Added line #L359 was not covered by tests
[warning] 380-380: plugwise_usb/messages/properties.py#L380
Added line #L380 was not covered by tests
[warning] 403-403: plugwise_usb/messages/properties.py#L403
Added line #L403 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and publish Python 🐍 distributions 📦 to TestPyPI
🔇 Additional comments (27)
plugwise_usb/connection/queue.py (1)
87-126:❌ Incorrect review comment
Verify retry loop termination and exception handling.
The retry loop
while request.resend:should eventually terminate when retries are exhausted, but this should be verified. Also, the exception handlers on lines 119-124 always callself._stick.correct_received_messages(1)which may not be appropriate for all exception types.
🏁 Script executed:
#!/bin/bash # Description: Verify that PlugwiseRequest has proper retry limit handling to prevent infinite loops # Search for retry-related properties and methods in request classes rg -A 5 -B 5 "resend|retries_left|max_retries" --type pyLength of output: 8012
🏁 Script executed:
#!/bin/bash set -e echo "=== Inspecting _add_request_to_queue implementation in queue.py ===" rg -C 20 "async def _add_request_to_queue" -n plugwise_usb/connection/queue.py echo echo "=== Searching for add_send_attempt calls in queue.py ===" rg -C 5 "add_send_attempt" -n plugwise_usb/connection/queue.py echo echo "=== Inspecting write_to_stick in manager.py ===" rg -C 20 "async def write_to_stick" -n plugwise_usb/connection/manager.py echo echo "=== Searching for add_send_attempt calls in manager.py ===" rg -C 5 "add_send_attempt" -n plugwise_usb/connection/manager.pyLength of output: 2416
🏁 Script executed:
#!/bin/bash set -e echo "=== Searching for write_to_stick definition ===" rg "def write_to_stick" -n plugwise_usb/connection/manager.py echo echo "=== Showing manager.py around write_to_stick ===" rg -C 20 "write_to_stick" -n plugwise_usb/connection/manager.py echo echo "=== Searching for add_send_attempt calls across the repo ===" rg -n "add_send_attempt" .Length of output: 2605
🏁 Script executed:
#!/bin/bash set -e echo "=== Inspecting add_send_attempt in sender.py ===" rg -C 20 "add_send_attempt" -n plugwise_usb/connection/sender.py echo echo "=== Locating correct_received_messages definition ===" rg -C 20 "def correct_received_messages" -n . echo echo "=== Inspecting correct_received_messages usage ===" rg -n "correct_received_messages" -n .Length of output: 5635
Retry loop and exception handling are correct as implemented
The
while request.resend:loop uses
resend → (_send_counter < _max_retries), and every send attempt in
StickSender.write_request_to_port()callsrequest.add_send_attempt().
_send_counterwill reach_max_retries, makingrequest.resendfalse and terminating the loop.
self._stick.correct_received_messages(1)is only invoked in the
except StickErrorandexcept BaseExceptionhandlers (queue.py lines 113–118)
to roll back the internal message count when a non‐timeout error occurs.
Timeout exceptions (NodeTimeout/StickTimeout) do not callcorrect_received_messages.No changes required.
Likely an incorrect or invalid review comment.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 97-97: plugwise_usb/connection/queue.py#L97
Added line #L97 was not covered by tests
[warning] 109-109: plugwise_usb/connection/queue.py#L109
Added line #L109 was not covered by tests
[warning] 111-111: plugwise_usb/connection/queue.py#L111
Added line #L111 was not covered by tests
[warning] 113-113: plugwise_usb/connection/queue.py#L113
Added line #L113 was not covered by testsplugwise_usb/nodes/helpers/counter.py (2)
1-50: LGTM! Well-structured module setup.The imports, enum definition, and constant groupings are well organized and follow best practices.
52-179: Well-designed energy counter collection management.The
EnergyCountersclass effectively encapsulates the management of multiple energy counters with clear separation of concerns. The integration withPulseCollectionand proper null checks for calibration data are good practices.plugwise_usb/connection/__init__.py (3)
31-109: Well-structured controller initialization and properties.The separation between connection and initialization states is clear, and the error handling with descriptive messages helps with debugging. The properties appropriately validate state before returning values.
162-204: Robust stick initialization sequence.The initialization properly validates connection state, sends init request, processes response, retrieves node info, and validates network status. The error messages are descriptive and helpful for troubleshooting.
205-258: Clean implementation of node communication and lifecycle methods.The
get_node_detailsmethod efficiently handles optional ping operations,sendprovides flexible error handling, anddisconnect_from_stickensures proper resource cleanup. The implementation follows async best practices.plugwise_usb/nodes/helpers/pulses.py (1)
270-313: LGTM! Pulse counter reset detection logic is correct.Based on the retrieved learnings, the comparison operators are correctly implemented. Production pulse values are stored as negative numbers while consumption values are positive, so the
<operator for production (line 290) and>operator for consumption (line 283) properly detect counter resets.plugwise_usb/nodes/sense.py (1)
93-121: LGTM! Return value logic has been fixed.The method now correctly uses a
report_receivedflag to track whether either temperature or humidity was processed successfully, addressing the previous issue where only humidity processing would returnTrue.🧰 Tools
🪛 Ruff (0.11.9)
120-120: Blank line contains whitespace
Remove whitespace from blank line
(W293)
plugwise_usb/network/registry.py (2)
140-152: LGTM! Duplicate variable assignment has been resolved.The previous issue with
mac_of_nodebeing assigned twice has been properly fixed using the walrus operator in the conditional check.
260-264: LGTM! String formatting has been corrected.The string formatting issue has been resolved - the error message now properly uses an f-string to include the exception message.
plugwise_usb/network/__init__.py (3)
251-267: LGTM! Whitespace and return logic issues have been resolved.The previous whitespace issues and inconsistent return value logic have been properly addressed in the
node_join_available_messagemethod.
269-290: LGTM! Whitespace issues have been resolved.The trailing whitespace on blank lines has been cleaned up in the
node_rejoin_messagemethod.
540-568: Well-designed subscription pattern for node events.The subscription mechanism is well-implemented with proper cleanup via the returned unsubscribe function. This provides a clean way for external components to listen to node events without memory leaks.
plugwise_usb/constants.py (2)
10-27: LGTM: Well-organized cache and time constants.The addition of cache-related constants and time unit constants provides good structure for the async refactoring. The organization is clear and the naming is consistent.
60-90: LGTM: Improved hardware model names with proper spacing.The addition of spaces around "+" signs in device names (e.g., "Circle + type B") improves readability and consistency in the hardware model dictionary.
plugwise_usb/nodes/node.py (4)
55-87: LGTM: Comprehensive base class initialization.The constructor properly initializes all necessary components including controller references, message subscriptions, caching, and node state tracking. The separation of concerns is well-designed.
319-349: LGTM: Smart protocol and feature detection.The
_setup_protocolmethod intelligently determines supported features based on firmware versions, providing good backward compatibility while enabling new features when available.
654-667: LGTM: Efficient async cache management.The cache saving implementation with task management prevents concurrent writes and supports both triggered and immediate saving modes, which is well-suited for the async architecture.
681-772: LGTM: Proper abstract method pattern.All configuration methods appropriately raise
NotImplementedError, forcing subclasses to implement device-specific behavior while maintaining a consistent interface.plugwise_usb/nodes/scan.py (5)
87-126: LGTM: Comprehensive async initialization.The load and initialize methods properly handle cache restoration, default loading, protocol setup, and message subscriptions. The error handling and logging are appropriate for debugging.
276-345: LGTM: Smart configuration queuing system.The configuration methods intelligently queue changes and schedule a single configuration task when the node is awake. This prevents unnecessary wake-ups and batches configuration changes efficiently.
309-312: LGTM: Proper input validation.The validation for motion reset timer properly checks bounds (1-255 minutes) and provides a clear error message for invalid values.
465-511: LGTM: Robust configuration handling.The
scan_configuremethod properly handles different response types, clears pending changes on failure, and provides appropriate logging for debugging configuration issues.
555-579: LGTM: Efficient state retrieval.The
get_statemethod efficiently handles device-specific features while delegating unsupported features to the superclass, and ensures availability state is always included.plugwise_usb/messages/requests.py (1)
61-109: Well-designed async request framework.The
PlugwiseRequestbase class provides a comprehensive foundation for async request/response handling with proper state management, subscription handling, and error propagation. The design supports retry logic, timeout management, and clean unsubscription patterns.plugwise_usb/api.py (1)
225-681: Excellent protocol design for node interface.The
PlugwiseNodeprotocol provides a comprehensive and well-structured interface that clearly defines:
- Feature support and node properties
- Async lifecycle management (load/unload)
- State retrieval and configuration methods
- Proper error handling with specific exceptions
- Clear documentation for each method
This creates a solid foundation for the async node implementations.
plugwise_usb/messages/responses.py (1)
98-216: Excellent refactor of response message architecture.The new
PlugwiseResponsebase class significantly improves:
- Type safety with explicit typed attributes
- Robust deserialization with comprehensive validation
- Unified error handling with
MessageError- Clean parameter parsing with proper error wrapping
This provides a solid foundation for the async message handling framework.
| if node_type == NodeType.CIRCLE_PLUS: | ||
| return PlugwiseCirclePlus( | ||
| mac, | ||
| address, | ||
| controller, | ||
| loaded_callback, | ||
| ) | ||
|
|
||
| def _request_features(self, callback=None): | ||
| """Request supported features for this node.""" | ||
| self.message_sender( | ||
| NodeFeaturesRequest(self._mac), | ||
| callback, | ||
| if node_type == NodeType.CIRCLE: | ||
| return PlugwiseCircle( | ||
| mac, | ||
| address, | ||
| controller, | ||
| loaded_callback, | ||
| ) | ||
|
|
||
| def _request_ping(self, callback=None, ignore_sensor=True): | ||
| """Ping node.""" | ||
| if ignore_sensor or FEATURE_PING["id"] in self._callbacks: | ||
| self.message_sender( | ||
| NodePingRequest(self._mac), | ||
| callback, | ||
| ) | ||
|
|
||
| def message_for_node(self, message): | ||
| """Process received message.""" | ||
| if message.mac == self._mac: | ||
| if message.timestamp is not None: | ||
| _LOGGER.debug( | ||
| "Previous update %s of node %s, last message %s", | ||
| str(self._last_update), | ||
| self.mac, | ||
| str(message.timestamp), | ||
| ) | ||
| self._last_update = message.timestamp | ||
| if not self._available: | ||
| self.available = True | ||
| self._request_info() | ||
| if isinstance(message, NodePingResponse): | ||
| self._process_ping_response(message) | ||
| elif isinstance(message, NodeInfoResponse): | ||
| self._process_info_response(message) | ||
| elif isinstance(message, NodeFeaturesResponse): | ||
| self._process_features_response(message) | ||
| elif isinstance(message, NodeJoinAckResponse): | ||
| self._process_join_ack_response(message) | ||
| else: | ||
| self.message_for_circle(message) | ||
| self.message_for_sed(message) | ||
| else: | ||
| _LOGGER.debug( | ||
| "Skip message, mac of node (%s) != mac at message (%s)", | ||
| message.mac.decode(UTF8_DECODE), | ||
| self.mac, | ||
| ) | ||
|
|
||
| def message_for_circle(self, message): | ||
| """Pass messages to PlugwiseCircle class""" | ||
|
|
||
| def message_for_sed(self, message): | ||
| """Pass messages to NodeSED class""" | ||
|
|
||
| def subscribe_callback(self, callback, sensor) -> bool: | ||
| """Subscribe callback to execute when state change happens.""" | ||
| if sensor in self._features: | ||
| if sensor not in self._callbacks: | ||
| self._callbacks[sensor] = [] | ||
| self._callbacks[sensor].append(callback) | ||
| return True | ||
| return False | ||
|
|
||
| def unsubscribe_callback(self, callback, sensor): | ||
| """Unsubscribe callback to execute when state change happens.""" | ||
| if sensor in self._callbacks: | ||
| self._callbacks[sensor].remove(callback) | ||
|
|
||
| def do_callback(self, sensor): | ||
| """Execute callbacks registered for specified callback type.""" | ||
| if sensor in self._callbacks: | ||
| for callback in self._callbacks[sensor]: | ||
| try: | ||
| callback(None) | ||
| # TODO: narrow exception | ||
| except Exception as err: # pylint: disable=broad-except | ||
| _LOGGER.error( | ||
| "Error while executing all callback : %s", | ||
| err, | ||
| ) | ||
|
|
||
| def _process_join_ack_response(self, message): | ||
| """Process join acknowledge response message""" | ||
| _LOGGER.info( | ||
| "Node %s has (re)joined plugwise network", | ||
| self.mac, | ||
| if node_type == NodeType.SWITCH: | ||
| return PlugwiseSwitch( | ||
| mac, | ||
| address, | ||
| controller, | ||
| loaded_callback, | ||
| ) | ||
|
|
||
| def _process_ping_response(self, message): | ||
| """Process ping response message.""" | ||
| if self._rssi_in != message.rssi_in.value: | ||
| self._rssi_in = message.rssi_in.value | ||
| self.do_callback(FEATURE_RSSI_IN["id"]) | ||
| if self._rssi_out != message.rssi_out.value: | ||
| self._rssi_out = message.rssi_out.value | ||
| self.do_callback(FEATURE_RSSI_OUT["id"]) | ||
| if self._ping != message.ping_ms.value: | ||
| self._ping = message.ping_ms.value | ||
| self.do_callback(FEATURE_PING["id"]) | ||
|
|
||
| def _process_info_response(self, message): | ||
| """Process info response message.""" | ||
| _LOGGER.debug( | ||
| "Response info message for node %s, last log address %s", | ||
| self.mac, | ||
| str(message.last_logaddr.value), | ||
| if node_type == NodeType.SENSE: | ||
| return PlugwiseSense( | ||
| mac, | ||
| address, | ||
| controller, | ||
| loaded_callback, | ||
| ) | ||
| if message.relay_state.serialize() == b"01": | ||
| if not self._relay_state: | ||
| self._relay_state = True | ||
| self.do_callback(FEATURE_RELAY["id"]) | ||
| else: | ||
| if self._relay_state: | ||
| self._relay_state = False | ||
| self.do_callback(FEATURE_RELAY["id"]) | ||
| self._hardware_version = message.hw_ver.value.decode(UTF8_DECODE) | ||
| self._firmware_version = message.fw_ver.value | ||
| self._node_type = message.node_type.value | ||
| if self._last_log_address != message.last_logaddr.value: | ||
| self._last_log_address = message.last_logaddr.value | ||
| _LOGGER.debug("Node type = %s", self.hardware_model) | ||
| if not self._battery_powered: | ||
| _LOGGER.debug("Relay state = %s", str(self._relay_state)) | ||
| _LOGGER.debug("Hardware version = %s", str(self._hardware_version)) | ||
| _LOGGER.debug("Firmware version = %s", str(self._firmware_version)) | ||
|
|
||
| def _process_features_response(self, message): | ||
| """Process features message.""" | ||
| _LOGGER.warning( | ||
| "Node %s supports features %s", self.mac, str(message.features.value) | ||
| if node_type == NodeType.SCAN: | ||
| return PlugwiseScan( | ||
| mac, | ||
| address, | ||
| controller, | ||
| loaded_callback, | ||
| ) | ||
| if node_type == NodeType.STEALTH: | ||
| return PlugwiseStealth( | ||
| mac, | ||
| address, | ||
| controller, | ||
| loaded_callback, | ||
| ) | ||
| self._device_features = message.features.value | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the if-else chain to use a dictionary mapping for better maintainability.
The current implementation uses a long if-else chain that could be simplified using a dictionary mapping. This would make it easier to add new node types and improve readability.
Apply this refactor to improve maintainability:
def get_plugwise_node(
mac: str,
address: int,
controller: StickController,
loaded_callback: Callable[[NodeEvent, str], Awaitable[None]],
node_type: NodeType,
) -> PlugwiseNode | None:
"""Return an initialized plugwise node class based on given the node type."""
+
+ node_class_map = {
+ NodeType.CIRCLE_PLUS: PlugwiseCirclePlus,
+ NodeType.CIRCLE: PlugwiseCircle,
+ NodeType.SWITCH: PlugwiseSwitch,
+ NodeType.SENSE: PlugwiseSense,
+ NodeType.SCAN: PlugwiseScan,
+ NodeType.STEALTH: PlugwiseStealth,
+ }
+
+ node_class = node_class_map.get(node_type)
+ if node_class is None:
+ return None
+
+ return node_class(
+ mac,
+ address,
+ controller,
+ loaded_callback,
+ )
- if node_type == NodeType.CIRCLE_PLUS:
- return PlugwiseCirclePlus(
- mac,
- address,
- controller,
- loaded_callback,
- )
- if node_type == NodeType.CIRCLE:
- return PlugwiseCircle(
- mac,
- address,
- controller,
- loaded_callback,
- )
- if node_type == NodeType.SWITCH:
- return PlugwiseSwitch(
- mac,
- address,
- controller,
- loaded_callback,
- )
- if node_type == NodeType.SENSE:
- return PlugwiseSense(
- mac,
- address,
- controller,
- loaded_callback,
- )
- if node_type == NodeType.SCAN:
- return PlugwiseScan(
- mac,
- address,
- controller,
- loaded_callback,
- )
- if node_type == NodeType.STEALTH:
- return PlugwiseStealth(
- mac,
- address,
- controller,
- loaded_callback,
- )
- return None📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if node_type == NodeType.CIRCLE_PLUS: | |
| return PlugwiseCirclePlus( | |
| mac, | |
| address, | |
| controller, | |
| loaded_callback, | |
| ) | |
| def _request_features(self, callback=None): | |
| """Request supported features for this node.""" | |
| self.message_sender( | |
| NodeFeaturesRequest(self._mac), | |
| callback, | |
| if node_type == NodeType.CIRCLE: | |
| return PlugwiseCircle( | |
| mac, | |
| address, | |
| controller, | |
| loaded_callback, | |
| ) | |
| def _request_ping(self, callback=None, ignore_sensor=True): | |
| """Ping node.""" | |
| if ignore_sensor or FEATURE_PING["id"] in self._callbacks: | |
| self.message_sender( | |
| NodePingRequest(self._mac), | |
| callback, | |
| ) | |
| def message_for_node(self, message): | |
| """Process received message.""" | |
| if message.mac == self._mac: | |
| if message.timestamp is not None: | |
| _LOGGER.debug( | |
| "Previous update %s of node %s, last message %s", | |
| str(self._last_update), | |
| self.mac, | |
| str(message.timestamp), | |
| ) | |
| self._last_update = message.timestamp | |
| if not self._available: | |
| self.available = True | |
| self._request_info() | |
| if isinstance(message, NodePingResponse): | |
| self._process_ping_response(message) | |
| elif isinstance(message, NodeInfoResponse): | |
| self._process_info_response(message) | |
| elif isinstance(message, NodeFeaturesResponse): | |
| self._process_features_response(message) | |
| elif isinstance(message, NodeJoinAckResponse): | |
| self._process_join_ack_response(message) | |
| else: | |
| self.message_for_circle(message) | |
| self.message_for_sed(message) | |
| else: | |
| _LOGGER.debug( | |
| "Skip message, mac of node (%s) != mac at message (%s)", | |
| message.mac.decode(UTF8_DECODE), | |
| self.mac, | |
| ) | |
| def message_for_circle(self, message): | |
| """Pass messages to PlugwiseCircle class""" | |
| def message_for_sed(self, message): | |
| """Pass messages to NodeSED class""" | |
| def subscribe_callback(self, callback, sensor) -> bool: | |
| """Subscribe callback to execute when state change happens.""" | |
| if sensor in self._features: | |
| if sensor not in self._callbacks: | |
| self._callbacks[sensor] = [] | |
| self._callbacks[sensor].append(callback) | |
| return True | |
| return False | |
| def unsubscribe_callback(self, callback, sensor): | |
| """Unsubscribe callback to execute when state change happens.""" | |
| if sensor in self._callbacks: | |
| self._callbacks[sensor].remove(callback) | |
| def do_callback(self, sensor): | |
| """Execute callbacks registered for specified callback type.""" | |
| if sensor in self._callbacks: | |
| for callback in self._callbacks[sensor]: | |
| try: | |
| callback(None) | |
| # TODO: narrow exception | |
| except Exception as err: # pylint: disable=broad-except | |
| _LOGGER.error( | |
| "Error while executing all callback : %s", | |
| err, | |
| ) | |
| def _process_join_ack_response(self, message): | |
| """Process join acknowledge response message""" | |
| _LOGGER.info( | |
| "Node %s has (re)joined plugwise network", | |
| self.mac, | |
| if node_type == NodeType.SWITCH: | |
| return PlugwiseSwitch( | |
| mac, | |
| address, | |
| controller, | |
| loaded_callback, | |
| ) | |
| def _process_ping_response(self, message): | |
| """Process ping response message.""" | |
| if self._rssi_in != message.rssi_in.value: | |
| self._rssi_in = message.rssi_in.value | |
| self.do_callback(FEATURE_RSSI_IN["id"]) | |
| if self._rssi_out != message.rssi_out.value: | |
| self._rssi_out = message.rssi_out.value | |
| self.do_callback(FEATURE_RSSI_OUT["id"]) | |
| if self._ping != message.ping_ms.value: | |
| self._ping = message.ping_ms.value | |
| self.do_callback(FEATURE_PING["id"]) | |
| def _process_info_response(self, message): | |
| """Process info response message.""" | |
| _LOGGER.debug( | |
| "Response info message for node %s, last log address %s", | |
| self.mac, | |
| str(message.last_logaddr.value), | |
| if node_type == NodeType.SENSE: | |
| return PlugwiseSense( | |
| mac, | |
| address, | |
| controller, | |
| loaded_callback, | |
| ) | |
| if message.relay_state.serialize() == b"01": | |
| if not self._relay_state: | |
| self._relay_state = True | |
| self.do_callback(FEATURE_RELAY["id"]) | |
| else: | |
| if self._relay_state: | |
| self._relay_state = False | |
| self.do_callback(FEATURE_RELAY["id"]) | |
| self._hardware_version = message.hw_ver.value.decode(UTF8_DECODE) | |
| self._firmware_version = message.fw_ver.value | |
| self._node_type = message.node_type.value | |
| if self._last_log_address != message.last_logaddr.value: | |
| self._last_log_address = message.last_logaddr.value | |
| _LOGGER.debug("Node type = %s", self.hardware_model) | |
| if not self._battery_powered: | |
| _LOGGER.debug("Relay state = %s", str(self._relay_state)) | |
| _LOGGER.debug("Hardware version = %s", str(self._hardware_version)) | |
| _LOGGER.debug("Firmware version = %s", str(self._firmware_version)) | |
| def _process_features_response(self, message): | |
| """Process features message.""" | |
| _LOGGER.warning( | |
| "Node %s supports features %s", self.mac, str(message.features.value) | |
| if node_type == NodeType.SCAN: | |
| return PlugwiseScan( | |
| mac, | |
| address, | |
| controller, | |
| loaded_callback, | |
| ) | |
| if node_type == NodeType.STEALTH: | |
| return PlugwiseStealth( | |
| mac, | |
| address, | |
| controller, | |
| loaded_callback, | |
| ) | |
| self._device_features = message.features.value | |
| return None | |
| def get_plugwise_node( | |
| mac: str, | |
| address: int, | |
| controller: StickController, | |
| loaded_callback: Callable[[NodeEvent, str], Awaitable[None]], | |
| node_type: NodeType, | |
| ) -> PlugwiseNode | None: | |
| """Return an initialized plugwise node class based on given the node type.""" | |
| - | |
| - if node_type == NodeType.CIRCLE_PLUS: | |
| - return PlugwiseCirclePlus( | |
| - mac, | |
| - address, | |
| - controller, | |
| - loaded_callback, | |
| - ) | |
| - if node_type == NodeType.CIRCLE: | |
| - return PlugwiseCircle( | |
| - mac, | |
| - address, | |
| - controller, | |
| - loaded_callback, | |
| - ) | |
| - if node_type == NodeType.SWITCH: | |
| - return PlugwiseSwitch( | |
| - mac, | |
| - address, | |
| - controller, | |
| - loaded_callback, | |
| - ) | |
| - if node_type == NodeType.SENSE: | |
| - return PlugwiseSense( | |
| - mac, | |
| - address, | |
| - controller, | |
| - loaded_callback, | |
| - ) | |
| - if node_type == NodeType.SCAN: | |
| - return PlugwiseScan( | |
| - mac, | |
| - address, | |
| - controller, | |
| - loaded_callback, | |
| - ) | |
| - if node_type == NodeType.STEALTH: | |
| - return PlugwiseStealth( | |
| - mac, | |
| - address, | |
| - controller, | |
| - loaded_callback, | |
| - ) | |
| - return None | |
| + | |
| + node_class_map = { | |
| + NodeType.CIRCLE_PLUS: PlugwiseCirclePlus, | |
| + NodeType.CIRCLE: PlugwiseCircle, | |
| + NodeType.SWITCH: PlugwiseSwitch, | |
| + NodeType.SENSE: PlugwiseSense, | |
| + NodeType.SCAN: PlugwiseScan, | |
| + NodeType.STEALTH: PlugwiseStealth, | |
| + } | |
| + | |
| + node_class = node_class_map.get(node_type) | |
| + if node_class is None: | |
| + return None | |
| + | |
| + return node_class( | |
| + mac, | |
| + address, | |
| + controller, | |
| + loaded_callback, | |
| + ) |
🤖 Prompt for AI Agents
In plugwise_usb/nodes/__init__.py between lines 26 and 68, replace the long
if-else chain that returns different Plugwise node instances based on node_type
with a dictionary mapping node_type values to their corresponding classes. Use
this dictionary to look up the class and instantiate it with the given
parameters. This refactor improves maintainability and readability by
centralizing the mapping and simplifying the return logic.
| def subscribe_to_stick_events( | ||
| self, | ||
| stick_event_callback: Callable[[StickEvent], Coroutine[Any, Any, None]], | ||
| events: tuple[StickEvent], | ||
| ) -> Callable[[], None]: | ||
| """Subscribe callback when specified StickEvent occurs. | ||
| Returns the function to be called to unsubscribe later. | ||
| """ | ||
| if self._device_nodes.get(mac): | ||
| _LOGGER.debug( | ||
| "Received node available message for node %s which is already joined.", | ||
| mac, | ||
| ) | ||
| else: | ||
| if self._accept_join_requests: | ||
| # Send accept join request | ||
| _LOGGER.info( | ||
| "Accepting network join request for node with mac %s", | ||
| mac, | ||
| ) | ||
| self.msg_controller.send(NodeAddRequest(node_join_request.mac, True)) | ||
| self._nodes_not_discovered[mac] = (None, None) | ||
| else: | ||
| _LOGGER.debug( | ||
| "New node with mac %s requesting to join Plugwise network, do callback", | ||
| mac, | ||
| ) | ||
| self.do_callback(CB_JOIN_REQUEST, mac) | ||
|
|
||
| def _process_node_remove(self, node_remove_response): | ||
| """Process NodeRemoveResponse message with confirmation | ||
| if node is is removed from the Plugwise network. | ||
| return self._controller.subscribe_to_stick_events( | ||
| stick_event_callback, | ||
| events, | ||
| ) | ||
|
|
There was a problem hiding this comment.
Fix type annotation for events parameter.
The type annotation should use ellipsis to indicate a variable-length tuple, consistent with the node events method and the controller's expected signature.
- events: tuple[StickEvent],
+ events: tuple[StickEvent, ...],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def subscribe_to_stick_events( | |
| self, | |
| stick_event_callback: Callable[[StickEvent], Coroutine[Any, Any, None]], | |
| events: tuple[StickEvent], | |
| ) -> Callable[[], None]: | |
| """Subscribe callback when specified StickEvent occurs. | |
| Returns the function to be called to unsubscribe later. | |
| """ | |
| if self._device_nodes.get(mac): | |
| _LOGGER.debug( | |
| "Received node available message for node %s which is already joined.", | |
| mac, | |
| ) | |
| else: | |
| if self._accept_join_requests: | |
| # Send accept join request | |
| _LOGGER.info( | |
| "Accepting network join request for node with mac %s", | |
| mac, | |
| ) | |
| self.msg_controller.send(NodeAddRequest(node_join_request.mac, True)) | |
| self._nodes_not_discovered[mac] = (None, None) | |
| else: | |
| _LOGGER.debug( | |
| "New node with mac %s requesting to join Plugwise network, do callback", | |
| mac, | |
| ) | |
| self.do_callback(CB_JOIN_REQUEST, mac) | |
| def _process_node_remove(self, node_remove_response): | |
| """Process NodeRemoveResponse message with confirmation | |
| if node is is removed from the Plugwise network. | |
| return self._controller.subscribe_to_stick_events( | |
| stick_event_callback, | |
| events, | |
| ) | |
| def subscribe_to_stick_events( | |
| self, | |
| stick_event_callback: Callable[[StickEvent], Coroutine[Any, Any, None]], | |
| events: tuple[StickEvent, ...], | |
| ) -> Callable[[], None]: | |
| """Subscribe callback when specified StickEvent occurs. | |
| Returns the function to be called to unsubscribe later. | |
| """ | |
| return self._controller.subscribe_to_stick_events( | |
| stick_event_callback, | |
| events, | |
| ) |
🤖 Prompt for AI Agents
In plugwise_usb/__init__.py lines 218 to 231, the type annotation for the events
parameter is currently a fixed-length tuple. Update it to use an ellipsis to
indicate a variable-length tuple, matching the expected signature in the
controller and node events method. Change the annotation from tuple[StickEvent]
to tuple[StickEvent, ...] to allow passing any number of StickEvent items.
| async def unregister_node(self, mac: str) -> None: | ||
| """Remove node to plugwise network.""" | ||
| if self._network is None: | ||
| return | ||
| try: | ||
| await self._network.unregister_node(mac) | ||
| except MessageError as exc: | ||
| raise NodeError(f"Unable to remove Node ({mac}): {exc}") from exc | ||
|
|
There was a problem hiding this comment.
Handle network None case consistently.
The method silently returns when network is None, unlike other methods that either raise exceptions or return False. Consider raising an exception or logging a warning for consistency.
async def unregister_node(self, mac: str) -> None:
"""Remove node to plugwise network."""
if self._network is None:
- return
+ raise StickError("Network not initialized, cannot unregister node")
try:
await self._network.unregister_node(mac)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def unregister_node(self, mac: str) -> None: | |
| """Remove node to plugwise network.""" | |
| if self._network is None: | |
| return | |
| try: | |
| await self._network.unregister_node(mac) | |
| except MessageError as exc: | |
| raise NodeError(f"Unable to remove Node ({mac}): {exc}") from exc | |
| async def unregister_node(self, mac: str) -> None: | |
| """Remove node to plugwise network.""" | |
| if self._network is None: | |
| raise StickError("Network not initialized, cannot unregister node") | |
| try: | |
| await self._network.unregister_node(mac) | |
| except MessageError as exc: | |
| raise NodeError(f"Unable to remove Node ({mac}): {exc}") from exc |
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 363-366: plugwise_usb/init.py#L363-L366
Added lines #L363 - L366 were not covered by tests
🤖 Prompt for AI Agents
In plugwise_usb/__init__.py around lines 359 to 367, the unregister_node method
silently returns when self._network is None, which is inconsistent with other
methods that raise exceptions or return False in this case. Modify the method to
raise a specific exception or log a warning when self._network is None to ensure
consistent handling of the missing network scenario.
| @raise_calibration_missing | ||
| def _calc_watts(self, pulses: int, seconds: int, nano_offset: int) -> float | None: | ||
| """Calculate watts based on energy usages.""" | ||
| if self._calibration is None: | ||
| return None | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant calibration check.
The @raise_calibration_missing decorator already ensures calibration is not None, making this check redundant and the return statement unreachable.
@raise_calibration_missing
def _calc_watts(self, pulses: int, seconds: int, nano_offset: int) -> float | None:
"""Calculate watts based on energy usages."""
- if self._calibration is None:
- return None
-
pulses_per_s = self._correct_power_pulses(pulses, nano_offset) / float(seconds)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @raise_calibration_missing | |
| def _calc_watts(self, pulses: int, seconds: int, nano_offset: int) -> float | None: | |
| """Calculate watts based on energy usages.""" | |
| if self._calibration is None: | |
| return None | |
| @raise_calibration_missing | |
| def _calc_watts(self, pulses: int, seconds: int, nano_offset: int) -> float | None: | |
| """Calculate watts based on energy usages.""" | |
| pulses_per_s = self._correct_power_pulses(pulses, nano_offset) / float(seconds) | |
| # ... rest of the calculation logic ... |
🤖 Prompt for AI Agents
In plugwise_usb/nodes/circle.py around lines 1032 to 1037, remove the redundant
check for self._calibration being None inside the _calc_watts method, as the
@raise_calibration_missing decorator already guarantees calibration is present.
Delete the if statement and the return None line to clean up unreachable code.
Bump to b3 for testing
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
CHANGELOG.md (4)
17-20: Maintain imperative style in list items
Change “Correcting messageflow to HA” to “Correct message flow to HA” to align with the imperative tone used elsewhere in the changelog.
33-35: Improve header and capitalization
- Add a space between version and parentheses:
## v0.31.4 (a0).- Capitalize “Python” in “Re-add python 3.12 checks and compatibility.”
37-39: Use consistent imperative bugfix phrase
Change “Bugfix midnight rollover for circles without power usage registered during first hour(s)” to an imperative form, e.g.- Fix midnight rollover bug for circles without power usage during the first hour.
42-48: Ensure consistent bullet punctuation
Bullets under## v0.31.2end with periods, but the entry in## v0.31.3does not. Adopt a uniform style (either all with or all without trailing periods) across all release notes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
CHANGELOG.md
25-25: Lists should be surrounded by blank lines
null
(MD032, blanks-around-lists)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check commit
- GitHub Check: Check commit
🔇 Additional comments (1)
CHANGELOG.md (1)
3-16: Approve v0.40.0 changelog entries
The enhancements and bug fixes listed under## v0.40.0are clear, well‐structured, and already follow markdownlint’s MD032 rule with blank lines surrounding the list.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
CHANGELOG.md (4)
17-20: Typo: use ‘message flow’ and imperative voice
Consider changing the bullet to an imperative form and split “messageflow” into two words for clarity:- Correcting messageflow to HA + Correct message flow to HA
21-23: Grammar improvement: add missing articles
Refine for clarity by inserting “the” and “an”:- Full rewrite of library into async version (@brefra). + Full rewrite of the library into an async version (@brefra).🧰 Tools
🪛 LanguageTool
[uncategorized] ~23-~23: You might be missing the article “the” here.
Context: ...to HA ## v0.40.0 (a4) Full rewrite of library into async version (@brefra). Main list...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~23-~23: You might be missing the article “an” here.
Context: ...40.0 (a4) Full rewrite of library into async version (@brefra). Main list of changes...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
24-32: Enhance formatting of sub-section and nested list
The “Main list of changes:” line would benefit from being styled as a sub-heading, with its subsequent items indented to reflect nesting. For example:- Main list of changes: + ### Main list of changes: - Full async and typed + - Full async and typed - Improved protocol handling + - Improved protocol handling - Support for local caching of collected data to improve startup and device detection + - Support for local caching of collected data to improve startup and device detection [...]
40-40: Nitpick: refine bugfix bullet
For consistency and readability, add a colon after “Bugfix” and clarify wording:- Bugfix midnight rollover for circles without power usage registered during first hour(s) + Bugfix: Fix midnight rollover for circles missing power usage during the first hour(s).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~23-~23: You might be missing the article “the” here.
Context: ...to HA ## v0.40.0 (a4) Full rewrite of library into async version (@brefra). Main list...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~23-~23: You might be missing the article “an” here.
Context: ...40.0 (a4) Full rewrite of library into async version (@brefra). Main list of changes...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check commit
- GitHub Check: Check commit
🔇 Additional comments (3)
CHANGELOG.md (3)
3-3: Add version header for v0.40.0
The new## v0.40.0section correctly introduces the list of changes for this release.
5-8: Approve auto-joining feature details
The bullet point and its nested items clearly explain the improvements for auto-joining behavior. The nested list is properly indented and informative.
9-15: Approve core improvements and fixes
These top-level bullets succinctly describe async task handling, cache limits, production support, node info collection, bugfixes, and GitHub-related updates. All items are clear and consistent.
ArnoutD
left a comment
ArnoutD
left a comment
There was a problem hiding this comment.
Good for main, this branch can run for as long as you want... current main requires daily restarts
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
17-20: Typo: “messageflow” → “message flow”
Please add a space to correct the phrase for clarity:- - Correcting messageflow to HA (@ArnoutD) + - Correcting message flow to HA (@ArnoutD)
21-23: Add missing article and bullet marker
The “Full rewrite…” line should be a bullet and include the articles “A” and “an” for grammatical correctness. For example:## v0.40.0 (a4) -- Full rewrite of library into async version (@brefra). +- A full rewrite of the library into an async version (@brefra).🧰 Tools
🪛 LanguageTool
[uncategorized] ~23-~23: You might be missing the article “the” here.
Context: ...outD) ## v0.40.0 (a4) Full rewrite of library into async version (@brefra). Main list...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~23-~23: You might be missing the article “an” here.
Context: ...40.0 (a4) Full rewrite of library into async version (@brefra). Main list of changes...(AI_EN_LECTOR_MISSING_DETERMINER_AN)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~23-~23: You might be missing the article “the” here.
Context: ...outD) ## v0.40.0 (a4) Full rewrite of library into async version (@brefra). Main list...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~23-~23: You might be missing the article “an” here.
Context: ...40.0 (a4) Full rewrite of library into async version (@brefra). Main list of changes...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Check commit
- GitHub Check: Check commit
🔇 Additional comments (2)
CHANGELOG.md (2)
3-15: Changelog entry for v0.40.0 looks good
All main bullet points are clear and correctly formatted, and the nested list under “Make auto-joining work” is properly indented.
40-40: v0.31.3 bugfix description is fine
The midnight rollover fix for circles is clearly described and formatted consistently with other entries.
|
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores
Documentation
Tests
Revert