Append report interval setting to Sense node configuration. #338
Conversation
Extend MockStickController with a response queue to send allow sending different responses sequentially
WalkthroughAdds a per-node Sense report_interval field to the hysteresis config, persists and exposes it on PlugwiseSense with setter and awake-time configuration requests/ACK handling, updates tests to a queued mock response model for multi-step flows, and bumps version/changelog. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Caller
participant PS as PlugwiseSense
participant SC as StickController
participant D as Sense Device
rect rgba(230,245,255,0.6)
Note over U,PS: Request to set report interval
U->>PS: set_report_interval(minutes)
alt invalid or unchanged
PS-->>U: False / no-op
else valid & changed
PS->>PS: mark config dirty
PS-->>U: True (update scheduled)
end
end
rect rgba(240,255,240,0.6)
Note over PS,D: Awake configuration phase
PS->>SC: SenseReportIntervalRequest(report_interval)
SC->>D: transmit
alt ACK accepted
D-->>SC: SENSE_INTERVAL_ACCEPTED
SC-->>PS: ACK accepted
PS->>PS: persist to cache/state
else ACK failed or timeout
D-->>SC: SENSE_INTERVAL_FAILED / none
SC-->>PS: failure/timeout
PS->>PS: log failure (no persist)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
✨ 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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #338 +/- ##
==========================================
+ Coverage 81.30% 81.33% +0.02%
==========================================
Files 36 36
Lines 8062 8107 +45
==========================================
+ Hits 6555 6594 +39
- Misses 1507 1513 +6 ☔ 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: 2
🧹 Nitpick comments (7)
plugwise_usb/api.py (1)
281-282: Tighten docstring for report_interval (unit + range, minor grammar)Clarify minutes and expected range.
- report_interval: int | None = None: Interval time at which the temperature and humidity are reported + report_interval: int | None = None: Interval in minutes at which temperature and humidity are reported (1–60)plugwise_usb/nodes/sense.py (3)
76-76: Default interval constant: consider explicit typing (nit)Optional: annotate Final[int] for clarity.
-DEFAULT_SENSE_HYSTERESIS_REPORT_INTERVAL: Final = 15 +DEFAULT_SENSE_HYSTERESIS_REPORT_INTERVAL: Final[int] = 15
260-267: Fix type hint and docstring for _report_interval_from_cacheReturns int, not float. Align annotation and wording.
- def _report_interval_from_cache(self) -> float | None: - """Report interval from cache.""" + def _report_interval_from_cache(self) -> int | None: + """Load report interval from cache.""" if ( report_interval := self._get_cache(CACHE_SENSE_HYSTERESIS_REPORT_INTERVAL) ) is not None: return int(report_interval) return None
565-589: Setter looks good; optionally enforce int inputPrevent accidental floats/bools.
_LOGGER.debug( "set_report_interval | Device %s | %s -> %s", self.name, self._hysteresis_config.report_interval, report_interval, ) + if not isinstance(report_interval, int) or isinstance(report_interval, bool): + raise TypeError( + "report_interval must be an int number of minutes (1–60)." + ) if report_interval < 1 or report_interval > 60: raise ValueError( f"Invalid measurement interval {report_interval}. It must be between 1 and 60 minutes" )tests/test_usb.py (3)
251-254: Param/name nits and typing for clarity.Avoid shadowing the attribute name and add a precise type.
- def append_response(self, send_response) -> None: - """Add response to queue.""" - self.send_response.append(send_response) + def append_response(self, response: pw_responses.PlugwiseResponse) -> None: + """Add response to queue.""" + self.send_response.append(response)
261-263: FIFO pop from front; prefer deque for efficiency.
pop(0)is O(n). For small queues it’s fine, butdeque.popleft()is cleaner and O(1).Minimal tweak (keep list, micro cleanup):
- if len(self.send_response) > 0: - return self.send_response.pop(0) + if self.send_response: + return self.send_response.pop(0) return NoneIf switching to deque (requires import and minor changes outside this hunk):
- self.send_response.append(response) + self.send_response.append(response) # in append_responseand
- if self.send_response: - return self.send_response.pop(0) + if self.send_response: + return self.send_response.popleft()Additional changes outside this hunk:
- Add
from collections import dequenear the imports.- In
__init__:self.send_response: deque[pw_responses.PlugwiseResponse] = deque().
2299-2299: Provide a clear helper to reset the queue (avoid poking internals).Instead of assigning
send_response = [], addclear_responses()on the mock. Safer if the backing container changes (e.g., to deque).Example addition (outside this hunk):
+ def clear_responses(self) -> None: + """Clear queued responses.""" + self.send_response.clear()Then use
mock_stick_controller.clear_responses().
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugwise_usb/api.py(2 hunks)plugwise_usb/messages/requests.py(1 hunks)plugwise_usb/nodes/sense.py(13 hunks)tests/test_usb.py(19 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-01T11:34:30.372Z
Learnt from: dirixmjm
PR: plugwise/python-plugwise-usb#330
File: plugwise_usb/nodes/sense.py:659-661
Timestamp: 2025-09-01T11:34:30.372Z
Learning: In Plugwise Sense hysteresis configuration, the disabled sentinel values are hardware-specific: 17099 for temperature (-1°C) and 2621 for humidity (-1%), as actually sent by the Source device, not calculated values.
Applied to files:
plugwise_usb/nodes/sense.py
📚 Learning: 2025-09-02T11:06:20.120Z
Learnt from: dirixmjm
PR: plugwise/python-plugwise-usb#330
File: plugwise_usb/api.py:90-90
Timestamp: 2025-09-02T11:06:20.120Z
Learning: Plugwise Sense devices push hysteresis state updates through the NODE_SWITCH_GROUP_ID mechanism. When hysteresis thresholds are crossed, the device sends NodeSwitchGroupResponse messages which are processed by the _switch_group method, updating humidity_state and temperature_state, then publishing to subscribers via NodeFeature.SENSE.
Applied to files:
plugwise_usb/nodes/sense.py
📚 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/sense.py
🧬 Code graph analysis (3)
plugwise_usb/api.py (1)
plugwise_usb/nodes/sense.py (1)
report_interval(359-363)
tests/test_usb.py (3)
plugwise_usb/messages/requests.py (16)
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)PlugwiseRequest(60-335)plugwise_usb/connection/__init__.py (1)
send(227-238)plugwise_usb/messages/responses.py (3)
PlugwiseResponse(98-215)NodeAckResponse(902-924)deserialize(137-201)
plugwise_usb/nodes/sense.py (2)
plugwise_usb/messages/requests.py (18)
SenseReportIntervalRequest(1479-1509)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)send(904-913)plugwise_usb/messages/responses.py (1)
NodeAckResponseType(75-84)
🔇 Additional comments (14)
plugwise_usb/messages/requests.py (1)
1496-1496: Docstring correction LGTMAccurate class name; no functional impact.
plugwise_usb/api.py (1)
297-297: New field addition LGTMField type and default align with usage across the PR.
plugwise_usb/nodes/sense.py (7)
21-24: Importing SenseReportIntervalRequest is correctMatches request/ack flow and response types.
65-65: Cache key addition LGTMStable naming consistent with other hysteresis keys.
183-186: Loading report_interval from cache LGTMMarking dirty when missing ensures the device gets configured on next wake.
197-199: Propagating report_interval into SenseHysteresisConfig LGTMKeeps the public config coherent.
690-691: Awake tasks integration LGTMNew task is included in the grouped configure call and partial-failure logging is clear.
Also applies to: 699-704
818-846: Report-interval configure task LGTMAck handling mirrors humidity/temperature flows; good early-return on non-dirty config.
871-871: Persisting report_interval to cache LGTMEnsures durability and consistent state serialization.
tests/test_usb.py (5)
1999-1999: Good switch to queued responses for SED flows.Using
append_response(...)here makes multi-step awake/config handling deterministic. LGTM.Also applies to: 2017-2017, 2043-2043, 2069-2069, 2090-2090, 2115-2115
2221-2221: Queued Scan acks look correct.Preloading fail/accept sequences via
append_responsematches the expected awake retry path. LGTM.Also applies to: 2235-2235, 2259-2259
2368-2370: Cache hook for Sense report_interval added.Matches the new config surface; ensures default
15is restored from cache for tests. LGTM.
2466-2469: Sense: correct multi-step sequencing (hysteresis + report_interval).The queued (fail→retry→accept) sequences align with the new awake/config workflow and the added report-interval programming. LGTM.
Also applies to: 2473-2476, 2500-2503, 2531-2534, 2543-2546, 2570-2573
2403-2411: Verified: NodeAckResponseType includes 0x00B3 / 0x00B4 — no action required.SENSE_INTERVAL_ACCEPTED = b"00B3" and SENSE_INTERVAL_FAILED = b"00B4" are defined in plugwise_usb/messages/responses.py (≈lines 81–82).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_usb.py (1)
3-4: Make response queue instance-scoped and use O(1) FIFO.Class-level mutable
send_responseis shared across tests/instances → flakiness. Also,pop(0)is O(n). Move the queue to__init__and switch tocollections.deque.Apply:
@@ import asyncio -from collections.abc import Callable, Coroutine +from collections.abc import Callable, Coroutine +from collections import deque @@ class MockStickController: """Mock stick controller.""" - send_response: list = [] + def __init__(self) -> None: + # Instance-scoped FIFO queue to avoid cross-test bleed between tests/instances. + self.send_response: deque[pw_responses.PlugwiseResponse] = deque() @@ async def send( self, request: pw_requests.PlugwiseRequest, # type: ignore[name-defined] suppress_node_errors=True, ) -> pw_responses.PlugwiseResponse | None: # type: ignore[name-defined] """Submit request to queue and return response.""" - if self.send_response: - return self.send_response.pop(0) + if self.send_response: + return self.send_response.popleft() return NoneAlso applies to: 231-267
plugwise_usb/nodes/sense.py (1)
358-364: Fix type and docstring of report_interval property.Public API should be int minutes; docstring is incorrect.
@property -def report_interval(self) -> float: - """Temperature lower bound value.""" +def report_interval(self) -> int: + """Sense report interval in minutes.""" if self._hysteresis_config.report_interval is not None: return self._hysteresis_config.report_interval return DEFAULT_SENSE_HYSTERESIS_REPORT_INTERVAL
🧹 Nitpick comments (2)
tests/test_usb.py (1)
2407-2414: Add negative-path coverage for report interval (SENSE_INTERVAL_FAILED).
sense_config_report_interval_failedis defined but never enqueued; add one failing ACK path assertion (dirty remains True; retry succeeds).Also applies to: 2471-2480, 2534-2542, 2574-2581
plugwise_usb/nodes/sense.py (1)
714-717: Name the disabled sentinels (avoid magic numbers 2621/17099).Improves readability and aligns with device-specific semantics.
Suggested constants (place near other DEFAULT/SENSE constants):
+SENSE_DISABLED_HUMIDITY_SENTINEL: Final = 2621 # -1% (hardware sentinel) +SENSE_DISABLED_TEMPERATURE_SENTINEL: Final = 17099 # -1°C (hardware sentinel)And use them:
- humidity_lower_bound = 2621 - humidity_upper_bound = 2621 + humidity_lower_bound = SENSE_DISABLED_HUMIDITY_SENTINEL + humidity_upper_bound = SENSE_DISABLED_HUMIDITY_SENTINEL @@ - temperature_lower_bound = 17099 - temperature_upper_bound = 17099 + temperature_lower_bound = SENSE_DISABLED_TEMPERATURE_SENTINEL + temperature_upper_bound = SENSE_DISABLED_TEMPERATURE_SENTINELAlso applies to: 760-762
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise_usb/api.py(2 hunks)plugwise_usb/nodes/sense.py(13 hunks)tests/test_usb.py(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise_usb/api.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-01T11:34:30.372Z
Learnt from: dirixmjm
PR: plugwise/python-plugwise-usb#330
File: plugwise_usb/nodes/sense.py:659-661
Timestamp: 2025-09-01T11:34:30.372Z
Learning: In Plugwise Sense hysteresis configuration, the disabled sentinel values are hardware-specific: 17099 for temperature (-1°C) and 2621 for humidity (-1%), as actually sent by the Source device, not calculated values.
Applied to files:
plugwise_usb/nodes/sense.py
📚 Learning: 2025-09-02T11:06:20.120Z
Learnt from: dirixmjm
PR: plugwise/python-plugwise-usb#330
File: plugwise_usb/api.py:90-90
Timestamp: 2025-09-02T11:06:20.120Z
Learning: Plugwise Sense devices push hysteresis state updates through the NODE_SWITCH_GROUP_ID mechanism. When hysteresis thresholds are crossed, the device sends NodeSwitchGroupResponse messages which are processed by the _switch_group method, updating humidity_state and temperature_state, then publishing to subscribers via NodeFeature.SENSE.
Applied to files:
plugwise_usb/nodes/sense.py
🧬 Code graph analysis (2)
tests/test_usb.py (2)
plugwise_usb/messages/requests.py (16)
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)PlugwiseRequest(60-335)plugwise_usb/connection/__init__.py (1)
send(227-238)
plugwise_usb/nodes/sense.py (3)
plugwise_usb/messages/requests.py (18)
SenseReportIntervalRequest(1479-1509)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)send(904-913)plugwise_usb/nodes/node.py (3)
_get_cache(653-657)name(218-222)_set_cache(690-704)plugwise_usb/messages/responses.py (2)
node_ack_type(918-920)NodeAckResponseType(75-84)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/test_usb.py (6)
231-269: Use deque for O(1) FIFO and tighten typing/naming.pop(0) on list is O(n). deque gives O(1) popleft and clearer FIFO semantics. Also, rename the parameter in append_response to response and add a type hint.
Apply:
@@ - def __init__(self) -> None: - """Initialize MockStickController.""" - self.send_response: list[pw_responses.PlugwiseResponse] = [] + def __init__(self) -> None: + """Initialize MockStickController.""" + self.send_response: deque[pw_responses.PlugwiseResponse] = deque() @@ - def append_response(self, send_response) -> None: - """Add response to queue.""" - self.send_response.append(send_response) + def append_response(self, response: pw_responses.PlugwiseResponse) -> None: + """Enqueue a response (FIFO).""" + self.send_response.append(response) @@ - if self.send_response: - return self.send_response.pop(0) + if self.send_response: + return self.send_response.popleft() return NoneAdd import (outside this hunk):
from collections import deque
257-260: Minor naming nit: pluralize to reflect collection.clear_response() clears all queued items; clear_responses() better conveys intent.
- def clear_response(self) -> None: + def clear_responses(self) -> None: """Clear response queue.""" self.send_response.clear()
2005-2005: Guard against leftover queued responses between awake cycles.If the implementation early-returns on first failure, subsequent pre-queued responses may remain and affect the next cycle’s expectations. Add an assertion that the queue is empty after each cycle or explicitly clear it before enqueuing the next group.
Example:
await asyncio.wait_for(asyncio.shield(test_sed._delayed_task), timeout=2) assert not mock_stick_controller.send_response # ensure no leftovers # or mock_stick_controller.clear_responses()Also applies to: 2023-2023, 2049-2049, 2075-2075, 2096-2096, 2121-2121
2409-2416: De-magic the ACK payload construction.These raw b"0100...00B3/00B4" bytes are brittle. Consider a tiny helper/fixture to build NodeAckResponse for a given MAC and code to improve readability and maintenance.
Example helper:
def make_node_ack(mac_hex: bytes, code_hex: bytes) -> pw_responses.NodeAckResponse: ack = pw_responses.NodeAckResponse() ack.deserialize(construct_message(b"0100" + mac_hex + code_hex, b"0000")) return ackThen:
sense_config_report_interval_accepted = make_node_ack(b"5555...5555", b"00B3") sense_config_report_interval_failed = make_node_ack(b"5555...5555", b"00B4")
2473-2476: Bundle responses per awake-cycle and verify drain.You enqueue multiple (accepted/failed) triplets across several cycles. To avoid hidden coupling to internal send order, group responses per cycle and assert the queue is drained afterwards. A tiny local helper can reduce repetition.
Example:
async def run_awake_with_responses(node, awake_resp, responses): mock_stick_controller.clear_responses() for r in responses: mock_stick_controller.append_response(r) await node._awake_response(awake_resp) # pylint: disable=protected-access assert node._delayed_task is not None await asyncio.wait_for(asyncio.shield(node._delayed_task), timeout=2) assert not mock_stick_controller.send_responseUsage per step with the intended sequence makes the test intent explicit and robust.
Also applies to: 2480-2487, 2507-2514, 2538-2545, 2550-2557, 2577-2587, 2602-2612, 2614-2625
2597-2599: Duplicate assertion.hysteresis_config_dirty is asserted twice back-to-back; drop one.
- assert test_sense.hysteresis_config_dirty - assert test_sense.hysteresis_config_dirty + assert test_sense.hysteresis_config_dirty
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_usb.py(20 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_usb.py (4)
plugwise_usb/messages/responses.py (3)
PlugwiseResponse(98-215)NodeAckResponse(902-924)deserialize(137-201)plugwise_usb/messages/requests.py (16)
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)PlugwiseRequest(60-335)plugwise_usb/connection/__init__.py (1)
send(227-238)plugwise_usb/nodes/sense.py (4)
report_interval(359-363)hysteresis_config_dirty(366-368)temperature_lower_bound(345-349)set_report_interval(565-588)
🔇 Additional comments (3)
tests/test_usb.py (3)
231-233: Good fix: make the response queue instance-scoped.This eliminates cross-test bleed and flakiness introduced by shared class-level state.
2304-2306: Nice: reset the queue before a new flow.This prevents cross-flow interference and improves test determinism.
2374-2376: Cache stub for report_interval looks correct.Adds coverage for the new cache key and ensures initial value is loaded as int 15.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_usb.py (1)
231-234: Instance-scoped queue: good; prefer deque for O(1) FIFO popsNice fix moving to an instance queue. To avoid O(n) per pop(0) downstream, switch to collections.deque.
- self.send_response: list[pw_responses.PlugwiseResponse] = [] + self.send_response: deque[pw_responses.PlugwiseResponse] = deque()Add import near the other imports:
from collections import deque # at top of file
🧹 Nitpick comments (6)
tests/test_usb.py (6)
253-256: Avoid name shadowing and add type on append_response()The parameter name shadows the attribute; rename and type it for clarity.
- def append_response(self, send_response) -> None: + def append_response(self, response: pw_responses.PlugwiseResponse) -> None: """Add response to queue.""" - self.send_response.append(send_response) + self.send_response.append(response)
267-269: Pop from the left and guard empty-queue cleanlyWith deque, prefer popleft() and catch IndexError. Also consider prefixing unused args with underscores to appease linters.
- if self.send_response: - return self.send_response.pop(0) - return None + try: + return self.send_response.popleft() + except IndexError: + return None
2005-2005: SED tests: deterministic multi-step config via queued responses looks goodEnqueuing per-awake responses before invoking _awake_response() makes the flow deterministic. Consider a tiny helper (e.g., enqueue_and_run_awake(responses)) to DRY the repeated pattern, similar to the Sense helper.
Also applies to: 2023-2023, 2049-2049, 2075-2075, 2096-2096, 2121-2121
2227-2227: Scan tests: good use of the response queueSame note as SED: looks correct; a small helper would reduce duplication.
Also applies to: 2241-2241, 2265-2265, 2294-2294
2418-2434: Type-hint the responses parameter and consider pluralized nameMinor: add a type hint to responses and consider naming run_awake_with_responses for clarity.
- async def run_awake_with_response(responses): + async def run_awake_with_response( + responses: list[pw_responses.PlugwiseResponse], + ) -> None:
2486-2492: Reduce duplication in ACK sequencesThese repeated three-item sequences could be factored into named constants or a small factory for readability (e.g., SEQ_ALL_ACCEPTED, SEQ_FIRST_FAIL, SEQ_RPT_FAIL).
Also applies to: 2494-2500, 2518-2524, 2546-2552, 2555-2561, 2579-2585, 2600-2605, 2609-2614
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_usb.py(20 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_usb.py (4)
plugwise_usb/messages/responses.py (4)
PlugwiseResponse(98-215)NodeAckResponse(902-924)deserialize(137-201)NodeAwakeResponse(801-833)plugwise_usb/messages/requests.py (16)
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)PlugwiseRequest(60-335)plugwise_usb/connection/__init__.py (1)
send(227-238)plugwise_usb/nodes/sense.py (4)
report_interval(359-363)hysteresis_config_dirty(366-368)temperature_lower_bound(345-349)set_report_interval(565-588)
⏰ 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: Process test coverage
🔇 Additional comments (5)
tests/test_usb.py (5)
257-260: LGTM: explicit queue reset helperclear_responses() is a useful test helper as-is.
2305-2305: Resetting the queue between flowsclear_responses() before the cached-scan scenario prevents bleed-through. Good.
2374-2376: Report-interval cache default: verify constant alignmentFake cache uses "15" and the test expects 15. Please confirm this matches DEFAULT_SENSE_HYSTERESIS_REPORT_INTERVAL in the implementation to avoid brittle tests if the default changes.
Also applies to: 2465-2465
2588-2617: Sense report-interval tests: coverage looks correctBoundary validation and dirty/ack flows are covered well. Nice.
2409-2417: ACK ids verified — tests match enum valuesConfirmed in plugwise_usb/messages/responses.py: NodeAckResponseType.SENSE_INTERVAL_ACCEPTED = b"00B3" and SENSE_INTERVAL_FAILED = b"00B4"; the test raw ids match the enum, no change required.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_usb.py (3)
231-233: Use deque for O(1) FIFO and tighten typing on the queue.The per-instance queue fixes cross-test bleed. To avoid O(n) pops and improve clarity, switch to collections.deque, add a bulk enqueue helper, and type the append argument.
Apply:
+from collections import deque +from typing import Iterable @@ class MockStickController: @@ - def __init__(self) -> None: + def __init__(self) -> None: """Initialize MockStickController.""" - self.send_response: list[pw_responses.PlugwiseResponse] = [] + self.send_response: deque[pw_responses.PlugwiseResponse] = deque() @@ - def append_response(self, response) -> None: + def append_response(self, response: pw_responses.PlugwiseResponse) -> None: """Add response to queue.""" self.send_response.append(response) + def extend_responses(self, responses: Iterable[pw_responses.PlugwiseResponse]) -> None: + """Add multiple responses to queue.""" + self.send_response.extend(responses) @@ async def send( self, request: pw_requests.PlugwiseRequest, # type: ignore[name-defined] suppress_node_errors=True, ) -> pw_responses.PlugwiseResponse | None: # type: ignore[name-defined] """Submit request to queue and return response.""" - if self.send_response: - return self.send_response.pop(0) - return None + try: + return self.send_response.popleft() + except IndexError: + return NoneNit: if suppress_node_errors is intentionally ignored here, consider naming it _suppress_node_errors to signal it's unused.
Also applies to: 253-260, 267-269
2418-2436: Nit: avoid mutating a shared NodeAwakeResponse across runs.run_awake_with_response increments the same awake_response timestamp. Create a fresh copy per call to avoid temporal coupling between tests.
Apply:
+from copy import copy @@ - async def run_awake_with_response( - responses: list[pw_responses.PlugwiseResponse], - ): + async def run_awake_with_response( + responses: list[pw_responses.PlugwiseResponse], + ): """Wake node up and run tasks.""" mock_stick_controller.clear_responses() for r in responses: mock_stick_controller.append_response(r) - awake_response.timestamp = awake_response.timestamp + td( - seconds=pw_sed.AWAKE_RETRY - ) - await test_sense._awake_response(awake_response) # pylint: disable=protected-access + ar = copy(awake_response) + ar.timestamp = ar.timestamp + td(seconds=pw_sed.AWAKE_RETRY) + await test_sense._awake_response(ar) # pylint: disable=protected-access assert test_sense._delayed_task is not None # pylint: disable=protected-access await asyncio.wait_for(asyncio.shield(test_sense._delayed_task), timeout=2)
2488-2493: Good coverage of Sense hysteresis + report_interval flows; consider parametrizing sequences.You exercise fail-first-then-accept and pure-accept paths, plus standalone report_interval failure/success. To reduce duplication and ease future additions, parameterize the response sequences and expected dirty/value outcomes.
Example sketch:
@pytest.mark.parametrize( "responses, expect_dirty, expect_ri", [ ([fail, accept, ri_accept], True, None), ([accept, accept, ri_accept], False, None), ([accept, accept, ri_fail], True, None), ([accept, accept, ri_accept], False, 5), ], ) async def test_sense_config_sequences(responses, expect_dirty, expect_ri): await run_awake_with_response(responses) assert test_sense.hysteresis_config_dirty is expect_dirty if expect_ri is not None: assert test_sense.report_interval == expect_riAlso applies to: 2497-2502, 2521-2525, 2549-2553, 2557-2562, 2581-2586, 2591-2600, 2602-2609, 2611-2619
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_usb.py(20 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: dirixmjm
PR: plugwise/python-plugwise-usb#330
File: plugwise_usb/api.py:90-90
Timestamp: 2025-09-02T11:06:20.120Z
Learning: Plugwise Sense devices push hysteresis state updates through the NODE_SWITCH_GROUP_ID mechanism. When hysteresis thresholds are crossed, the device sends NodeSwitchGroupResponse messages which are processed by the _switch_group method, updating humidity_state and temperature_state, then publishing to subscribers via NodeFeature.SENSE.
🧬 Code graph analysis (1)
tests/test_usb.py (5)
plugwise_usb/messages/responses.py (4)
PlugwiseResponse(98-215)NodeAckResponse(902-924)deserialize(137-201)NodeAwakeResponse(801-833)plugwise_usb/messages/requests.py (17)
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)PlugwiseRequest(60-335)plugwise_usb/connection/__init__.py (1)
send(227-238)plugwise_usb/nodes/sed.py (1)
_awake_response(408-465)plugwise_usb/nodes/sense.py (4)
report_interval(359-363)hysteresis_config_dirty(366-368)temperature_lower_bound(345-349)set_report_interval(565-588)
🔇 Additional comments (4)
tests/test_usb.py (4)
2005-2006: LGTM: tests now enqueue per-instance responses deterministically.Good migration to instance-scoped queue via append_response/clear_responses; this removes inter-test coupling and makes multi-step flows predictable.
Also applies to: 2023-2024, 2049-2050, 2075-2076, 2096-2097, 2121-2122, 2227-2228, 2241-2242, 2265-2266, 2294-2296, 2305-2306
2467-2468: LGTM: validates default report_interval (15) after cache load.Matches the default path when hysteresis_config.report_interval is None.
2374-2376: Confirm cache key parity for report_interval — resolved.
CACHE_SENSE_HYSTERESIS_REPORT_INTERVAL is defined in plugwise_usb/nodes/sense.py and is used by _report_interval_from_cache (load) and _set_cache (save); tests expecting 15 match.
2410-2416: ACK IDs verified — no action required.
NodeAckResponseType defines SENSE_INTERVAL_ACCEPTED = b"00B3" and SENSE_INTERVAL_FAILED = b"00B4" (plugwise_usb/messages/responses.py) and plugwise_usb/nodes/sense.py compares response.node_ack_type to these in _configure_sense_report_interval_task; tests reference the same codes.
Append report interval setting to Sense node configuration.
Extend MockStickController with a response queue to send allow sending different responses sequentially
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests
Chores