store MotionSensitivity enum which makes communication to HA easier #323
Conversation
WalkthroughMotion sensitivity moved from numeric to a MotionSensitivity enum and MotionConfig gained a scan_calibrate_light() API. Scan node adds a scheduled light-calibration flow, updates sensitivity handling, cache serialization, and messaging. Tests and package version updated to reflect enum usage and changelog entry added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant App as Client Code
participant Scan as PlugwiseScan
participant Dev as Device
rect rgb(245,248,255)
note over User,App: Request scheduling
User->>App: scan_calibrate_light()
App->>Scan: scan_calibrate_light()
Scan->>Scan: set _scan_calibrate_light_scheduled = True
Scan-->>App: return True (if newly scheduled)
end
rect rgb(240,255,245)
note over Scan,Dev: Awake cycle processes scheduled calibration
Scan->>Scan: awake handler runs
alt calibration scheduled
Scan->>Dev: ScanLightCalibrateRequest
alt ACK accepted
Dev-->>Scan: ACK
Scan->>Scan: clear _scan_calibrate_light_scheduled
else no/timeout
Dev--x Scan: no response
Scan->>Scan: log warning
end
else no calibration pending
Scan->>Scan: normal awake flow
end
end
sequenceDiagram
autonumber
actor User
participant App as Client Code
participant Scan as PlugwiseScan
participant Cache as Cache/Store
participant Dev as Device
rect rgb(255,248,240)
note over User,Scan: Set motion sensitivity using enum
User->>App: set MotionSensitivity.HIGH
App->>Scan: set_motion_sensitivity_level(level: MotionSensitivity)
alt level unchanged
Scan-->>App: return False
else level changed
Scan->>Scan: update MotionConfig.sensitivity_level (enum)
Scan->>Dev: scan_configure(reset_timer, level.value, daylight_mode)
Scan->>Cache: _scan_configure_update(save enum name, reset_timer, daylight_mode, dirty)
Scan-->>App: return True
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 #323 +/- ##
==========================================
- Coverage 80.53% 80.46% -0.08%
==========================================
Files 36 36
Lines 7559 7571 +12
==========================================
+ Hits 6088 6092 +4
- Misses 1471 1479 +8 ☔ 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: 0
🔭 Outside diff range comments (1)
plugwise_usb/nodes/scan.py (1)
201-210: Backwards-incompatible cache parsing — legacy numeric entries will raise KeyErrorMotionSensitivity[sensitivity_level] expects an enum name. Older caches likely stored numeric values ("20", "30", "255"). This will raise KeyError and break _load_from_cache. Handle both enum names (case-insensitive) and numeric strings.
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.""" + if (raw := self._get_cache(CACHE_SCAN_CONFIG_SENSITIVITY)) is None: + return None + raw_str = str(raw).strip() + # Try by enum name (case-insensitive) + try: + return MotionSensitivity[raw_str.upper()] + except KeyError: + # Fallback: try numeric value (e.g., "20", "30", "255") + try: + return MotionSensitivity(int(raw_str)) + except ValueError: + _LOGGER.debug( + "Ignoring invalid cached sensitivity_level %r for %s", + raw, + self._mac_in_str, + ) + return None
🧹 Nitpick comments (5)
plugwise_usb/api.py (2)
236-245: Docstring says MotionSensitivity, field is still int — align the dataclass to the enum to avoid .value/.name pitfallsYou’re now using .value/.name on sensitivity_level downstream (scan.py), so keeping this annotated as int | None is misleading for type checkers and risks attribute errors if None ever sneaks in. Recommend changing the field type to MotionSensitivity | None.
Apply this diff:
class MotionConfig: @@ Attributes: reset_timer: int | None: Motion reset timer in minutes before the motion detection is switched off. daylight_mode: bool | None: Motion detection when light level is below threshold. - sensitivity_level: MotionSensitivity | None: Motion sensitivity level. + 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
662-680: Align the Protocol signature forset_motion_sensitivity_levelwith the convenience overloadTo avoid type‐checking errors for callers using
PlugwiseNode, update the Protocol inplugwise_usb/api.pyso that it accepts the sameMotionSensitivity | int | strunion exposed byPlugwiseScan:• File: plugwise_usb/api.py
• Location: methodset_motion_sensitivity_levelon the Protocol (around line 662)--- a/plugwise_usb/api.py +++ b/plugwise_usb/api.py @@ -660,7 +660,9 @@ class PlugwiseNode(Protocol): async def set_motion_sensitivity_level( - self, level: MotionSensitivity + self, + level: MotionSensitivity | int | str ) -> bool: """Configure motion sensitivity level.This keeps the Protocol in sync with the implemented overload in
plugwise_usb/nodes/scan.py, preventing static type errors when passing anintorstr.plugwise_usb/nodes/scan.py (3)
329-358: Normalize inputs more robustly and avoid exception-level logs for expected invalid valuesGood move widening the API. Two tweaks:
- Accept case-insensitive names and numeric strings (e.g., "medium", "30").
- Use warning (not exception) logging for invalid user input, and include the offending value.
Apply this diff:
async def set_motion_sensitivity_level( - self, level: MotionSensitivity | int | str + self, level: MotionSensitivity | int | str ) -> bool: @@ - if isinstance(level, int): - try: - level = MotionSensitivity(level) - except ValueError: - _LOGGER.exception( - "MotionSensitivity for %s: value error ", self._mac_in_str - ) - return False + if isinstance(level, int): + try: + level = MotionSensitivity(level) + except ValueError: + _LOGGER.warning( + "MotionSensitivity for %s: invalid numeric value %r", + self._mac_in_str, + level, + ) + return False - if isinstance(level, str): - try: - level = MotionSensitivity[level] - except KeyError: - _LOGGER.exception( - "MotionSensitivity for %s: unknown level %s", - self._mac_in_str, - level, - ) - return False + if isinstance(level, str): + s = level.strip() + # numeric string support (e.g., "30") + if s.isdigit(): + try: + level = MotionSensitivity(int(s)) + except ValueError: + _LOGGER.warning( + "MotionSensitivity for %s: invalid numeric string %r", + self._mac_in_str, + level, + ) + return False + else: + try: + level = MotionSensitivity[s.upper()] + except KeyError: + _LOGGER.warning( + "MotionSensitivity for %s: unknown level %r", + self._mac_in_str, + level, + ) + return FalseOptional: For consistency with other setters (e.g., reset timer), consider raising ValueError instead of returning False on invalid input.
471-473: Use a safe default when sending to device to avoid None access and ease type checkingIf sensitivity_level is None (or still annotated as int | None),
.valuecan crash and type checkers will complain. Default to DEFAULT_SENSITIVITY here.Apply this diff:
- self._motion_config.sensitivity_level.value, + (self._motion_config.sensitivity_level or DEFAULT_SENSITIVITY).value,
501-503: Cache writes: guard for None and keep serialization stableMirror the device send: store the enum name, defaulting when needed. This also avoids attribute access on None.
Apply this diff:
- self._set_cache( - CACHE_SCAN_CONFIG_SENSITIVITY, - self._motion_config.sensitivity_level.name, - ) + self._set_cache( + CACHE_SCAN_CONFIG_SENSITIVITY, + (self._motion_config.sensitivity_level or DEFAULT_SENSITIVITY).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 (2)
plugwise_usb/api.py(1 hunks)plugwise_usb/nodes/scan.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/scan.py (2)
plugwise_usb/api.py (3)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-679)name(288-289)plugwise_usb/nodes/node.py (2)
set_motion_sensitivity_level(790-796)name(218-222)
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
plugwise_usb/nodes/scan.py (2)
201-210: Make cache deserialization resilient (handle legacy numeric values and avoid KeyError).
MotionSensitivity[sensitivity_level]will raise a KeyError for legacy caches storing numeric strings (e.g., "30") or invalid content, causing load failures. Add robust parsing: first try enum name (case-insensitive), then fall back to int, and log a warning instead of throwing.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.""" + if (raw := self._get_cache(CACHE_SCAN_CONFIG_SENSITIVITY)) is None: + return None + # Accept enum name (preferred) and legacy numeric values + try: + return MotionSensitivity[raw.strip().upper()] + except KeyError: + pass + try: + return MotionSensitivity(int(raw)) + except (ValueError, KeyError): + _LOGGER.warning( + "Ignoring invalid cached sensitivity '%s' for %s", + raw, + self._mac_in_str, + ) + return None
329-367: Normalize inputs robustly and avoid logging exceptions for expected invalid input.
- IntEnum is a subclass of int; current isinstance(level, int) branch also catches MotionSensitivity, which is OK, but the code/logging can be clearer.
- Treat strings case-insensitively, accept numeric strings, and reject bool explicitly (bool is a subclass of int).
- Use warning logs instead of exception tracebacks for invalid user input.
Apply this diff:
- async def set_motion_sensitivity_level( - self, level: MotionSensitivity | int | str - ) -> bool: + async def set_motion_sensitivity_level( + self, level: MotionSensitivity | int | str + ) -> bool: """Configure the motion sensitivity level.""" - _LOGGER.debug( - "set_motion_sensitivity_level | Device %s | %s -> %s", - self.name, - self._motion_config.sensitivity_level, - str(level), - ) - if isinstance(level, int): - try: - level = MotionSensitivity(level) - except ValueError: - _LOGGER.exception( - "MotionSensitivity for %s: value error ", self._mac_in_str - ) - return False - - if isinstance(level, str): - try: - level = MotionSensitivity[level] - except KeyError: - _LOGGER.exception( - "MotionSensitivity for %s: unknown level %s", - self._mac_in_str, - level, - ) - return False - if self._motion_config.sensitivity_level == level: + original_level = level + _LOGGER.debug( + "set_motion_sensitivity_level | Device %s | %s -> %s", + self.name, + self._motion_config.sensitivity_level, + original_level, + ) + # Normalize input to MotionSensitivity + if isinstance(level, MotionSensitivity): + normalized = level + elif isinstance(level, bool): + _LOGGER.warning( + "MotionSensitivity for %s: bool (%s) is not a valid sensitivity", + self._mac_in_str, + level, + ) + return False + elif isinstance(level, int): + try: + normalized = MotionSensitivity(level) + except ValueError: + _LOGGER.warning( + "MotionSensitivity for %s: invalid numeric value %s", + self._mac_in_str, + level, + ) + return False + elif isinstance(level, str): + key = level.strip().upper() + try: + normalized = MotionSensitivity[key] + except KeyError: + # Try numeric string fallback + try: + normalized = MotionSensitivity(int(key)) + except (ValueError, KeyError): + _LOGGER.warning( + "MotionSensitivity for %s: unknown level %s", + self._mac_in_str, + level, + ) + return False + else: + _LOGGER.warning( + "MotionSensitivity for %s: unsupported type %s", + self._mac_in_str, + type(level).__name__, + ) + return False + if self._motion_config.sensitivity_level == normalized: return False self._motion_config = replace( self._motion_config, - sensitivity_level=level, + sensitivity_level=normalized, dirty=True, ) await self._scan_configure_update() return True
🧹 Nitpick comments (1)
plugwise_usb/nodes/scan.py (1)
468-473: Type mismatch: sensitivity_level property annotated as int but used as Enum (.value).You call self.sensitivity_level.value here, but sensitivity_level is annotated to return int. Static type-checkers will flag that int has no attribute 'value'. Update the property annotation to return MotionSensitivity.
Apply outside this hunk (property lives at Lines 277-282) — suggested snippet:
# Replace the property signature/return type @property 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
📜 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 (2)
plugwise_usb/api.py(2 hunks)plugwise_usb/nodes/scan.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise_usb/api.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/scan.py (3)
plugwise_usb/api.py (2)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-681)plugwise_usb/nodes/node.py (2)
set_motion_sensitivity_level(790-796)_set_cache(689-703)plugwise_usb/nodes/sed.py (1)
dirty(340-342)
🔇 Additional comments (1)
plugwise_usb/nodes/scan.py (1)
497-504: LGTM: cache writes now store enum name and strings.Storing sensitivity via name aligns with your new enum-based deserialization, and writing booleans/ints as strings is consistent with _set_cache semantics. Together with the proposed resilient read, this will handle both new and legacy caches.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (4)
plugwise_usb/nodes/scan.py (4)
201-210: Guard against KeyError and support legacy numeric cache values
sensitivity_levelin cache used to be stored as numeric strings; now it’s stored by enum name. AccessingMotionSensitivity[sensitivity_level]directly will raiseKeyErroron older caches and crash load. Parse both enum names and numeric strings, and fail gracefully.Apply:
- 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 (supports legacy numeric values).""" + raw = self._get_cache(CACHE_SCAN_CONFIG_SENSITIVITY) + if raw is None: + return None + # First try enum by name (new format), then fall back to numeric (legacy). + try: + if isinstance(raw, str): + return MotionSensitivity[raw] + except KeyError: + pass + try: + return MotionSensitivity(int(raw)) + except (ValueError, TypeError): + _LOGGER.warning( + "Ignoring unknown cached sensitivity value %r for %s", + raw, + self._mac_in_str, + ) + return None
277-282: Property type mismatch: return type should be MotionSensitivity, not int
self.sensitivity_levelis used as an enum (.valueaccessed later) and defaults toMotionSensitivity.MEDIUM. The annotation-> intis incorrect and misleading for callers and type-checkers.- def sensitivity_level(self) -> int: + 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
188-190: Potential TypeError when reset timer isn’t cachedIf
CACHE_SCAN_CONFIG_RESET_TIMERis absent,_reset_timer_from_cache()returnsNone, making< None * 60crash. Fall back to the default reset timer.- and int((datetime.now(tz=UTC) - motion_timestamp).total_seconds()) - < self._reset_timer_from_cache() * 60 + and int((datetime.now(tz=UTC) - motion_timestamp).total_seconds()) + < ((self._reset_timer_from_cache() or DEFAULT_RESET_TIMER) * 60)
410-427: Convert reset_timer from seconds to minutes to match MotionConfig expectationsThe code at plugwise_usb/nodes/scan.py:410–427 currently measures the elapsed interval in seconds and stores it into
MotionConfig.reset_timer, which (per API docs and protocol) represents minutes. This will vastly over-report the reset interval. Please convert seconds→minutes (with a minimum of 1 minute and clamp to the device’s max).• Location: plugwise_usb/nodes/scan.py (around line 410)
• Protocol doc: messages/requests.py →reset_timer : Delay in minutes when signal is send
• MotionConfig field: minutes (plugwise_usb/api.py:234)Proposed diff:
- reset_timer = int( - (timestamp - self._reset_timer_motion_on).total_seconds() - ) + # Measure elapsed seconds, then convert to minutes + elapsed_secs = int( + (timestamp - self._reset_timer_motion_on).total_seconds() + ) + # Convert to minutes, enforce at least 1 and at most MAX_UINT_2 + reset_timer = max(1, min(MAX_UINT_2, elapsed_secs // 60))Also update the warning and any related documentation/comments to clarify that
reset_timeris in minutes.
♻️ Duplicate comments (1)
plugwise_usb/nodes/scan.py (1)
329-331: Align base Node signature with widened union (duplicate of earlier review)This subclass now accepts
MotionSensitivity | int | str, butplugwise_usb/nodes/node.pystill declares onlyMotionSensitivity. To avoid type-checker errors and API confusion, update the base method signature and docstring accordingly.Suggested change (in plugwise_usb/nodes/node.py):
@raise_not_loaded -async def set_motion_sensitivity_level(self, level: MotionSensitivity) -> bool: +async def set_motion_sensitivity_level(self, level: MotionSensitivity | int | str) -> bool: """Configure motion sensitivity level.""" if NodeFeature.MOTION not in self._features: raise FeatureError( f"Configuration of motion sensitivity is not supported for node {self.mac}" ) raise NotImplementedError()If you want, I can scan and patch all occurrences to keep the API consistent.
🧹 Nitpick comments (3)
plugwise_usb/nodes/scan.py (3)
339-360: Use warnings for invalid input (no traceback) and accept case-insensitive stringsUsing
_LOGGER.exceptionlogs a traceback even though you handle the error and returnFalse. Switch towarning(orerror) and normalize string input to be case-insensitive.- if isinstance(level, int): + if isinstance(level, int): try: level = MotionSensitivity(level) except ValueError: - _LOGGER.exception( + _LOGGER.warning( "MotionSensitivity for %s: invalid numeric value %s", self._mac_in_str, str(level), ) return False - if isinstance(level, str): + if isinstance(level, str): try: - level = MotionSensitivity[level] + level = MotionSensitivity[level.strip().upper()] except KeyError: - _LOGGER.exception( + _LOGGER.warning( "MotionSensitivity for %s: unknown level %s", self._mac_in_str, level, ) return FalseOptional: consider raising
ValueErrorfor invalid input to match the behavior ofset_motion_reset_timer, or keep returningFalsebut document that explicitly.
499-506: Storing enum name in cache is fine; ensure read path handles legacy valuesWriting
sensitivity_level.namemakes the cache human-readable and stable. Combined with a backward-compatible reader (see earlier comment on_sensitivity_level_from_cache) this is solid.Minor:
_set_cachealready stringifies non-strings. The explicitstr(...)around other values is redundant but harmless.
175-178: Fix docstring: not “awake duration”The method returns daylight mode; the docstring is misleading.
- def _daylight_mode_from_cache(self) -> bool | None: - """Load awake duration from cache.""" + def _daylight_mode_from_cache(self) -> bool | None: + """Load daylight mode from cache."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/scan.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/scan.py (3)
plugwise_usb/api.py (2)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-681)plugwise_usb/nodes/node.py (2)
set_motion_sensitivity_level(790-796)_set_cache(689-703)plugwise_usb/nodes/sed.py (1)
dirty(340-342)
🔇 Additional comments (1)
plugwise_usb/nodes/scan.py (1)
472-475: LGTM: send numeric value to deviceUsing
self.sensitivity_level.valueis correct now that the property returns aMotionSensitivity. This aligns device protocol with numeric values while keeping enum semantics internally.Note: ensure the property annotation is updated to
MotionSensitivity(see prior comment) to avoid confusion.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise_usb/nodes/node.py (1)
790-796: Whitespace nit in union type
All other overrides already useMotionSensitivity | int | str. Just add the missing space inplugwise_usb/nodes/node.py.• plugwise_usb/nodes/node.py: line 790
Changeasync def set_motion_sensitivity_level(self, level: MotionSensitivity |int | str) -> bool:to
async def set_motion_sensitivity_level(self, level: MotionSensitivity | int | str) -> bool:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/node.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/node.py (2)
plugwise_usb/api.py (2)
set_motion_sensitivity_level(662-681)MotionSensitivity(22-27)plugwise_usb/nodes/scan.py (1)
set_motion_sensitivity_level(329-368)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run pytest using Python 3.13
There was a problem hiding this comment.
Actionable comments posted: 3
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: Backwards-compat: handle legacy numeric sensitivity values in cache_cache now stores sensitivity by enum name and loader expects names. Existing installs may still have numeric values like "20" in cache, which will raise KeyError. Add a numeric fallback to avoid breaking upgrades.
Apply this diff:
- def _sensitivity_level_from_cache(self) -> MotionSensitivity | None: + 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 + if (cached := self._get_cache(CACHE_SCAN_CONFIG_SENSITIVITY)) is None: + return None + # Prefer enum-name; fallback to legacy numeric string + try: + return MotionSensitivity[cached] + except KeyError: + try: + return MotionSensitivity(int(cached)) + except (ValueError, KeyError): + _LOGGER.warning( + "Unknown cached sensitivity_level '%s' for %s; using default", + cached, + self._mac_in_str, + ) + return None
🧹 Nitpick comments (4)
plugwise_usb/api.py (1)
662-664: Docstring still states MotionSensitivity-only; update to reflect int|str acceptanceThe signature accepts MotionSensitivity | int | str, but the “Args: level” doc still mentions only MotionSensitivity. Please update to avoid developer confusion.
Proposed docstring section (outside selected lines):
Args: level: Desired sensitivity level. Accepts: - MotionSensitivity enum - int (20=HIGH, 30=MEDIUM, 255=OFF) - str enum name ("HIGH", "MEDIUM", "OFF"; case-insensitive)plugwise_usb/nodes/scan.py (3)
329-369: Normalize string inputs case-insensitively and avoid logging full tracebacks for bad inputs
- Accept case-insensitive enum names (e.g., "medium") to be user-friendly.
- Use _LOGGER.warning instead of _LOGGER.exception for expected input-validation failures to avoid noisy tracebacks.
Apply this diff:
_LOGGER.debug( "set_motion_sensitivity_level | Device %s | %s -> %s", self.name, self._motion_config.sensitivity_level, - str(level), + str(level), ) if isinstance(level, int): try: level = MotionSensitivity(level) except ValueError: - _LOGGER.exception( + _LOGGER.warning( "MotionSensitivity for %s: invalid numeric value %s", self._mac_in_str, str(level), ) return False if isinstance(level, str): try: - level = MotionSensitivity[level] + level = MotionSensitivity[level.upper()] except KeyError: - _LOGGER.exception( + _LOGGER.warning( "MotionSensitivity for %s: unknown level %s", self._mac_in_str, level, ) return FalseAs a follow-up, consider unit tests covering:
- int inputs (20, 30, 255)
- valid strings in various cases (“HIGH”, “high”, “MedIuM”)
- invalid int and string inputs (should return False)
501-508: Guard against None when caching sensitivity_level; store name reliablyIf sensitivity_level is ever None, access to .name will raise. While most paths set it, guarding improves robustness and simplifies future changes.
Apply this diff:
- self._set_cache( - CACHE_SCAN_CONFIG_SENSITIVITY, - self._motion_config.sensitivity_level.name, - ) + level = self._motion_config.sensitivity_level or DEFAULT_SENSITIVITY + self._set_cache(CACHE_SCAN_CONFIG_SENSITIVITY, level.name)
520-535: Confirm retry semantics on NACK/unknown ACK for light calibrationOn non-accepted ACK you return False but keep scheduling active (flag remains True). This will retry every awake cycle. Is this intentional? If not, consider clearing the flag and implementing backoff to avoid repeated requests.
If the intention is to retry, consider logging once when first scheduled and then only on state changes to reduce log noise.
Also, minor grammar nit in the docstring: “Request calibration of light sensitivity of Scan device.”
📜 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 (2)
plugwise_usb/api.py(3 hunks)plugwise_usb/nodes/scan.py(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
plugwise_usb/api.py (2)
plugwise_usb/nodes/scan.py (5)
daylight_mode(234-238)reset_timer(270-274)sensitivity_level(277-281)set_motion_sensitivity_level(329-368)scan_calibrate_light(516-518)plugwise_usb/nodes/node.py (2)
set_motion_sensitivity_level(790-798)scan_calibrate_light(369-371)
plugwise_usb/nodes/scan.py (4)
plugwise_usb/api.py (4)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-681)name(288-289)scan_calibrate_light(683-688)plugwise_usb/nodes/node.py (4)
set_motion_sensitivity_level(790-798)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: Run pytest using Python 3.13
🔇 Additional comments (3)
plugwise_usb/api.py (1)
236-244: LGTM: switch to MotionSensitivity on MotionConfig.sensitivity_level is coherentThe dataclass and its Attributes docstring now accurately reflect MotionSensitivity. This aligns with nodes/scan.py usage and improves clarity.
plugwise_usb/nodes/scan.py (2)
89-89: LGTM: initialize calibration scheduling flagThe explicit _scan_calibrate_light_scheduled flag improves readability of the awake-task workflow.
452-454: LGTM: hook calibration scheduling into awake cycleThis integrates cleanly with existing awake-task handling.
There was a problem hiding this comment.
Actionable comments posted: 2
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-210: Backward-compatibility for cached sensitivity valuesOlder caches likely store numeric strings (e.g., "30"). MotionSensitivity["30"] will raise KeyError and break load. Parse legacy numeric strings and handle invalid values gracefully.
- def _sensitivity_level_from_cache(self) -> MotionSensitivity | None: + 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 + raw = self._get_cache(CACHE_SCAN_CONFIG_SENSITIVITY) + if raw is None: + return None + # Prefer enum name (new cache) + try: + return MotionSensitivity[raw] + except KeyError: + # Fallback for legacy caches storing numeric values as strings + try: + return MotionSensitivity(int(raw)) + except (ValueError, KeyError): + _LOGGER.warning( + "Invalid cached sensitivity '%s' for %s; using default.", + raw, + self.mac, + ) + return None
♻️ Duplicate comments (2)
plugwise_usb/nodes/scan.py (2)
277-282: Type-correct sensitivity_level property — thanks for fixingReturning MotionSensitivity (not int) resolves the earlier static typing mismatch with uses of .value/.name.
493-496: Public API mismatch: scan_calibrate_light should return boolapi.py and base nodes expect scan_calibrate_light() -> bool. Make this method idempotent and return True only when you transition from unscheduled->scheduled.
- async def scan_calibrate_light(self) -> None: - """Schedule light sensitivity calibration of Scan device.""" - self._scan_calibrate_light_scheduled = True + async def scan_calibrate_light(self) -> bool: + """Schedule light sensitivity calibration of Scan device. + + Returns True when scheduling was newly activated; False if it was already scheduled. + """ + if self._scan_calibrate_light_scheduled: + return False + self._scan_calibrate_light_scheduled = True + return True
🧹 Nitpick comments (4)
tests/test_usb.py (1)
1494-1504: Consider adding tests for legacy cache and light calibration scheduling
- Legacy cache: Add a test where CACHE_SCAN_CONFIG_SENSITIVITY returns "30" (old numeric string) and assert that sensitivity_level resolves to MotionSensitivity.MEDIUM. This protects users upgrading with existing caches.
- Light calibration: Add a small test to assert scan.scan_calibrate_light() returns a boolean and that repeated scheduling returns False when already scheduled. This will guard the public API contract.
I can draft these tests if helpful.
Also applies to: 2240-2270
plugwise_usb/nodes/scan.py (3)
89-89: Nit: annotate and document the scheduling flagAdd a type hint and a brief comment so future readers know the lifecycle of this flag (scheduled on user call, cleared on accepted ack).
- self._scan_calibrate_light_scheduled = False + # True when a light calibration should be sent on next awake cycle + self._scan_calibrate_light_scheduled: bool = False
429-431: Awake task robustness: prevent calibration exception from aborting flowcurrent behavior: if _scan_calibrate_light raises (e.g., NodeTimeout), this await will bubble up and may abort the awake flow. Prefer having _scan_calibrate_light return False on failure (see next comments), or catch/log here.
- if self._scan_calibrate_light_scheduled: - await self._scan_calibrate_light() + if self._scan_calibrate_light_scheduled: + try: + await self._scan_calibrate_light() + except Exception: # keep awake loop resilient + _LOGGER.warning("Light calibration failed for %s; will retry", self.mac)
478-485: Cache writes are fine; explicit str() is redundant_set_cache already stringifies non-datetime values. Keeping str() is harmless, but you can omit it for consistency with other setters.
- self._set_cache(CACHE_SCAN_CONFIG_RESET_TIMER, str(self.reset_timer)) + self._set_cache(CACHE_SCAN_CONFIG_RESET_TIMER, self.reset_timer) @@ - self._motion_config.sensitivity_level.name, + self._motion_config.sensitivity_level.name, @@ - 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_DAYLIGHT_MODE, self.daylight_mode) + self._set_cache(CACHE_SCAN_CONFIG_DIRTY, self.dirty)
📜 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/api.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)
- plugwise_usb/api.py
🧰 Additional context used
🧬 Code graph analysis (2)
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)
tests/test_usb.py (3)
plugwise_usb/api.py (3)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-679)motion_config(498-508)plugwise_usb/nodes/scan.py (4)
set_motion_sensitivity_level(329-345)sensitivity_level(277-281)motion_config(260-267)dirty(229-231)plugwise_usb/nodes/node.py (2)
set_motion_sensitivity_level(790-796)motion_config(249-255)
⏰ 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 (6)
tests/test_usb.py (5)
1497-1497: Good switch to enum in request construction; serialization still stablePassing MotionSensitivity.MEDIUM keeps the serialized payload identical (0x1E). Assertion below confirms backward compatibility.
1837-1837: Tests now exercise enum-typed API surfaceUsing MotionSensitivity in the basenode error-path tests aligns with the updated typing and expected exceptions.
Also applies to: 1868-1868, 1895-1895
2243-2249: Assertions against enum values look correctValidating both the property and the motion_config to be MotionSensitivity.MEDIUM ensures type and value correctness after load.
2252-2254: Behavioral check on sensitivity change is appropriateAsserting False on idempotent set and True on actual change matches the contract.
2266-2269: Post-awake confirmation of updated sensitivityVerifies both surface property and internal config carry MotionSensitivity.HIGH after the awake cycle.
plugwise_usb/nodes/scan.py (1)
451-454: Using public properties for serialization is correctreset_timer, sensitivity_level.value, and daylight_mode reflect current effective settings, including defaults. This also decouples on-wire values from internal None states.
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)
387-406: reset_timer unit mismatch: seconds vs minutesreset_timer is documented and handled elsewhere as minutes, but here it’s assigned in seconds. This causes incorrect comparisons and configuration being sent to the device.
Fix by converting to minutes (min 1), and compare minutes-to-minutes:
- reset_timer = int( - (timestamp - self._reset_timer_motion_on).total_seconds() - ) + # Convert observed motion-on duration to minutes (rounded down), clamp to at least 1 minute + reset_timer = max( + 1, + int((timestamp - self._reset_timer_motion_on).total_seconds() // 60), + ) ... - elif reset_timer < self._motion_config.reset_timer: + elif reset_timer < self._motion_config.reset_timer: _LOGGER.warning( "Adjust reset timer for %s from %s -> %s", self.name, self._motion_config.reset_timer, reset_timer, )
181-194: Possible TypeError when cached reset_timer is missingWhen CACHE_SCAN_MOTION_STATE is "True" and the timestamp is present, you multiply a possibly None reset timer by 60. If the cache doesn’t contain reset_timer yet, this will throw.
Use a safe fallback:
- and int((datetime.now(tz=UTC) - motion_timestamp).total_seconds()) - < self._reset_timer_from_cache() * 60 + and int((datetime.now(tz=UTC) - motion_timestamp).total_seconds()) + < (self._reset_timer_from_cache() or DEFAULT_RESET_TIMER) * 60
201-210: Backwards-compatible cache parsing for sensitivityOlder caches may store numeric sensitivity values. Current code assumes enum names and will KeyError on legacy numeric strings.
Be tolerant to both enum names and numeric values:
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] + try: + # Prefer name-based (new format) + return MotionSensitivity[sensitivity_level] + except KeyError: + try: + # Fallback to value-based (legacy numeric) + return MotionSensitivity(int(sensitivity_level)) + except (ValueError, KeyError): + _LOGGER.warning( + "Unknown sensitivity level in cache for %s: %s", + self.name, + sensitivity_level, + ) + return None return None
♻️ Duplicate comments (1)
plugwise_usb/nodes/scan.py (1)
504-525: _scan_calibrate_light lacks a final return valueFor unexpected ack types you log a warning but fall off the end of the function, returning None despite the bool return type. This can propagate as a logic bug.
_LOGGER.warning( "Unexpected ack type %s for light calibration on %s", response.node_ack_type, self.name, ) - + return False
🧹 Nitpick comments (5)
CHANGELOG.md (2)
5-5: Tighten wording and match enum casingUse the enum names and tweak phrasing for clarity and consistency with code (MotionSensitivity: OFF/MEDIUM/HIGH) and product naming (“Scan”).
Apply:
-- PR [323](https://github.com/plugwise/python-plugwise-usb/pull/323): Motion Sensitivity to use named levels (Off/Medium/High) instead of numeric values, add light sensitivity calibration on wake-up for scan devices. +- PR [323](https://github.com/plugwise/python-plugwise-usb/pull/323): Use named motion sensitivity levels (OFF/MEDIUM/HIGH) instead of numeric values, and add light-sensitivity calibration on device wake-up for Scan devices.
6-6: Punctuation/style consistencyPrevious bullets often end with a period; add one here for consistency.
-- PR [322](https://github.com/plugwise/python-plugwise-usb/pull/322): Improve Circle+ load function to align to Circle load function +- PR [322](https://github.com/plugwise/python-plugwise-usb/pull/322): Improve Circle+ load function to align to Circle load function.plugwise_usb/nodes/scan.py (3)
478-485: Avoid None dereference when caching sensitivityIf sensitivity_level hasn’t been set yet (e.g., prior to load-from-cache defaults), .name will raise. Use the property that guarantees a value.
- self._set_cache( - CACHE_SCAN_CONFIG_SENSITIVITY, - self._motion_config.sensitivity_level.name, - ) + self._set_cache( + CACHE_SCAN_CONFIG_SENSITIVITY, + self.sensitivity_level.name, + )
315-318: Error message should reflect the actual upper boundThe guard uses MAX_UINT_2, but the message hardcodes 255. Align the message to the constant to avoid confusion if MAX_UINT_2 != 255.
- raise ValueError( - f"Invalid motion reset timer ({minutes}). It must be between 1 and 255 minutes." - ) + raise ValueError( + f"Invalid motion reset timer ({minutes}). It must be between 1 and {MAX_UINT_2} minutes." + )
360-365: Consider lowering log level for switch-group eventsSwitch-group transitions for motion will be frequent; warning-level logging may be noisy in production. Recommend debug or info.
- _LOGGER.warning("%s received %s", self.name, response) + _LOGGER.debug("%s received %s", self.name, response)
📜 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)
CHANGELOG.md(1 hunks)plugwise_usb/nodes/scan.py(9 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/nodes/scan.py (5)
plugwise_usb/exceptions.py (2)
MessageError(20-21)NodeError(24-25)plugwise_usb/api.py (3)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-679)name(288-289)plugwise_usb/nodes/node.py (3)
set_motion_sensitivity_level(790-796)name(218-222)_set_cache(689-703)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)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...calibration on wake-up for scan devices. - PR [322](https://github.com/plugwise/pyt...
(QB_NEW_EN)
⏰ 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 (5)
plugwise_usb/nodes/scan.py (5)
329-346: LGTM: enum-based sensitivity pathSwitching to MotionSensitivity in the public setter, logging names, and pushing config via value is clean and aligns API, cache, and transport.
451-454: LGTM: configure uses reset_timer, sensitivity.value, daylight_modeExplicitly sending the numeric enum value is correct. Good warning paths on None/failed ack.
494-503: LGTM: scheduling returns a booleanReturning True only on first schedule and False if already scheduled is a simple, race-free contract for callers.
269-282: LGTM: sensitivity_level property returns MotionSensitivityThis resolves the prior type mismatch (.value/.name usage) and removes the need for guard code at callers.
1-553: Allset_motion_sensitivity_levelcalls correctly use theMotionSensitivityenumVerification output shows:
- Every call site (in tests/test_usb.py) passes
pw_api.MotionSensitivity.<LEVEL>- No occurrences of raw integer literals
- No occurrences of quoted strings
No further action required.
…ect contains None
scan_light_calibration
bump version to 0.44.12
|
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)
179-194: Fix None handling for reset_timer when deriving motion-from-cache.If reset_timer is absent in cache,
_reset_timer_from_cache()returns None and the multiplicationNone * 60raises TypeError. Use a default.- if ( - cached_motion_state := self._get_cache(CACHE_SCAN_MOTION_STATE) - ) is not None: - if ( - cached_motion_state == "True" - and (motion_timestamp := self._motion_timestamp_from_cache()) - is not None - and int((datetime.now(tz=UTC) - motion_timestamp).total_seconds()) - < self._reset_timer_from_cache() * 60 - ): - return True - return False + cached_motion_state = self._get_cache(CACHE_SCAN_MOTION_STATE) + if cached_motion_state is not None: + if cached_motion_state == "True": + motion_timestamp = self._motion_timestamp_from_cache() + if motion_timestamp is None: + return False + reset_timer = self._reset_timer_from_cache() or DEFAULT_RESET_TIMER + if int((datetime.now(tz=UTC) - motion_timestamp).total_seconds()) < reset_timer * 60: + return True + return False return DEFAULT_MOTION_STATE
201-210: Make cache reader robust to legacy numeric sensitivity values.Older caches likely stored numeric values (e.g., "30"). Indexing the enum by name will KeyError. Accept both enum names and numeric strings/ints.
- 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 (supports enum-name and legacy numeric).""" + raw = self._get_cache(CACHE_SCAN_CONFIG_SENSITIVITY) + if raw is None: + return None + try: + # Preferred: enum name stored as string, e.g. "MEDIUM" + return MotionSensitivity[str(raw)] + except Exception: + try: + # Legacy: numeric value stored as int or str, e.g. 30 + return MotionSensitivity[int(raw)] + except Exception: + _LOGGER.warning( + "Unknown cached sensitivity_level %r for %s; falling back to %s", + raw, + self.name, + DEFAULT_SENSITIVITY.name, + ) + return DEFAULT_SENSITIVITY
387-405: reset_timer computed in seconds; config expects minutes.
reset_timeris derived via total_seconds() and then stored directly, but the rest of the code and protocol treat it as minutes. This will misconfigure the device and skew comparisons.- reset_timer = int( - (timestamp - self._reset_timer_motion_on).total_seconds() - ) - if self._motion_config.reset_timer is None: - self._motion_config = replace( - self._motion_config, reset_timer=reset_timer, dirty=True - ) + reset_seconds = int( + (timestamp - self._reset_timer_motion_on).total_seconds() + ) + # Convert to minutes, round up, ensure minimum 1 + reset_minutes = max(1, (reset_seconds + 59) // 60) + if self._motion_config.reset_timer is None: + self._motion_config = replace( + self._motion_config, reset_timer=reset_minutes, dirty=True + ) - elif reset_timer < self._motion_config.reset_timer: + elif reset_minutes < self._motion_config.reset_timer: _LOGGER.warning( "Adjust reset timer for %s from %s -> %s", self.name, self._motion_config.reset_timer, - reset_timer, + reset_minutes, ) self._motion_config = replace( - self._motion_config, reset_timer=reset_timer, dirty=True + self._motion_config, reset_timer=reset_minutes, dirty=True )
♻️ Duplicate comments (1)
plugwise_usb/nodes/scan.py (1)
504-525: Ensure _scan_calibrate_light always returns a bool.On unexpected ack you log a warning but don’t return a value — this yields None. Make sure to return False in that path.
if ( response.node_ack_type == NodeAckResponseType.SCAN_LIGHT_CALIBRATION_ACCEPTED ): self._scan_calibrate_light_scheduled = False return True _LOGGER.warning( "Unexpected ack type %s for light calibration on %s", response.node_ack_type, self.name, ) + return False
🧹 Nitpick comments (3)
CHANGELOG.md (1)
5-6: Polish wording and consistency in new bullet points.
- Prefer “align with” over “align to”.
- Be consistent about capitalization and hyphenation; consider using enum-case (OFF/MEDIUM/HIGH) to match code, or keep all Title Case but be consistent.
- Optional: end both bullets with a period.
Apply:
- - PR [323](https://github.com/plugwise/python-plugwise-usb/pull/323): Motion Sensitivity to use named levels (Off/Medium/High) instead of numeric values, add light sensitivity calibration on wake-up for scan devices. - - PR [322](https://github.com/plugwise/python-plugwise-usb/pull/322): Improve Circle+ load function to align to Circle load function + - PR [323](https://github.com/plugwise/python-plugwise-usb/pull/323): Motion sensitivity now uses named levels (OFF/MEDIUM/HIGH) instead of numeric values; add light sensitivity calibration on wake up for Scan devices. + - PR [322](https://github.com/plugwise/python-plugwise-usb/pull/322): Improve Circle+ load function to align with the Circle load function.plugwise_usb/nodes/scan.py (2)
315-318: Bounds message mismatches the actual check.You validate against
MAX_UINT_2but the error says “1 and 255 minutes.” Make these consistent, or clamp to the actual device spec (likely 1–240 min).Option A — keep current bound, fix message:
- raise ValueError( - f"Invalid motion reset timer ({minutes}). It must be between 1 and 255 minutes." - ) + raise ValueError( + f"Invalid motion reset timer ({minutes}). It must be between 1 and {MAX_UINT_2} minutes." + )Option B — enforce device spec (recommended if the firmware only accepts up to 240 min):
# Near defaults SCAN_RESET_TIMER_MAX: Final = 240- if minutes < 1 or minutes > MAX_UINT_2: + if minutes < 1 or minutes > SCAN_RESET_TIMER_MAX: raise ValueError( - f"Invalid motion reset timer ({minutes}). It must be between 1 and 255 minutes." + f"Invalid motion reset timer ({minutes}). It must be between 1 and {SCAN_RESET_TIMER_MAX} minutes." )Would you like me to adjust to Option A or B?
478-485: Use property to avoid None when caching sensitivity level.If sensitivity is ever None,
.nameon the field will raise. The property already guarantees a value (falls back to default).self._set_cache( CACHE_SCAN_CONFIG_SENSITIVITY, - self._motion_config.sensitivity_level.name, + self.sensitivity_level.name, ) - self._set_cache(CACHE_SCAN_CONFIG_DAYLIGHT_MODE, self.daylight_mode) - self._set_cache(CACHE_SCAN_CONFIG_DIRTY, self.dirty) + self._set_cache(CACHE_SCAN_CONFIG_DAYLIGHT_MODE, self.daylight_mode) + self._set_cache(CACHE_SCAN_CONFIG_DIRTY, self.dirty)
📜 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 (5)
CHANGELOG.md(1 hunks)plugwise_usb/api.py(2 hunks)plugwise_usb/nodes/scan.py(9 hunks)pyproject.toml(1 hunks)tests/test_usb.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- plugwise_usb/api.py
- pyproject.toml
- tests/test_usb.py
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/nodes/scan.py (5)
plugwise_usb/exceptions.py (2)
MessageError(20-21)NodeError(24-25)plugwise_usb/api.py (3)
MotionSensitivity(22-27)set_motion_sensitivity_level(662-679)name(288-289)plugwise_usb/nodes/node.py (3)
set_motion_sensitivity_level(790-796)name(218-222)_set_cache(689-703)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)
🪛 LanguageTool
CHANGELOG.md
[grammar] ~5-~5: There might be a mistake here.
Context: ...calibration on wake-up for scan devices. - PR [322](https://github.com/plugwise/pyt...
(QB_NEW_EN)
⏰ 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 (6)
plugwise_usb/nodes/scan.py (6)
22-22: Import cleanup aligns with non-raising timeout semantics.Dropping NodeTimeout in favor of warning-and-false paths matches recent changes. Good.
277-282: Property now returns MotionSensitivity (not int) — good.This matches downstream usage of
.valueand.nameand resolves the previous type mismatch.
329-336: Safer debug logging by using the property — nice.This avoids dereferencing None when the cached level isn’t set yet.
429-431: Awake task hook for light calibration scheduling — good.This neatly reuses the awake loop to perform the deferred calibration.
451-454: Correct payload types for ScanConfigureRequest.Passing reset_timer (int), sensitivity_level.value (int), and daylight_mode (bool) matches the protocol.
494-503: Scheduling API returning bool — matches API docs.Clear semantics and idempotent behavior. Looks good.
Summary by CodeRabbit