schedule clock synchronization every 3600 seconds #341
Conversation
WalkthroughAdds a background scheduler to PlugwiseCircle that periodically calls clock_synchronize() every 3600s (with jitter), starts/stores/cancels the task during init/unload, converts MAX_LOG_HOURS to a Final, changes clock-drift checks in Circle and Circle Plus to use absolute drift, and adds a comprehensive DummyTransport test harness and test data. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as App/Framework
participant PC as PlugwiseCircle
participant BG as _clock_synchronize_task
participant Dev as Circle Device
App->>PC: __init__()
alt no task or task.done()
PC->>BG: create_task(_clock_synchronize_scheduler())
end
rect rgb(245,250,255)
note over BG: Background loop — every 3600s (+/- jitter)
loop every 3600s
BG->>PC: call clock_synchronize()
PC->>Dev: perform clock sync
Dev-->>PC: ack/result
BG->>BG: sleep(CLOCK_SYNC_PERIOD + jitter)
end
end
App->>PC: unload()
alt task active
PC->>BG: cancel()
BG-->>PC: raises CancelledError / stops
PC->>PC: await gather(task, return_exceptions=True)
PC->>PC: clear _clock_synchronize_task
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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 #341 +/- ##
==========================================
+ Coverage 81.25% 81.33% +0.08%
==========================================
Files 36 36
Lines 8110 8129 +19
==========================================
+ Hits 6590 6612 +22
+ Misses 1520 1517 -3 ☔ 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: 3
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
1003-1006: Name the task for easier debugging (optional).Keeps logs and task dumps readable without changing behavior.
if self._clock_synchronize_task is None or self._clock_synchronize_task.done(): self._clock_synchronize_task = create_task( self._clock_synchronize_scheduler() ) + if hasattr(self._clock_synchronize_task, "set_name"): + self._clock_synchronize_task.set_name( + f"plugwise-clock-sync:{self._mac_in_str}" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (1)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-108)
🔇 Additional comments (1)
plugwise_usb/nodes/circle.py (1)
144-145: LGTM: dedicated field for the scheduler task.Clear ownership and type are good. No issues.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
857-865: Keep the scheduler alive on errors; preserve cancellation semantics.Unhandled exceptions in clock_synchronize() will terminate the loop; also re‑raise CancelledError after logging to keep true cancellation semantics.
Apply:
async def _clock_synchronize_scheduler(self) -> None: """Background task: periodically synchronize the clock until cancelled.""" try: while True: await sleep(60) - await self.clock_synchronize() + try: + await self.clock_synchronize() + except Exception: + _LOGGER.exception( + "Clock synchronization failed for %s", self._mac_in_str + ) except CancelledError: - _LOGGER.debug("Clock sync scheduler cancelled for %s", self.name) + _LOGGER.debug("Clock sync scheduler cancelled for %s", self._mac_in_str) + raiseOptional: add small jitter (±5s) to the sleep to avoid aligned bursts when many nodes start together.
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
1006-1009: Optionally name the task for easier debugging.Helps tracing in logs and debuggers.
- self._clock_synchronize_task = create_task( - self._clock_synchronize_scheduler() - ) + t = create_task(self._clock_synchronize_scheduler()) + if hasattr(t, "set_name"): + t.set_name(f"clock-sync:{self._mac_in_str}") + self._clock_synchronize_task = t
1100-1104: Good: cancel, await, and clear the clock-sync task.Prevents “Task exception was never retrieved” and leaks.
For consistency, consider draining and clearing _retrieve_energy_logs_task above as well:
if self._retrieve_energy_logs_task and not self._retrieve_energy_logs_task.done(): self._retrieve_energy_logs_task.cancel() await gather(self._retrieve_energy_logs_task, return_exceptions=True) self._retrieve_energy_logs_task = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (2)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-108)plugwise_usb/nodes/node.py (1)
name(218-222)
🔇 Additional comments (2)
plugwise_usb/nodes/circle.py (2)
5-5: Good: asyncio imports updated.Including CancelledError, Task, create_task, gather, sleep is correct for the new scheduler and shutdown path.
144-145: Good: explicit task handle for the scheduler.Typed as Task[None] | None and initialized in init, preventing attribute‑errors later.
CR: Make the scheduler resilient: fix return type, handle cancellation, and keep the loop alive on errors. CR: Import CancelledError for proper task shutdown handling.
dirixmjm
changed the title
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/circle.py (1)
885-889: Bug: timedelta.seconds ignores sign — negative drifts never sync.
Using timedelta.seconds drops the sign; the second clause is always true. Use abs(total_seconds()).- if (clock_offset.seconds < MAX_TIME_DRIFT) or ( - clock_offset.seconds > -(MAX_TIME_DRIFT) - ): + if abs(clock_offset.total_seconds()) < MAX_TIME_DRIFT: return True
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle.py (3)
859-871: Resilient background loop — add tiny jitter and unify log context.
Loop/cancellation are solid. To avoid thundering-herd effects, add ±5s jitter; also prefer consistent id in logs.- await sleep(CLOCK_SYNC_PERIOD) + await sleep(CLOCK_SYNC_PERIOD) try: - await self.clock_synchronize() + await self.clock_synchronize() except Exception: - _LOGGER.exception("Clock synchronisation failed for %s", self.name) + _LOGGER.exception("Clock synchronization failed for %s", self._mac_in_str) except CancelledError: - _LOGGER.debug("Clock sync scheduler cancelled for %s", self.name) + _LOGGER.debug("Clock sync scheduler cancelled for %s", self._mac_in_str) raiseOptional jitter (outside this hunk):
# add at top-level imports from random import uniform # replace the sleep line: await sleep(CLOCK_SYNC_PERIOD + uniform(-5, 5))
1012-1015: Start the scheduler only after successful initialize.
Currently the task starts before later checks (e.g., calibration) that can fail, leaving a background task running when initialize() returns False.Apply within this hunk:
- if self._clock_synchronize_task is None or self._clock_synchronize_task.done(): - self._clock_synchronize_task = create_task( - self._clock_synchronize_scheduler() - )Then add just before
return True(afterawait super().initialize()):if self._clock_synchronize_task is None or self._clock_synchronize_task.done(): self._clock_synchronize_task = create_task(self._clock_synchronize_scheduler())
1106-1110: Graceful cancellation and draining — good.
Cancel + gather + clear avoids “Task exception was never retrieved.”Mirror this pattern for
_retrieve_energy_logs_taskabove to drain its cancellation as well:if self._retrieve_energy_logs_task is not None and not self._retrieve_energy_logs_task.done(): self._retrieve_energy_logs_task.cancel() await gather(self._retrieve_energy_logs_task, return_exceptions=True) self._retrieve_energy_logs_task = None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (1)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-108)
⏰ 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 (3)
plugwise_usb/nodes/circle.py (3)
5-5: Asyncio imports updated — good.
Importing CancelledError and friends is correct and matches the shutdown handling.
146-148: Scheduler task field initialization — OK.
Clear None-or-Task typing; fits later lifecycle calls.
77-80: Align CLOCK_SYNC_PERIOD with PR objective (60s vs 3600s).PR title/objective says "every 60 seconds" but the code sets CLOCK_SYNC_PERIOD = 3600 — confirm intended cadence and change if 60s is correct.
File: plugwise_usb/nodes/circle.py (lines 77–80)
-CLOCK_SYNC_PERIOD: Final = 3600 +CLOCK_SYNC_PERIOD: Final = 60 # seconds
negative offsets.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
859-871: Fix AttributeError: use _mac_in_str in logs inside the scheduler.
self.mac_in_strdoesn’t exist; this would raise at runtime and kill the loop.Apply:
- _LOGGER.exception("Clock synchronisation failed for %s", self.mac_in_str) + _LOGGER.exception("Clock synchronisation failed for %s", self._mac_in_str) @@ - _LOGGER.debug("Clock sync scheduler cancelled for %s", self.mac_in_str) + _LOGGER.debug("Clock sync scheduler cancelled for %s", self._mac_in_str)Optional: add small jitter (±5s) to
sleep(CLOCK_SYNC_PERIOD)to avoid synchronized bursts when many nodes start together.
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
889-897: Log the correct drift magnitude and fix a typo in the error message.
- Use
abs(total_seconds())for drift logging (current.secondscan misreport negatives).- Typo: “en” → “when”.
Apply:
- _LOGGER.info( - "Reset clock of node %s because time has drifted %s sec", - self._mac_in_str, - str(clock_offset.seconds), - ) + _LOGGER.info( + "Reset clock of node %s because time has drifted %s sec", + self._mac_in_str, + str(int(abs(clock_offset.total_seconds()))), + ) @@ - raise NodeError( - "Unable to synchronize clock en when protocol version is unknown" - ) + raise NodeError( + "Unable to synchronize clock when protocol version is unknown" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/nodes/circle.py(7 hunks)plugwise_usb/nodes/circle_plus.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle_plus.pyplugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (1)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-106)
🔇 Additional comments (7)
plugwise_usb/nodes/circle_plus.py (1)
89-96: Absolute drift check is good; tweak logging for correctness and remove stray parenthesis.
clock_offset.secondsis misleading for negative offsets; preferabs(int(clock_offset.total_seconds())). Also the log message ends with an extra “)”.
[ suggest_nitpick_refactor ]
Apply:- _LOGGER.info( - "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds)", - self._node_info.mac, - str(clock_offset.seconds), - str(MAX_TIME_DRIFT), - ) + _LOGGER.info( + "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds", + self._node_info.mac, + str(int(abs(clock_offset.total_seconds()))), + str(MAX_TIME_DRIFT), + )plugwise_usb/nodes/circle.py (6)
5-5: Importing CancelledError for clean shutdown — LGTM.
77-80: New Finals (MAX_LOG_HOURS,CLOCK_SYNC_PERIOD) — LGTM.
146-148: Scheduler task field initialization — LGTM.
886-887: Absolute drift comparison — LGTM.
1010-1013: Starting the background scheduler after initial sync — LGTM.
1104-1108: Task cancellation is properly drained — LGTM.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
plugwise_usb/nodes/circle.py (4)
859-871: Add small jitter and unify spelling in log message.Prevents synchronized bursts across many nodes and keeps spelling consistent ("synchronization"). Behavior unchanged otherwise.
Apply:
@@ - while True: - await sleep(CLOCK_SYNC_PERIOD) + while True: + # add small jitter to avoid thundering herd when many nodes start together + await sleep(CLOCK_SYNC_PERIOD + (random.uniform(-5, 5))) @@ - except Exception: - _LOGGER.exception("Clock synchronisation failed for %s", self._mac_in_str) + except Exception: + _LOGGER.exception("Clock synchronization failed for %s", self._mac_in_str)Also add import:
import logging +import random from math import ceil
886-892: Nit: align info log with CirclePlus and include max drift.Keeps logs consistent and more informative.
- _LOGGER.info( - "Reset clock of node %s because time has drifted %s sec", - self._mac_in_str, - str(int(abs(clock_offset.total_seconds()))), - ) + _LOGGER.info( + "Reset realtime clock of node %s because time drifted %s seconds (max %s seconds)", + self._mac_in_str, + str(int(abs(clock_offset.total_seconds()))), + str(MAX_TIME_DRIFT), + )
893-897: Typo in error message.Remove stray “en”.
- raise NodeError( - "Unable to synchronize clock en when protocol version is unknown" - ) + raise NodeError( + "Unable to synchronize clock when protocol version is unknown" + )
1010-1013: Name the task for easier debugging (optional).A task name helps tracing in logs and debuggers.
- self._clock_synchronize_task = create_task( - self._clock_synchronize_scheduler() - ) + self._clock_synchronize_task = create_task( + self._clock_synchronize_scheduler(), + name=f"clock-sync-{self._mac_in_str}", + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/nodes/circle.py(7 hunks)plugwise_usb/nodes/circle_plus.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- plugwise_usb/nodes/circle_plus.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (1)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-106)
🔇 Additional comments (5)
plugwise_usb/nodes/circle.py (5)
5-5: Good: proper asyncio imports for task lifecycle.Includes CancelledError, sleep, gather; consistent with usage below.
77-78: Const correctness looks good.Keeping MAX_LOG_HOURS as Final preserves semantics.
79-79: Clock sync period defined clearly.Defaulting to 3600s matches the PR objective.
146-147: Task handle field added and typed.Clean addition; avoids stray tasks across reloads.
1104-1108: Good: cancel, await, and clear the scheduler task.Drains cancellation and avoids “Task exception was never retrieved”.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/network/cache.py(2 hunks)plugwise_usb/nodes/circle.py(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugwise_usb/network/cache.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (1)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-106)
🪛 GitHub Actions: Latest commit
plugwise_usb/nodes/circle.py
[error] 863-863: F821 Undefined name random.
🔇 Additional comments (6)
plugwise_usb/nodes/circle.py (6)
5-5: Good: imported CancelledError for graceful task shutdown.
This aligns with the cancellation handling in the scheduler and unload.
77-80: Constants look good and typed as Final.
MAX_LOG_HOURS aliasing and CLOCK_SYNC_PERIOD=3600 are clear.
146-148: Task holder initialization is correct and typed.
This prevents multiple concurrent schedulers.
888-895: Correct: use absolute drift for comparison and clearer log.
This avoids sign sensitivity on early/late clock differences.
1013-1016: Scheduler startup placement is fine.
Guarding with “is None or done()” avoids duplicates.
1107-1111: Good: cancel, await, and clear the scheduler task.
Prevents “Task exception was never retrieved.”
|
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/circle.py (1)
624-647: Fix slots_empty semantics in energy_log_update (outdated records wrongly mark address as “updated”).cache_updated is set even when a non-empty record is outdated and skipped, which makes slots_empty False while no recent data was stored. This breaks the contract (“True when all four slots are empty or outdated”) and prevents early stop in _get_initial_energy_logs, causing unnecessary backfill requests.
Apply:
@@ - for _slot in range(4, 0, -1): + for _slot in range(4, 0, -1): log_timestamp, log_pulses = response.log_data[_slot] _LOGGER.debug( "In slot=%s: pulses=%s, timestamp=%s", _slot, log_pulses, log_timestamp ) - address = response.log_address + address = response.log_address if log_timestamp is None or log_pulses is None: self._energy_counters.add_empty_log(address, _slot) else: log_timestamp = log_timestamp.replace(tzinfo=UTC) - if self._check_timestamp_is_recent(address, _slot, log_timestamp): - self._energy_counters.add_pulse_log( - address, - _slot, - log_timestamp, - log_pulses, - import_only=True, - ) - cache_updated = True + if self._check_timestamp_is_recent(address, _slot, log_timestamp): + self._energy_counters.add_pulse_log( + address, + _slot, + log_timestamp, + log_pulses, + import_only=True, + ) + cache_updated = True
🧹 Nitpick comments (5)
plugwise_usb/nodes/circle.py (3)
624-631: Tiny readability nit: don’t shadow the address parameter.Rename the inner address variable to avoid confusion.
- address = response.log_address - if log_timestamp is None or log_pulses is None: - self._energy_counters.add_empty_log(address, _slot) + log_address = response.log_address + if log_timestamp is None or log_pulses is None: + self._energy_counters.add_empty_log(log_address, _slot) @@ - self._energy_counters.add_pulse_log( - address, + self._energy_counters.add_pulse_log( + log_address,
1098-1104: Drain and clear _retrieve_energy_logs_task on unload.You cancel but don’t await; this can leak “Task exception was never retrieved”.
- if ( - self._retrieve_energy_logs_task is not None - and not self._retrieve_energy_logs_task.done() - ): - self._retrieve_energy_logs_task.cancel() + if self._retrieve_energy_logs_task: + self._retrieve_energy_logs_task.cancel() + await gather(self._retrieve_energy_logs_task, return_exceptions=True) + self._retrieve_energy_logs_task = None
80-81: Optional: single source of truth for MAX_LOG_HOURS.Consider importing the limit from the pulses/helper module (or constants) to avoid divergent behavior between modules and tests that monkeypatch the pulses value.
tests/stick_test_data.py (1)
62-73: Simplify seq-id formatting.Use zfill for clarity and micro-efficiency.
- temp_str = str(hex(temp_int)).lstrip("0x").upper() - while len(temp_str) < 4: - temp_str = "0" + temp_str - return temp_str.encode() + return f"{temp_int:04X}".encode()tests/test_usb.py (1)
795-802: Match the patched method’s signature.get_missing_energy_logs is async and takes only self; align the fake to avoid confusion and future regressions.
- async def fake_get_missing_energy_logs(address: int) -> None: + async def fake_get_missing_energy_logs(self) -> None: """Mock missing energy logs.""" # no-op
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise_usb/nodes/circle.py(7 hunks)tests/stick_test_data.py(2 hunks)tests/test_usb.py(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (3)
tests/test_usb.py (1)
tests/stick_test_data.py (2)
DummyTransport(39-142)construct_message(28-36)
plugwise_usb/nodes/circle.py (2)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-106)plugwise_usb/exceptions.py (1)
NodeError(24-25)
tests/stick_test_data.py (3)
plugwise_usb/messages/requests.py (3)
seq_id(131-133)seq_id(136-145)response(124-128)plugwise_usb/messages/__init__.py (2)
seq_id(38-40)seq_id(43-45)plugwise_usb/connection/receiver.py (1)
close(157-161)
🔇 Additional comments (10)
plugwise_usb/nodes/circle.py (5)
5-11: Imports for task cancellation and jitter look good.Adding CancelledError and random unblocks the scheduler and fixes the F821.
78-81: Constants introduced are sensible defaults.Final-annotating MAX_LOG_HOURS and adding CLOCK_SYNC_PERIOD=3600 fit the PR goal.
147-149: Good: explicit task slot for the scheduler.The dedicated _clock_synchronize_task field simplifies lifecycle management.
860-873: Scheduler is resilient and cancellation-safe.Looping with jitter, exception logging, and explicit CancelledError handling are correct.
1014-1017: Starting the scheduler after initial sync is correct.Creates the task only once or when a prior one completed.
tests/stick_test_data.py (2)
28-36: Message constructor looks correct (CRC and framing).
109-123: Transport response helpers simulate fragmentation well.The delayed and immediate paths cover typical I/O behaviors.
Also applies to: 124-139
tests/test_usb.py (3)
54-55: Type annotation update to DummyTransport is correct.
73-79: Return type updated appropriately.
1854-1859: Good migration to shared test data utilities.Centralizing construct_message and DummyTransport reduces duplication and brittleness across tests.
Also applies to: 1879-1880, 1891-1893, 1920-1921, 1945-1947, 1967-1968, 1992-1993, 2056-2061, 2101-2102, 2112-2113, 2165-2166
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation