Alternative scan update #325
Conversation
WalkthroughMotion sensitivity moved from integers to the MotionSensitivity enum across API, messages, node logic and cache; Scan light calibration was changed to a scheduled flow executed during awake tasks (public call now schedules); tests updated to use the enum; project version bumped to 0.44.12a2. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant PlugwiseScan
participant AwakeLoop as "Awake Tasks Loop"
participant Device as "Scan Device"
Caller->>PlugwiseScan: scan_calibrate_light() (schedule)
Note right of PlugwiseScan #f9f0c1: set _scan_calibrate_light_scheduled = True
PlugwiseScan-->>Caller: return None
AwakeLoop->>PlugwiseScan: _run_awake_tasks()
alt scheduled
PlugwiseScan->>PlugwiseScan: _scan_calibrate_light()
PlugwiseScan->>Device: ScanLightCalibrateRequest
alt Accepted
Device-->>PlugwiseScan: Accepted
Note right of PlugwiseScan #cfe8d4: clear schedule flag
else Denied
Device-->>PlugwiseScan: Denied
Note right of PlugwiseScan #fce8d6: keep/clear flag per result
else Timeout
Device--x PlugwiseScan: no response (NodeTimeout)
end
else not scheduled
Note right of AwakeLoop #eef3ff: continue other tasks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
==========================================
- Coverage 80.57% 80.45% -0.12%
==========================================
Files 36 36
Lines 7566 7574 +8
==========================================
- Hits 6096 6094 -2
- Misses 1470 1480 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugwise_usb/nodes/scan.py (2)
201-209: Make cache loader robust to both legacy names and numeric values_sensitivity_level_from_cache assumes uppercase enum names (e.g., "MEDIUM"). With the new plan to store numbers, this will KeyError on "30"/"20"/"255". Support both:
- def _sensitivity_level_from_cache(self) -> MotionSensitivity | None: - """Load sensitivity level from cache.""" - if ( - sensitivity_level := self._get_cache( - CACHE_SCAN_CONFIG_SENSITIVITY - ) # returns level in string CAPITALS - ) is not None: - return MotionSensitivity[sensitivity_level] - return None + def _sensitivity_level_from_cache(self) -> MotionSensitivity | None: + """Load sensitivity level from cache (accepts legacy names and numeric values).""" + if (raw := self._get_cache(CACHE_SCAN_CONFIG_SENSITIVITY)) is None: + return None + # Legacy: "HIGH" / "MEDIUM" / "OFF" + try: + return MotionSensitivity[raw] + except KeyError: + pass + # New: "20" / "30" / "255" + try: + return MotionSensitivity(int(raw)) + except (ValueError, KeyError): + _LOGGER.warning( + "Invalid cached sensitivity '%s' for %s, defaulting to %s", + raw, + self.name, + DEFAULT_SENSITIVITY.name, + ) + return DEFAULT_SENSITIVITY
329-346: Operate with enums internally; convert to value only at the boundaryGiven MotionConfig.sensitivity_level should be MotionSensitivity, adapt comparisons, logging, and storage accordingly:
- async def set_motion_sensitivity_level(self, level: MotionSensitivity) -> bool: + async def set_motion_sensitivity_level(self, level: MotionSensitivity) -> bool: """Configure the motion sensitivity level.""" _LOGGER.debug( "set_motion_sensitivity_level | Device %s | %s -> %s", self.name, self._motion_config.sensitivity_level, - level.value, + level, ) - if self._motion_config.sensitivity_level == level.value: + if self._motion_config.sensitivity_level == level: return False self._motion_config = replace( self._motion_config, - sensitivity_level=level.value, + sensitivity_level=level, dirty=True, ) await self._scan_configure_update() return True
🧹 Nitpick comments (4)
plugwise_usb/api.py (1)
681-687: Nit: docstring grammar“Request to calibration …” → “Request to calibrate …” if you decide to keep the old semantics.
tests/test_usb.py (1)
2245-2253: Enum-based sensitivity tests LGTM; consider adding cache-backward-compat coverageThese checks are fine. Given the code aims to store numeric sensitivity in cache, add a parametrized test to verify both legacy (“MEDIUM”, “HIGH”, “OFF”) and numeric (“30”, “20”, “255”) cache values are accepted and mapped to the same enum.
I can draft an additional test function that monkeypatches _get_cache to return each variant and asserts the resulting MotionConfig.sensitivity_level is MotionSensitivity.MEDIUM/HIGH/OFF.
plugwise_usb/nodes/scan.py (2)
429-431: Awake task: handle calibrate exceptions to avoid aborting the awake cycle_node timeout during _scan_calibrate_light will raise NodeTimeout. Consider catching/logging to keep the awake-task pipeline resilient:
- if self._scan_calibrate_light_scheduled: - await self._scan_calibrate_light() + if self._scan_calibrate_light_scheduled: + try: + await self._scan_calibrate_light() + except NodeTimeout as exc: + _LOGGER.warning("Light calibration timed out for %s: %s", self.name, exc)If you prefer to retry automatically, we can keep the scheduled flag set on timeout to retry on the next awake cycle (current behavior).
505-506: Re-try policy on failure: clarify behaviorYou only clear the scheduled flag on SCAN_LIGHT_CALIBRATION_ACCEPTED. On failure, the flag stays set, so the next awake cycle will retry. If that’s intentional, mention it in the docstring of scan_calibrate_light() or add a short comment here for maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
plugwise_usb/api.py(2 hunks)plugwise_usb/nodes/scan.py(8 hunks)pyproject.toml(1 hunks)tests/test_usb.py(4 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T17:50:29.188Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.188Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.
Applied to files:
plugwise_usb/nodes/scan.py
🧬 Code graph analysis (3)
plugwise_usb/api.py (2)
plugwise_usb/nodes/scan.py (4)
daylight_mode(234-238)reset_timer(270-274)sensitivity_level(277-281)scan_calibrate_light(493-495)plugwise_usb/nodes/node.py (1)
scan_calibrate_light(369-371)
tests/test_usb.py (3)
plugwise_usb/nodes/scan.py (3)
set_motion_sensitivity_level(329-345)motion_config(260-267)dirty(229-231)plugwise_usb/api.py (3)
set_motion_sensitivity_level(662-679)MotionSensitivity(22-27)motion_config(498-508)plugwise_usb/nodes/node.py (2)
set_motion_sensitivity_level(790-796)motion_config(249-255)
plugwise_usb/nodes/scan.py (4)
plugwise_usb/api.py (4)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-679)name(288-289)scan_calibrate_light(681-686)plugwise_usb/nodes/node.py (4)
set_motion_sensitivity_level(790-796)name(218-222)_set_cache(689-703)scan_calibrate_light(369-371)plugwise_usb/messages/requests.py (17)
ScanLightCalibrateRequest(1407-1425)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(504-513)send(534-545)send(558-567)send(580-589)send(613-622)send(659-668)send(711-720)send(767-776)send(800-809)send(840-849)send(872-881)plugwise_usb/messages/responses.py (1)
NodeAckResponseType(75-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (3)
pyproject.toml (1)
7-7: Version bump looks goodNo issues spotted with the version update to 0.44.12a2. Please ensure a matching changelog entry and tag are created when moving this PR out of draft.
Would you like me to draft a brief CHANGELOG section for 0.44.12a2?
tests/test_usb.py (1)
1837-1838: Enum usage in tests looks goodSwitching to pw_api.MotionSensitivity in these tests is consistent with the updated setter signature and increases readability.
Also applies to: 1868-1869, 1895-1896
plugwise_usb/nodes/scan.py (1)
89-89: Scheduling flag initialization: good addition_init sets _scan_calibrate_light_scheduled = False. This integrates cleanly with the awake-task flow.
| sensitivity_level: MotionSensitivity | None: Motion sensitivity level. | ||
| dirty: bool: Settings changed, device update pending | ||
| """ | ||
|
|
||
| daylight_mode: bool | None = None | ||
| reset_timer: int | None = None | ||
| sensitivity_level: int | None = None | ||
| sensitivity_level: MotionSensitivity | None = None | ||
| dirty: bool = False |
There was a problem hiding this comment.
Type contract drift: MotionConfig.sensitivity_level vs implementation
You’ve changed MotionConfig.sensitivity_level to MotionSensitivity | None, which is a good direction. However, PlugwiseScan currently stores and exposes an int for sensitivity_level (see nodes/scan.py line 168 and property at line 276), leading to a mismatch with this public type. This will trip strict type-checking and confuse downstream consumers.
Recommend making the node consistently use MotionSensitivity end-to-end (store the enum in _motion_config, property returns MotionSensitivity) and only convert to the numeric value at the request boundary and when writing to cache. See suggested fixes in scan.py comments.
🤖 Prompt for AI Agents
In plugwise_usb/api.py around lines 236 to 244, MotionConfig.sensitivity_level
is now typed as MotionSensitivity | None but the node implementation stores and
exposes an int; update the node to store the MotionSensitivity enum in its
internal _motion_config and make the public property return MotionSensitivity
(or None) instead of int; when persisting to cache or preparing network requests
convert the enum to its numeric value (e.g., enum.value) and when reading from
cache or responses convert numeric values back into MotionSensitivity using
MotionSensitivity(value) to maintain a consistent enum-based contract
end-to-end.
| async def scan_calibrate_light(self) -> bool: | ||
| """Request to calibration light sensitivity of Scan device. | ||
| Description: | ||
| Request to calibration light sensitivity of Scan device. | ||
| """ | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
API contract mismatch: scan_calibrate_light return type
In this Protocol, scan_calibrate_light() returns bool with semantics “True if successful.” In PlugwiseScan (nodes/scan.py lines 493-496), the method now schedules calibration and returns None, with execution moved to _scan_calibrate_light() during awake tasks.
This is a breaking contract between the public API (Protocol and base class) and the concrete implementation. Please align the API to the new scheduling model. Suggested change: make the public method “schedule-only” and return None consistently across API, base class, and implementations; document that actual success/failure will be surfaced via logs/events.
Proposed diff for this file:
- async def scan_calibrate_light(self) -> bool:
- """Request to calibration light sensitivity of Scan device.
-
- Description:
- Request to calibration light sensitivity of Scan device.
- """
+ async def scan_calibrate_light(self) -> None:
+ """Schedule light sensitivity calibration of a Scan device.
+
+ Description:
+ Schedules a light sensitivity calibration request to be performed
+ when the device is awake for maintenance. No return value.
+ """Also update nodes/node.py to match (see scan.py review for a code block).
📝 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 scan_calibrate_light(self) -> bool: | |
| """Request to calibration light sensitivity of Scan device. | |
| Description: | |
| Request to calibration light sensitivity of Scan device. | |
| """ | |
| async def scan_calibrate_light(self) -> None: | |
| """Schedule light sensitivity calibration of a Scan device. | |
| Description: | |
| Schedules a light sensitivity calibration request to be performed | |
| when the device is awake for maintenance. No return value. | |
| """ |
| async def scan_calibrate_light(self) -> None: | ||
| """Schedule light sensitivity calibration of Scan device.""" | ||
| self._scan_calibrate_light_scheduled = True | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Public API semantics changed; propagate to base class and Protocol
This method now schedules calibration and returns None. Please update:
- PlugwiseNode Protocol (api.py) to -> None with a scheduling docstring (see api.py comment).
- PlugwiseBaseNode (nodes/node.py) signature and NotImplementedError docstring accordingly.
You can keep _scan_calibrate_light() as the boolean-returning executor.
🤖 Prompt for AI Agents
In plugwise_usb/api.py and plugwise_usb/nodes/node.py (update the PlugwiseNode
Protocol and PlugwiseBaseNode method definitions respectively), change the
signature of scan_calibrate_light to async def scan_calibrate_light(self) ->
None and update the docstring to state this method schedules light sensitivity
calibration (does not run it) and returns None; in PlugwiseBaseNode adjust the
NotImplementedError text to instruct implementers to schedule calibration and
return None (keep the existing _scan_calibrate_light executor that returns a
boolean unchanged).
…ect contains None
bouwew
force-pushed
the
mdi_scan_bouwew
branch
from
2987192 to
a78207d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
plugwise_usb/nodes/scan.py (3)
201-210: Cache reader will KeyError on numeric writes; accept both numeric and legacy enum-name formsAs written, if the cache contains "30" (written by current _scan_configure_update),
MotionSensitivity["30"]raises KeyError during load. Make the reader dual-format and resilient.- def _sensitivity_level_from_cache(self) -> MotionSensitivity | None: - """Load sensitivity level from cache.""" - if ( - sensitivity_level := self._get_cache( - CACHE_SCAN_CONFIG_SENSITIVITY - ) # returns level in string CAPITALS - ) is not None: - return MotionSensitivity[sensitivity_level] - return None + def _sensitivity_level_from_cache(self) -> MotionSensitivity | None: + """Load sensitivity level from cache. + + Accepts either: + - numeric string/int (e.g. '20', '30', '255'), or + - legacy enum name (e.g. 'HIGH', 'MEDIUM', 'OFF'). + """ + raw = self._get_cache(CACHE_SCAN_CONFIG_SENSITIVITY) + if raw is None: + return None + # Prefer numeric interpretation first + try: + if isinstance(raw, int) or (isinstance(raw, str) and raw.isdigit()): + return MotionSensitivity(int(raw)) + except (ValueError, KeyError): + pass + # Fallback to enum name + if isinstance(raw, str): + try: + return MotionSensitivity[raw] + except KeyError: + _LOGGER.warning( + "Unsupported cached sensitivity '%s' for %s, defaulting to %s", + raw, + self.name, + DEFAULT_SENSITIVITY.name, + ) + return DEFAULT_SENSITIVITY + return None
277-282: Return MotionSensitivity from the property (aligns with public API)Keep the enum internally and expose it. Convert to int only at external boundaries (requests, cache). This also makes MotionConfig() constructed in
motion_configtype-correct.- def sensitivity_level(self) -> int: - """Sensitivity level of motion sensor.""" - if self._motion_config.sensitivity_level is not None: - return self._motion_config.sensitivity_level - return DEFAULT_SENSITIVITY.value + def sensitivity_level(self) -> MotionSensitivity: + """Sensitivity level of motion sensor.""" + if self._motion_config.sensitivity_level is not None: + return self._motion_config.sensitivity_level + return DEFAULT_SENSITIVITY
329-345: Accept and store the enum directly; fix comparison and loggingStore
level(enum) instead of its numeric value and compare against the enum to avoid accidental int/enum drift. Keep conversion at the request/cache boundaries.async def set_motion_sensitivity_level(self, level: MotionSensitivity) -> bool: """Configure the motion sensitivity level.""" _LOGGER.debug( "set_motion_sensitivity_level | Device %s | %s -> %s", self.name, self._motion_config.sensitivity_level, - level.value, + level, ) - if self._motion_config.sensitivity_level == level.value: + if self._motion_config.sensitivity_level == level: return False self._motion_config = replace( self._motion_config, - sensitivity_level=level.value, + sensitivity_level=level, dirty=True, ) await self._scan_configure_update() return True
♻️ Duplicate comments (4)
plugwise_usb/nodes/scan.py (4)
445-454: Convert enum to numeric only when building the requestIf you adopt the enum-internal model, pass
.valueat the boundary. Keeps internal types consistent and the wire format numeric.request = ScanConfigureRequest( self._send, self._mac_in_bytes, - self.reset_timer, - self.sensitivity_level, - self.daylight_mode, + self.reset_timer, + self.sensitivity_level.value, + self.daylight_mode, )
493-496: Public API semantics changed; align Protocol/base signatures and docsThis method now “schedules” calibration and returns None. Update the PlugwiseNode Protocol and PlugwiseBaseNode accordingly so downstream users don’t expect a bool.
Run to spot signature mismatches:
#!/bin/bash # Find public scan_calibrate_light declarations/usages that still expect a bool rg -nP -C2 --type=py '\bscan_calibrate_light\s*\('I can help prepare coordinated changes in api.py and nodes/node.py if you want.
165-170: Store MotionSensitivity in MotionConfig (not its int value)The MotionConfig type (in api.py) now expects MotionSensitivity | None. Storing
level.valuehere makes the public surface inconsistent and cascades into cache/write mismatches later.Apply:
self._motion_config = MotionConfig( daylight_mode=daylight_mode, reset_timer=reset_timer, - sensitivity_level=sensitivity_level.value, + sensitivity_level=sensitivity_level, dirty=dirty, )
476-485: Cache: persist numeric sensitivity (stringified) for stability and dual-read supportWriting the enum or a bare int leads to fragile reloads. Persist a numeric string; the updated reader accepts both numeric and legacy names.
- self._set_cache(CACHE_SCAN_CONFIG_RESET_TIMER, str(self.reset_timer)) - self._set_cache( - CACHE_SCAN_CONFIG_SENSITIVITY, - self._motion_config.sensitivity_level, - ) - self._set_cache(CACHE_SCAN_CONFIG_DAYLIGHT_MODE, str(self.daylight_mode)) - self._set_cache(CACHE_SCAN_CONFIG_DIRTY, str(self.dirty)) + self._set_cache(CACHE_SCAN_CONFIG_RESET_TIMER, str(self.reset_timer)) + self._set_cache( + CACHE_SCAN_CONFIG_SENSITIVITY, + str((self._motion_config.sensitivity_level or DEFAULT_SENSITIVITY).value), + ) + self._set_cache(CACHE_SCAN_CONFIG_DAYLIGHT_MODE, str(self.daylight_mode)) + self._set_cache(CACHE_SCAN_CONFIG_DIRTY, str(self.dirty))
🧹 Nitpick comments (1)
plugwise_usb/nodes/scan.py (1)
497-512: Optional: log negative ACK for calibrationCurrently a non-accepted ACK returns False silently. A warning with the ack type helps diagnose why a device refused calibration.
if (response := await request.send()) is not None: if ( response.node_ack_type == NodeAckResponseType.SCAN_LIGHT_CALIBRATION_ACCEPTED ): self._scan_calibrate_light_scheduled = False return True + _LOGGER.warning( + "Light calibration not accepted for %s (ack: %s)", + self.name, + response.node_ack_type, + ) return False
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
plugwise_usb/api.py(2 hunks)plugwise_usb/nodes/scan.py(9 hunks)pyproject.toml(1 hunks)tests/test_usb.py(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_usb.py
- plugwise_usb/api.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T17:50:29.188Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.188Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.
Applied to files:
plugwise_usb/nodes/scan.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/scan.py (4)
plugwise_usb/api.py (4)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-679)name(288-289)scan_calibrate_light(681-686)plugwise_usb/nodes/node.py (4)
set_motion_sensitivity_level(790-796)name(218-222)_set_cache(689-703)scan_calibrate_light(369-371)plugwise_usb/messages/requests.py (17)
ScanLightCalibrateRequest(1407-1425)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(504-513)send(534-545)send(558-567)send(580-589)send(613-622)send(659-668)send(711-720)send(767-776)send(800-809)send(840-849)send(872-881)plugwise_usb/messages/responses.py (2)
node_ack_type(918-920)NodeAckResponseType(75-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (2)
plugwise_usb/nodes/scan.py (2)
87-94: Good addition: explicit scheduling flag for light calibrationThe dedicated
_scan_calibrate_light_scheduledflag makes the awake-task flow clear and avoids immediate I/O from public API calls.
429-431: Nice: calibration runs during awake tasksTriggering
_scan_calibrate_light()from_run_awake_tasks()when scheduled matches the rest of the SED scheduling model.
|
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise_usb/nodes/scan.py (1)
201-209: Backward-compat cache read: handle legacy numeric values and invalid data_sensitivity_level_from_cache assumes enum names (e.g., "MEDIUM"). Existing caches likely contain numeric strings ("20", "30", "255"). MotionSensitivity["30"] raises KeyError -> load failure. Parse both forms and be robust to garbage with a warning.
Apply this diff:
- def _sensitivity_level_from_cache(self) -> MotionSensitivity | None: - """Load sensitivity level from cache.""" - if ( - sensitivity_level := self._get_cache( - CACHE_SCAN_CONFIG_SENSITIVITY - ) # returns level in string CAPITALS - ) is not None: - return MotionSensitivity[sensitivity_level] - return None + def _sensitivity_level_from_cache(self) -> MotionSensitivity | None: + """Load sensitivity level from cache. + + Accepts both enum name (e.g., 'MEDIUM') and legacy numeric string (e.g., '30').""" + raw = self._get_cache(CACHE_SCAN_CONFIG_SENSITIVITY) + if raw is None: + return None + # Try enum name + try: + return MotionSensitivity[raw] + except KeyError: + pass + # Try numeric + try: + return MotionSensitivity(int(raw)) + except (ValueError, KeyError): + _LOGGER.warning( + "Unexpected cached sensitivity '%s' for %s; falling back to default", + raw, + self.mac, + ) + return NonePlease add/adjust a unit test to cover both cache formats.
♻️ Duplicate comments (2)
plugwise_usb/nodes/scan.py (2)
478-485: Cache write uses enum name; consider storing numeric for stability (while keeping dual-read)Writing the enum name is readable, but names can be refactored; numeric values are the protocol constants. With the dual-read fix above, you can safely keep names, but consider persisting the numeric value for long-term stability.
Optional tweak:
- self._set_cache( - CACHE_SCAN_CONFIG_SENSITIVITY, - self._motion_config.sensitivity_level.name, - ) + self._set_cache( + CACHE_SCAN_CONFIG_SENSITIVITY, + str(self._motion_config.sensitivity_level.value), + )If you prefer names, keep as-is, since the reader will now accept both.
493-506: Public API behavior changed (scheduling, returns None) but Protocol/base still declare -> boolscan_calibrate_light now schedules and returns None, but PlugwiseNode Protocol in api.py still advertises -> bool (see provided snippet), and PlugwiseBaseNode likely mirrors that. This will confuse callers and type checkers.
Proposed follow-ups (outside this file):
- In plugwise_usb/api.py Protocol: change signature to -> None and update docstring to state it schedules calibration.
- In plugwise_usb/nodes/node.py base class: update abstract/NotImplementedError method signature and docstring accordingly.
- Ensure any callers/tests expecting a bool are adapted.
Example updates (outside this file):
# plugwise_usb/api.py class PlugwiseNode(Protocol): async def scan_calibrate_light(self) -> None: """Schedule light sensitivity calibration of the Scan device (returns immediately)."""# plugwise_usb/nodes/node.py class PlugwiseBaseNode: async def scan_calibrate_light(self) -> None: """Schedule light sensitivity calibration; implementers should enqueue and return None.""" raise NotImplementedError()Please also add a brief release note about the behavioral change.
Run this to spot mismatches:
#!/bin/bash # Find all scan_calibrate_light definitions and references rg -nP -C2 '\bscan_calibrate_light\s*\(' # Inspect cache key usage/format across codebase rg -nP -C2 'CACHE_SCAN_CONFIG_SENSITIVITY'
🧹 Nitpick comments (3)
plugwise_usb/messages/requests.py (2)
12-12: Consider dependency layering: messages now import API enumBringing MotionSensitivity in from ..api couples low-level message building to the high-level API module. If api.py ever depends on messages, you risk circular imports. Consider relocating MotionSensitivity to a neutral types/enums module (e.g., plugwise_usb/types.py) consumed by both api and messages.
1375-1382: Accept both enum and legacy int; validate early and fail fastThe signature now requires MotionSensitivity and you convert via sensitivity.value. That’s good for type safety, but it breaks any legacy call sites/tests that still pass an int. To be resilient, accept MotionSensitivity | int and normalize to the enum with a clear error on invalid values.
Apply this diff:
- def __init__( + def __init__( self, send_fn: Callable[[PlugwiseRequest, bool], Awaitable[PlugwiseResponse | None]], mac: bytes, reset_timer: int, - sensitivity: MotionSensitivity, + sensitivity: MotionSensitivity | int, light: bool, ): """Initialize ScanConfigureRequest message object.""" super().__init__(send_fn, mac) reset_timer_value = Int(reset_timer, length=2) # Sensitivity: HIGH(0x14), MEDIUM(0x1E), OFF(0xFF) - sensitivity_value = Int(sensitivity.value, length=2) + if not isinstance(sensitivity, MotionSensitivity): + try: + sensitivity = MotionSensitivity(int(sensitivity)) + except (ValueError, TypeError) as exc: + raise MessageError(f"Invalid Scan sensitivity: {sensitivity!r}") from exc + sensitivity_value = Int(sensitivity.value, length=2) light_temp = 1 if light else 0Also consider adding a short note to the class docstring that the parameter accepts MotionSensitivity (preferred) and legacy ints 20/30/255.
Also applies to: 1386-1394
plugwise_usb/nodes/scan.py (1)
329-336: Guard logging against None to avoid AttributeError on first-useIf _motion_config.sensitivity_level is None (e.g., very early before cache/defaults applied), .name access will raise. Minor, but easy to harden logging.
Apply this diff:
_LOGGER.debug( "set_motion_sensitivity_level | Device %s | %s -> %s", self.name, - self._motion_config.sensitivity_level.name, - level.name, + getattr(self._motion_config.sensitivity_level, "name", "None"), + level.name, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
plugwise_usb/messages/requests.py(2 hunks)plugwise_usb/nodes/scan.py(8 hunks)tests/test_usb.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_usb.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T17:50:29.188Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.188Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.
Applied to files:
plugwise_usb/nodes/scan.py
🧬 Code graph analysis (2)
plugwise_usb/messages/requests.py (2)
plugwise_usb/api.py (1)
MotionSensitivity(22-27)plugwise_usb/messages/properties.py (14)
Int(103-129)value(76-80)value(96-100)value(125-129)value(159-163)value(187-191)value(207-211)value(251-255)value(276-280)value(301-305)value(330-334)value(359-363)value(380-384)value(403-407)
plugwise_usb/nodes/scan.py (4)
plugwise_usb/api.py (4)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-679)name(288-289)scan_calibrate_light(681-686)plugwise_usb/nodes/node.py (4)
set_motion_sensitivity_level(790-796)name(218-222)_set_cache(689-703)scan_calibrate_light(369-371)plugwise_usb/messages/requests.py (17)
ScanLightCalibrateRequest(1408-1426)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)plugwise_usb/messages/responses.py (2)
node_ack_type(918-920)NodeAckResponseType(75-84)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Check commit
🔇 Additional comments (4)
plugwise_usb/nodes/scan.py (4)
87-92: Scheduling flag for light calibration: good additionThe dedicated _scan_calibrate_light_scheduled flag cleanly separates scheduling from execution. No issues spotted.
277-282: Property now returns MotionSensitivity: consistent with APIReturning the enum aligns with the updated API and reduces magic numbers in callers. LGTM.
429-431: Awake task executes scheduled calibration: good integrationExecuting the calibration opportunistically during awake time is sensible for SED nodes. LGTM.
451-454: Passing enum into ScanConfigureRequest matches new request APIUsing self.sensitivity_level (enum) is now correct given the request converts to .value.
|
Closing because #323 will be merged instead. |
Summary by CodeRabbit