Implement energy device clock synchronization #345
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a jittered hourly background clock-synchronization scheduler for Circle nodes, centralizes Circle+ clock-set flow with ACK validation, makes test timestamps deterministic (fixed_time and freeze_time), bumps project version to 0.47.0, updates pytest logging comments, and adds a CHANGELOG entry for hourly clock sync. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Host as Host App
participant Circle as PlugwiseCircle
participant Scheduler as _clock_synchronize_scheduler
participant Device as Circle Device
Host->>Circle: initialize()
Circle->>Circle: create task(_clock_synchronize_scheduler)
rect rgba(228,246,255,0.35)
loop Every CLOCK_SYNC_PERIOD ± jitter
Scheduler->>Circle: clock_synchronize()
Circle->>Device: CircleClockGetRequest
Device-->>Circle: date/time response
Circle->>Circle: compute circle_timestamp and drift (abs total_seconds)
alt |drift| >= MAX_TIME_DRIFT
Circle->>Device: CircleClockSetRequest
Device-->>Circle: ACK (accepted/rejected)
else
Circle-->>Scheduler: no-op
end
end
end
Host->>Circle: unload()
Circle->>Scheduler: cancel and await task
sequenceDiagram
autonumber
participant CPlus as PlugwiseCirclePlus
participant Dev as Circle+ Device
CPlus->>Dev: CirclePlusRealTimeClockGetRequest
Dev-->>CPlus: date/time response
CPlus->>CPlus: derive offsets and drift
alt Drift detected or missing date
CPlus->>CPlus: _send_clock_set_req()
CPlus->>Dev: CirclePlusRealTimeClockSetRequest
Dev-->>CPlus: ACK (CLOCK_ACCEPTED?)
CPlus-->>CPlus: return True/False
else
CPlus-->>CPlus: return True
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![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
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)
623-651: slots_empty/cache_updated semantics bug: outdated records mark address as “not empty”cache_updated is set True for any non-None pulses, even when the record is outdated and skipped. This incorrectly forces slots_empty=False, impacting initial collection and save decisions.
Apply this diff:
- cache_updated = False + cache_updated = False + has_recent = False 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 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 + has_recent = True + cache_updated = True @@ - self._energy_counters.update() - if cache_updated: - slots_empty = False + self._energy_counters.update() + if cache_updated: + slots_empty = not has_recent await self._energy_log_records_save_to_cache() if save_cache: _LOGGER.debug("Saving energy cache for %s", self._mac_in_str) await self.save_cache() return result, slots_empty
🧹 Nitpick comments (5)
scripts/tests_and_coverage.sh (1)
60-61: Keep a coverage path for CI or an opt-in flagDropping coverage entirely may hide regressions. Suggest gating coverage when running in CI or via an explicit arg.
Apply this diff:
- # PYTHONPATH=$(pwd) pytest -qx tests/ --cov='.' --no-cov-on-fail --cov-report term-missing || - PYTHONPATH=$(pwd) pytest -xrpP --log-level debug tests/ + if [ "${1:-}" = "coverage" ] || [ -n "${GITHUB_ACTIONS:-}" ]; then + PYTHONPATH=$(pwd) pytest -qx tests/ --cov='.' --no-cov-on-fail --cov-report term-missing + else + PYTHONPATH=$(pwd) pytest -xrpP --log-level debug tests/ + fitests/test_usb.py (1)
907-909: Drop commented decorator or add a noteThe commented freeze_time line adds noise. Remove it or add a brief comment explaining why time isn’t frozen here.
plugwise_usb/nodes/circle_plus.py (1)
88-90: Remove debug noise (“HOI …” logs)These look like leftover local debug. Drop them or rephrase to meaningful context-specific messages.
Apply this diff:
- _LOGGER.debug("HOI circle+ clock=%s", clock_response.timestamp.replace(microsecond=0)) - _LOGGER.debug("HOI _dt_of_circle=%s", _dt_of_circle) + _LOGGER.debug( + "Circle+ RTC: device=%s, host=%s", + clock_response.timestamp.replace(microsecond=0), + _dt_of_circle, + )plugwise_usb/nodes/circle.py (2)
1026-1030: Incorrect skip_update conditionYou call node_info_update() when skip_update() is True (i.e., when you intended to skip). Invert the condition.
Apply this diff:
- if ( - self.skip_update(self._node_info, 30) - and await self.node_info_update() is None - ): + if ( + not self.skip_update(self._node_info, 30) + and await self.node_info_update() is None + ):
897-901: Minor wording fix in error messageTypo “en” → “and”.
- 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 (7)
CHANGELOG.md(1 hunks)plugwise_usb/network/cache.py(2 hunks)plugwise_usb/nodes/circle.py(7 hunks)plugwise_usb/nodes/circle_plus.py(1 hunks)scripts/tests_and_coverage.sh(1 hunks)tests/stick_test_data.py(1 hunks)tests/test_usb.py(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-30T10:46:42.649Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.649Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/network/cache.py
📚 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.py
🧬 Code graph analysis (3)
plugwise_usb/network/cache.py (1)
plugwise_usb/helpers/cache.py (1)
write_cache(82-121)
plugwise_usb/nodes/circle_plus.py (1)
plugwise_usb/nodes/node.py (1)
mac(235-237)
plugwise_usb/nodes/circle.py (1)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-108)
🔇 Additional comments (5)
CHANGELOG.md (1)
5-5: Confirm PR reference and scopeEntry mentions PR 341 while this PR is 345. Please verify the correct PR link/number and whether the scheduler change belongs under this PR or 341.
plugwise_usb/network/cache.py (1)
33-35: Formatting-only change looks goodMulti-line formatting for write_cache and warning call preserves behavior.
Also applies to: 59-61
tests/stick_test_data.py (1)
599-607: Validate encoded weekday and payload against 2025‑04‑03The fixed RTC payload encodes weekday and timestamp components. Please double‑check the weekday value for 2025-04-03 and that the set‑clock request bytes match what CirclePlusRealTimeClockSetRequest generates with the frozen time (22:00:30 UTC); CRC must match too.
Also applies to: 608-612
plugwise_usb/nodes/circle.py (1)
860-874: Scheduler lifecycle and cancellation LGTMPeriodic sync with jitter and graceful cancellation on unload are implemented correctly.
Ensure no duplicate tasks are created across re-initializations; your guard with
.done()covers this.Also applies to: 1108-1112
tests/test_usb.py (1)
778-780: Confirm freezegun >= 1.4.0 or avoid real_asyncioreal_asyncio was added in freezegun 1.4.0 — ensure the repo pins freezegun >= 1.4.0 or upgrade it; otherwise replace real_asyncio with auto_tick_seconds or another approach. File: tests/test_usb.py lines 778–780
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #345 +/- ##
==========================================
+ Coverage 81.78% 82.14% +0.36%
==========================================
Files 36 36
Lines 8140 8171 +31
==========================================
+ Hits 6657 6712 +55
+ Misses 1483 1459 -24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
plugwise_usb/nodes/circle_plus.py (6)
89-90: Tidy up debug messages"HOI" looks like a leftover. Make the messages meaningful.
- _LOGGER.debug("HOI circle+ clock=%s", clock_response.timestamp) - _LOGGER.debug("HOI _dt_req_to_circle=%s", _dt_req_to_circle) + _LOGGER.debug("Circle+ reported clock=%s", clock_response.timestamp) + _LOGGER.debug("Local request timestamp=%s", _dt_req_to_circle)
91-93: Align precision on both sides of the subtractionOnly truncating the device timestamp to seconds biases the offset. Truncate both (or neither) for a fair comparison.
- clock_offset = ( - clock_response.timestamp.replace(microsecond=0) - _dt_req_to_circle - ) + clock_offset = ( + clock_response.timestamp.replace(microsecond=0) + - _dt_req_to_circle.replace(microsecond=0) + )
96-101: Fix log typo and prefer rounded seconds; use self.mac for consistencyRemoves stray “)” and avoids flooring via int(); also prefer the public mac string.
- _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), - ) + _LOGGER.info( + "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds", + self.mac, + f"{round(abs(clock_offset.total_seconds())):.0f}", + str(MAX_TIME_DRIFT), + )
82-88: Remove commented-out codeDead/commented code adds noise; rely on git history.
- # _dt_req_to_circle: datetime = datetime.now(tz=UTC).replace( - # hour=clock_response.time.value.hour, - # minute=clock_response.time.value.minute, - # second=clock_response.time.value.second, - # microsecond=0, - # tzinfo=UTC, - # )
70-75: Optional: compensate for RTT with midpoint timestampUsing send-time slightly overestimates drift by ~RTT. Consider midpoint estimation using a monotonic clock.
Example (outside exact hunk; requires minor imports):
# at top: from datetime import UTC, datetime, timedelta from time import monotonic # before sending: t0 = monotonic() dt0 = datetime.now(tz=UTC) # after receiving: t1 = monotonic() dt_mid = dt0 + timedelta(seconds=(t1 - t0) / 2) # then compute offset (with matching precision policy): clock_offset = clock_response.timestamp.replace(microsecond=0) - dt_mid.replace(microsecond=0)
103-106: Optional: no functional change required — microseconds are ignored
CirclePlusRealTimeClockSetRequest only uses dt.hour/dt.minute/dt.second (plugwise_usb/messages/requests.py), so passing datetime with microseconds has no effect; using .replace(microsecond=0) is purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise_usb/nodes/circle_plus.py(2 hunks)tests/stick_test_data.py(1 hunks)tests/test_usb.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_usb.py
- tests/stick_test_data.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_plus.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle_plus.py (1)
plugwise_usb/nodes/node.py (1)
mac(235-237)
⏰ 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 (1)
plugwise_usb/nodes/circle_plus.py (1)
94-95: Good fix: use total_seconds()Switching from .seconds to .total_seconds() avoids day-boundary pitfalls.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle_plus.py (3)
73-76: Fix stale log string (method name)Log references async_realtime_clock_synchronize(), but the method is clock_synchronize().
- "No response for async_realtime_clock_synchronize() for %s", self.mac + "No response for clock_synchronize() for %s", self.mac
95-100: Nit: stray closing parenthesis in info logMinor textual fix for readability.
- _LOGGER.info( - "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds)", + _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), )
101-103: Optional: normalize microseconds when setting RTCNot functionally required (seconds precision), but aligning to whole seconds avoids tiny display diffs in logs/tests.
- set_request = CirclePlusRealTimeClockSetRequest( - self._send, self._mac_in_bytes, datetime.now(tz=UTC) - ) + set_request = CirclePlusRealTimeClockSetRequest( + self._send, self._mac_in_bytes, datetime.now(tz=UTC).replace(microsecond=0) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise_usb/nodes/circle_plus.py(1 hunks)tests/stick_test_data.py(2 hunks)tests/test_usb.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/stick_test_data.py
- tests/test_usb.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_plus.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle_plus.py (3)
plugwise_usb/messages/requests.py (19)
CirclePlusRealTimeClockGetRequest(973-992)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)CirclePlusRealTimeClockSetRequest(938-970)plugwise_usb/nodes/node.py (2)
mac(235-237)_available_update_state(422-450)plugwise_usb/messages/properties.py (13)
value(76-80)value(96-100)value(125-129)value(159-163)value(187-191)value(207-211)value(251-255)value(276-280)value(301-305)value(330-334)value(359-363)value(380-384)value(403-407)
🔇 Additional comments (2)
plugwise_usb/nodes/circle_plus.py (2)
70-74: Clock GET request path looks goodUsing the typed request and walrus assignment is clear and correct.
79-79: Good: availability timestamp from responseFeeding response.timestamp into _available_update_state aligns last_seen with observed traffic.
bouwew
force-pushed
the
test_bouwew
branch
2 times, most recently
from
db25983 to
1984c84
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/stick_test_data.py (1)
10-12: Unused utc_now_offset; remove or useThis variable is never referenced.
Apply:
-utc_now_offset = datetime.now(tz=UTC).replace(tzinfo=UTC) + timedelta( - days=1, hours=4, seconds=30 -)plugwise_usb/nodes/circle_plus.py (1)
93-98: Trailing parenthesis in log messageExtra “)” at end of the format string.
Apply:
- "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds)", + "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)plugwise_usb/nodes/circle.py(6 hunks)plugwise_usb/nodes/circle_plus.py(1 hunks)plugwise_usb/nodes/node.py(1 hunks)tests/stick_test_data.py(3 hunks)tests/test_usb.py(2 hunks)
✅ Files skipped from review due to trivial changes (2)
- plugwise_usb/nodes/node.py
- tests/test_usb.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
🧰 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 (2)
plugwise_usb/nodes/circle_plus.py (3)
plugwise_usb/messages/requests.py (19)
CirclePlusRealTimeClockGetRequest(970-989)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(501-510)send(531-542)send(555-564)send(577-586)send(610-619)send(656-665)send(708-717)send(764-773)send(797-806)send(837-846)send(869-878)send(901-910)CirclePlusRealTimeClockSetRequest(935-967)plugwise_usb/nodes/node.py (2)
mac(235-237)_available_update_state(423-451)plugwise_usb/messages/properties.py (13)
value(76-80)value(96-100)value(125-129)value(159-163)value(187-191)value(207-211)value(251-255)value(276-280)value(301-305)value(330-334)value(359-363)value(380-384)value(403-407)
plugwise_usb/nodes/circle.py (5)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-108)plugwise_usb/messages/requests.py (19)
CircleClockGetRequest(998-1017)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(501-510)send(531-542)send(555-564)send(577-586)send(610-619)send(656-665)send(708-717)send(764-773)send(797-806)send(837-846)send(869-878)send(901-910)CircleClockSetRequest(720-773)plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/messages/responses.py (2)
ack_id(134-136)NodeResponseType(56-73)plugwise_usb/nodes/node.py (1)
name(218-222)
🪛 GitHub Actions: Latest commit
plugwise_usb/nodes/circle.py
[error] 884-884: Ruff: F841 Local variable 'circle_plus_timestamp' is assigned to but never used. Remove the assignment.
[error] 892-892: Ruff: F821 Undefined name 'circle_timestamp'.
🔇 Additional comments (11)
tests/stick_test_data.py (2)
603-609: Day-of-week/date encoding: confirm protocol expects decimal ASCII, and weekday baseCircle+ RTC fields are emitted with decimal formatting (%02d). Circle (003E/003F) uses hex (%02X). Please confirm the Circle+ response is indeed decimal ASCII and that weekday=0..6 (Mon=0) matches device expectations; some stacks use Sun=0 or 1..7.
Would you like a quick patch to normalize weekday mapping if needed?
611-615: ACK payload for set-clock looks goodACK id 00D7 and MAC placement match NodeResponseType.CLOCK_ACCEPTED.
plugwise_usb/nodes/circle.py (7)
5-5: Imports look goodAdded asyncio primitives and Task typing; needed for the scheduler.
12-12: Random jitter import OKJitter reduces thundering herd across nodes.
78-81: Constants promoted to FinalGood for readability and static checking.
147-149: New task handle for schedulerSolid lifecycle hook for periodic sync.
860-874: Background schedulerLoop + jitter + cancellation handling looks correct.
1018-1021: Starting scheduler on initGood to guard with None/done to avoid duplicates.
1112-1116: Clean shutdown of schedulerCancel + gather + None reset — correct.
plugwise_usb/nodes/circle_plus.py (2)
79-89: Clock drift calc can be wrong near midnight and on month boundariesReplacing day with (today.day + days_diff) can overflow the month and mis-pick dates around 00:00, causing false >23h offsets.
Use nearest-of-three candidates around the response timestamp:
-from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta @@ - dt_now = datetime.now(tz=UTC) - days_diff = (response.day_of_week.value - dt_now.weekday()) % 7 - circle_plus_timestamp: datetime = dt_now.replace( - day=(dt_now.day + days_diff), - hour=response.time.value.hour, - minute=response.time.value.minute, - second=response.time.value.second, - microsecond=0, - tzinfo=UTC, - ) - clock_offset = response.timestamp.replace(microsecond=0) - circle_plus_timestamp + base_ts = response.timestamp.replace(microsecond=0) + reported_time = response.time.value # datetime.time + base_date = base_ts.date() + candidates = [ + datetime.combine(base_date - timedelta(days=1), reported_time, tzinfo=UTC), + datetime.combine(base_date, reported_time, tzinfo=UTC), + datetime.combine(base_date + timedelta(days=1), reported_time, tzinfo=UTC), + ] + device_dt = min(candidates, key=lambda d: abs((base_ts - d).total_seconds())) + clock_offset = base_ts - device_dt
100-103: Cast ACK to enum for reliable comparisonAlign with enable_auto_join to avoid raw-bytes/int equality pitfalls.
- if (node_response := await set_request.send()) is not None: - return node_response.ack_id == NodeResponseType.CLOCK_ACCEPTED + if (node_response := await set_request.send()) is not None: + return NodeResponseType(node_response.ack_id) == NodeResponseType.CLOCK_ACCEPTED
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/stick_test_data.py (2)
610-614: Replace hard‑coded 0x0028 timestamped request key with a partial‑match entryThe exact key at tests/stick_test_data.py:610–614 encodes a fixed timestamp and will only match that one payload; other clock tests use prefix‑based PARTLY_RESPONSE_MESSAGES (see the "Clock set" entries around 1373–1392) and responses.py contains CLOCK_ACCEPTED = b"00D7". Remove the full, timestamped entry and add a PARTLY_RESPONSE_MESSAGES prefix entry for 0x0028+MAC.
- b"\x05\x05\x03\x030028009876543210123452441406210925D23C\r\n": ( - "Circle+ Realtime set clock for 0098765432101234", - b"000000C1", # Success ack - b"0000" + b"00D7" + b"0098765432101234", # msg_id, clock_ack, mac - ),Add (inside PARTLY_RESPONSE_MESSAGES or immediately after it):
# Accept any payload/time for Circle+ set-clock requests PARTLY_RESPONSE_MESSAGES[b"\x05\x05\x03\x0300280098765432101234"] = ( "Clock set 0098765432101234", b"000000C1", b"0000" + b"00D7" + b"0098765432101234", )
10-12: Avoid clock skew: derive utc_now_offset from utc_now and drop redundant tz replace.Derive utc_now_offset from the existing utc_now to avoid two datetime.now() calls; .replace(tzinfo=UTC) is redundant. File: tests/stick_test_data.py (lines 9–12).
-utc_now_offset = datetime.now(tz=UTC).replace(tzinfo=UTC) + timedelta( - days=1, hours=4, seconds=30 -) +utc_now_offset = utc_now + timedelta(days=1, hours=4, seconds=30)
- utc_now_offset appears unused in the file — consider removing it if not needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
pyproject.toml (1)
543-544: Make logs visible during test run (enable live logging).You set log_format/date_format, but without enabling log_cli the logs won’t stream for passing tests. Either configure pytest to show logs live or pass flags from the script.
Option A (config-only):
[tool.pytest.ini_options] testpaths = [ "tests", ] asyncio_default_fixture_loop_scope = "session" asyncio_mode = "strict" log_format = "%(asctime)s %(levelname)s %(message)s" log_date_format = "%Y-%m-%d %H:%M:%S" +log_cli = true +# Let the script control the level via --log-cli-level +# log_cli_level = "INFO"scripts/tests_and_coverage.sh (1)
60-61: Stream logs to console and fix stale comment.
- The comment mentions “rerun with debug if failures” but we no longer rerun; also we already run at debug.
- To actually see logs live, prefer enabling CLI logging (current --log-level only sets capture threshold).
Suggest:
- # PYTHONPATH=$(pwd) pytest -qx tests/ --cov='.' --no-cov-on-fail --cov-report term-missing || - PYTHONPATH=$(pwd) pytest -xrpP --log-level debug tests/ + # Single-shot run (no coverage). Use live logging for easier debugging. + PYTHONPATH=$(pwd) pytest -xrpP -o log_cli=true --log-level=DEBUG --log-cli-level=DEBUG tests/Optional: drop -x if you want a full failure list locally; keep -x for fast‑fail in CI.
plugwise_usb/nodes/circle.py (1)
879-882: Optional: update availability/last_seen on successful GETMirror Circle+ behavior to keep availability fresh.
response = await request.send() if response is None or response.timestamp is None: return False + await self._available_update_state(True, response.timestamp)plugwise_usb/nodes/circle_plus.py (1)
79-97: Optional: widen jitter/period desync to reduce fleet thundering herdIf many nodes start together, consider adding a larger initial random delay or wider jitter (e.g., ±300s).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CHANGELOG.md(1 hunks)plugwise_usb/nodes/circle.py(6 hunks)plugwise_usb/nodes/circle_plus.py(1 hunks)pyproject.toml(1 hunks)scripts/tests_and_coverage.sh(1 hunks)tests/stick_test_data.py(2 hunks)tests/test_usb.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_usb.py
- CHANGELOG.md
- tests/stick_test_data.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_plus.pyplugwise_usb/nodes/circle.py
🧬 Code graph analysis (2)
plugwise_usb/nodes/circle_plus.py (3)
plugwise_usb/messages/requests.py (19)
CirclePlusRealTimeClockGetRequest(970-989)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(501-510)send(531-542)send(555-564)send(577-586)send(610-619)send(656-665)send(708-717)send(764-773)send(797-806)send(837-846)send(869-878)send(901-910)CirclePlusRealTimeClockSetRequest(935-967)plugwise_usb/nodes/node.py (2)
mac(235-237)_available_update_state(423-451)plugwise_usb/messages/properties.py (13)
value(76-80)value(96-100)value(125-129)value(159-163)value(187-191)value(207-211)value(251-255)value(276-280)value(301-305)value(330-334)value(359-363)value(380-384)value(403-407)
plugwise_usb/nodes/circle.py (5)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-112)plugwise_usb/messages/requests.py (19)
CircleClockGetRequest(998-1017)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(501-510)send(531-542)send(555-564)send(577-586)send(610-619)send(656-665)send(708-717)send(764-773)send(797-806)send(837-846)send(869-878)send(901-910)CircleClockSetRequest(720-773)plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/messages/responses.py (2)
ack_id(134-136)NodeResponseType(56-73)plugwise_usb/nodes/node.py (1)
name(218-222)
🪛 GitHub Actions: Latest commit
plugwise_usb/nodes/circle.py
[error] 884-884: Ruff: Local variable 'circle_plus_timestamp' is assigned to but never used.
[error] 892-892: Ruff: Undefined name 'circle_timestamp'.
🔇 Additional comments (11)
plugwise_usb/nodes/circle.py (7)
5-5: LGTM on imports/constants and periodic sync intervalGood use of Final and light jitter on the period.
Also applies to: 12-12, 78-81
903-906: Fix typo in error textSmall grammar nit.
- raise NodeError( - "Unable to synchronize clock en when protocol version is unknown" - ) + raise NodeError("Unable to synchronize clock when protocol version is unknown")
1018-1021: LGTM: start background clock-sync after initial syncMatches lifecycle and avoids initial delay from the scheduler sleep.
1112-1116: LGTM: graceful scheduler cancellation on unloadCancel + gather with return_exceptions=True is correct.
877-895: Undefined variable and date overflow in drift calc; build nearest-day device datetime
circle_timestampis undefined anddt_now.replace(day=dt_now.day + days_diff, ...)can overflow months; also triggers false >23h drifts near midnight. Compute the device datetime from response.timestamp’s date using nearest of D-1/D/D+1.- dt_now = datetime.now(tz=UTC) - days_diff = (response.day_of_week.value - dt_now.weekday()) % 7 - circle_plus_timestamp: datetime = dt_now.replace( - day=dt_now.day + days_diff, - hour=response.time.value.hour, - minute=response.time.value.minute, - second=response.time.value.second, - microsecond=0, - tzinfo=UTC, - ) - clock_offset = response.timestamp.replace(microsecond=0) - circle_timestamp + base_ts = response.timestamp.replace(microsecond=0) + reported_time = response.time.value # datetime.time + base_date = base_ts.date() + candidates = [ + datetime.combine(base_date - timedelta(days=1), reported_time, tzinfo=UTC), + datetime.combine(base_date, reported_time, tzinfo=UTC), + datetime.combine(base_date + timedelta(days=1), reported_time, tzinfo=UTC), + ] + device_dt = min(candidates, key=lambda d: abs((base_ts - d).total_seconds())) + clock_offset = base_ts - device_dt if abs(clock_offset.total_seconds()) < MAX_TIME_DRIFT: return True
913-915: ACK comparison: cast to NodeResponseType for consistent equalityAvoid raw-value equality to prevent mismatches.
- if (node_response := await set_request.send()) is not None: - return node_response.ack_id == NodeResponseType.CLOCK_ACCEPTED + if (node_response := await set_request.send()) is not None: + return NodeResponseType(node_response.ack_id) == NodeResponseType.CLOCK_ACCEPTED
877-919: Fix undefined variable, remove debug logs, and re-run linter
- plugwise_usb/nodes/circle.py — line 892 still references undefined
circle_timestamp; replace withcircle_plus_timestampso clock_offset uses the computed value above.- plugwise_usb/nodes/circle_plus.py — remove HOI debug logs at lines 80, 81, 91, 92.
- Re-run ruff correctly (example):
ruff check plugwise_usb/nodes/circle.pyorruff check plugwise_usb.plugwise_usb/nodes/circle_plus.py (4)
5-5: Add timedelta import to support nearest-day computationNeeded by the drift fix.
-from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta
70-78: LGTM on availability update pathGood to update availability with response.timestamp.
79-97: Drift calc can be wrong near midnight; avoid day overflow; standardize logsReplace “today+time” computation with nearest-of (D-1/D/D+1) around response.timestamp. Also remove “HOI” debug strings and fix extra “)” in warning.
- dt_now = datetime.now(tz=UTC) - _LOGGER.debug("HOI dt_now weekday=%s", dt_now.weekday()) - _LOGGER.debug("HOI circle+ day_of_week=%s", response.day_of_week.value) - days_diff = (response.day_of_week.value - dt_now.weekday()) % 7 - circle_plus_timestamp: datetime = dt_now.replace( - day=(dt_now.day + days_diff), - hour=response.time.value.hour, - minute=response.time.value.minute, - second=response.time.value.second, - microsecond=0, - tzinfo=UTC, - ) - _LOGGER.debug("HOI circle+ clock=%s", circle_plus_timestamp) - _LOGGER.debug("HOI response timestamp=%s", response.timestamp) - clock_offset = response.timestamp.replace(microsecond=0) - circle_plus_timestamp + base_ts = response.timestamp.replace(microsecond=0) + reported_time = response.time.value # datetime.time + base_date = base_ts.date() + candidates = [ + datetime.combine(base_date - timedelta(days=1), reported_time, tzinfo=UTC), + datetime.combine(base_date, reported_time, tzinfo=UTC), + datetime.combine(base_date + timedelta(days=1), reported_time, tzinfo=UTC), + ] + device_dt = min(candidates, key=lambda d: abs((base_ts - d).total_seconds())) + _LOGGER.debug("Circle+ RTC candidate=%s, response timestamp=%s", device_dt, base_ts) + clock_offset = base_ts - device_dt if abs(clock_offset.total_seconds()) < MAX_TIME_DRIFT: return True - - _LOGGER.warning( - "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds)", - self._mac_in_str, - str(int(abs(clock_offset.total_seconds()))), - str(MAX_TIME_DRIFT), - ) + _LOGGER.warning( + "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds", + self._mac_in_str, + str(int(abs(clock_offset.total_seconds()))), + str(MAX_TIME_DRIFT), + )
106-107: ACK comparison: cast ack_id to NodeResponseTypeConsistent with enable_auto_join and avoids raw-value mismatches.
- if (node_response := await set_request.send()) is not None: - return node_response.ack_id == NodeResponseType.CLOCK_ACCEPTED + if (node_response := await set_request.send()) is not None: + return NodeResponseType(node_response.ack_id) == NodeResponseType.CLOCK_ACCEPTED
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tests/stick_test_data.py (3)
8-9: Fix Ruff F401 by making utc_now timezone-awareUse the imported UTC or drop it. Prefer making utc_now explicit UTC.
-# test using utc timezone - 2025-04-03 22:00:00 -utc_now = datetime(2025, 4, 3, 22, 0, 0) # datetime.now(tz=UTC).replace(tzinfo=UTC) +# test using utc timezone - 2025-04-03 22:00:00 +utc_now = datetime(2025, 4, 3, 22, 0, 0, tzinfo=UTC) # fixed UTC timestamp
602-605: Resolve UP031: replace percent-formatting with f-stringThis line triggers Ruff UP031. Use f-string + encode.
- + bytes( - ("%%0%dd" % 2) % (utc_now + timedelta(seconds=10)).second, pw_constants.UTF8 - ) # noqa: UP031 + + f"{(utc_now + timedelta(seconds=10)).second:02d}".encode(pw_constants.UTF8)
624-626: Resolve UP031 in hex seconds fieldUse f-string with hex formatting to avoid the linter error.
- + bytes( - ("%%0%dX" % 2) % (utc_now + timedelta(seconds=10)).second, pw_constants.UTF8 - ) # noqa: UP031 + + f"{(utc_now + timedelta(seconds=10)).second:02X}".encode(pw_constants.UTF8)plugwise_usb/nodes/circle.py (2)
903-906: Typo in error message“en” ➜ “when”.
- raise NodeError( - "Unable to synchronize clock en when protocol version is unknown" - ) + raise NodeError( + "Unable to synchronize clock when protocol version is unknown" + )
913-915: Be explicit when comparing ACK idDepending on response type, ack_id may be bytes. Cast to NodeResponseType for consistency (matches other code paths).
- if (node_response := await set_request.send()) is not None: - return node_response.ack_id == NodeResponseType.CLOCK_ACCEPTED + if (node_response := await set_request.send()) is not None: + return NodeResponseType(node_response.ack_id) == NodeResponseType.CLOCK_ACCEPTED
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
plugwise_usb/nodes/circle.py(6 hunks)tests/stick_test_data.py(2 hunks)tests/test_usb.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_usb.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 (5)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-112)plugwise_usb/messages/requests.py (19)
CircleClockGetRequest(998-1017)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(501-510)send(531-542)send(555-564)send(577-586)send(610-619)send(656-665)send(708-717)send(764-773)send(797-806)send(837-846)send(869-878)send(901-910)CircleClockSetRequest(720-773)plugwise_usb/exceptions.py (1)
NodeError(24-25)plugwise_usb/messages/responses.py (2)
ack_id(134-136)NodeResponseType(56-73)plugwise_usb/nodes/node.py (1)
name(218-222)
🪛 GitHub Actions: Latest commit
tests/stick_test_data.py
[error] 3-3: Ruff check failed: F401 'datetime.UTC' imported but unused. Command: ruff check plugwise_usb/ tests/.
[error] 603-603: Ruff check failed: UP031 Use format specifiers instead of percent format. Command: ruff check plugwise_usb/ tests/.
[error] 625-625: Ruff check failed: UP031 Use format specifiers instead of percent format. Command: ruff check plugwise_usb/ tests/.
🔇 Additional comments (5)
tests/stick_test_data.py (1)
612-616: LGTM: new Circle+ set-clock ACK messageThe added ACK entry looks correct and matches NodeResponseType.CLOCK_ACCEPTED.
plugwise_usb/nodes/circle.py (4)
1018-1021: LGTM: start background clock-sync after initial syncStarting the jittered scheduler only after a successful initial sync is sensible.
1112-1116: LGTM: proper cancellation and await of clock-sync task on unloadTask is cancelled, awaited with gather, and cleared.
Please also consider applying the same cancel+gather pattern to _retrieve_energy_logs_task above for symmetry.
860-874: Don’t swallow CancelledError in schedulerThe inner except catches CancelledError and prevents task shutdown on unload.
try: while True: await sleep(CLOCK_SYNC_PERIOD + (random.uniform(-5, 5))) - try: - await self.clock_synchronize() - except Exception: + try: + await self.clock_synchronize() + except CancelledError: + raise + except Exception: _LOGGER.exception( "Clock synchronization failed for %s", self._mac_in_str )
882-895: Clock drift calc can crash and be wrong near month boundariesUsing dt_now.replace(day=dt_now.day + days_diff, …) risks ValueError and wrong date around midnight. Derive the closest reported device datetime around the base timestamp.
- dt_now = datetime.now(tz=UTC) - days_diff = (response.day_of_week.value - dt_now.weekday()) % 7 - circle_timestamp: datetime = dt_now.replace( - day=dt_now.day + days_diff, - hour=response.time.value.hour, - minute=response.time.value.minute, - second=response.time.value.second, - microsecond=0, - tzinfo=UTC, - ) - clock_offset = response.timestamp.replace(microsecond=0) - circle_timestamp + base_ts = response.timestamp.replace(microsecond=0) + reported_time = response.time.value # datetime.time + base_date = base_ts.date() + candidates = [ + datetime.combine(base_date - timedelta(days=1), reported_time, tzinfo=UTC), + datetime.combine(base_date, reported_time, tzinfo=UTC), + datetime.combine(base_date + timedelta(days=1), reported_time, tzinfo=UTC), + ] + circle_timestamp = min( + candidates, key=lambda d: abs((base_ts - d).total_seconds()) + ) + clock_offset = base_ts - circle_timestamp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
tests/stick_test_data.py (3)
3-3: Make utc_now explicitly UTC (timezone‑aware) and drop dead code.Prevents future DST/naive/aware confusion in tests and removes commented leftovers.
-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone @@ -# test using utc timezone - 2025-04-03 22:00:00 -utc_now = datetime(2025, 4, 3, 22, 0, 0) # datetime.now(tz=UTC).replace(tzinfo=UTC) -# utc_now_offset = datetime.now(tz=UTC).replace(tzinfo=UTC) + timedelta( -# days=1, hours=4, seconds=30 -# ) +# test using UTC timezone - 2025-04-03 22:00:00 +utc_now = datetime(2025, 4, 3, 22, 0, 0, tzinfo=timezone.utc)Also applies to: 8-12
23-27: Avoid float math when deriving minute offset; use timedelta floor‑division.Eliminates float truncation and is clearer.
- delta_month = hour_timestamp - hour_timestamp.replace(day=1, hour=0) + delta_month = hour_timestamp - hour_timestamp.replace(day=1, hour=0, minute=0, second=0, microsecond=0) @@ - + bytes( - f"{int((delta_month.days * 1440) + (delta_month.seconds / 60)):04x}", - pw_constants.UTF8, - ) + + bytes( + f"{(delta_month // timedelta(minutes=1)):04x}", + pw_constants.UTF8, + )
601-608: Confirm expected encoding for 003A “Realtime clock” fields (decimal vs hex).Other clock payloads (003F) encode H/M/S/W as hex; here they’re decimal. If protocol expects ASCII‑hex, switch to :02x.
- + bytes(f"{(utc_now + timedelta(seconds=10)).second:02d}", pw_constants.UTF8) # noqa: UP031 - + bytes(f"{utc_now.minute:02d}", pw_constants.UTF8) # noqa: UP031 - + bytes(f"{utc_now.hour:02d}", pw_constants.UTF8) # noqa: UP031 - + bytes(f"{utc_now.weekday():02d}", pw_constants.UTF8) # noqa: UP031 - + bytes(f"{utc_now.day:02d}", pw_constants.UTF8) # noqa: UP031 - + bytes(f"{utc_now.month:02d}", pw_constants.UTF8) # noqa: UP031 - + bytes(f"{(utc_now.year - 2000):02d}", pw_constants.UTF8), # noqa: UP031 + + bytes(f"{(utc_now + timedelta(seconds=10)).second:02x}", pw_constants.UTF8) # noqa: UP031 + + bytes(f"{utc_now.minute:02x}", pw_constants.UTF8) # noqa: UP031 + + bytes(f"{utc_now.hour:02x}", pw_constants.UTF8) # noqa: UP031 + + bytes(f"{utc_now.weekday():02x}", pw_constants.UTF8) # noqa: UP031 + + bytes(f"{utc_now.day:02x}", pw_constants.UTF8) # noqa: UP031 + + bytes(f"{utc_now.month:02x}", pw_constants.UTF8) # noqa: UP031 + + bytes(f"{(utc_now.year - 2000):02x}", pw_constants.UTF8), # noqa: UP031Optional: extract tiny helpers to reduce future inconsistencies.
def hex2(n: int) -> bytes: return bytes(f"{n:02x}", pw_constants.UTF8) def dec2(n: int) -> bytes: return bytes(f"{n:02d}", pw_constants.UTF8)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/stick_test_data.py(6 hunks)
⏰ 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 (4)
tests/stick_test_data.py (4)
609-613: New Circle+ set‑clock ACK looks consistent with other ClockAccepted (00D7).Please confirm 0030/0028 frame mapping and 00D7 usage match device spec.
631-635: 003F clock for 222222… — encoding is consistent (all hex).
643-647: 003F clock for 333333… — encoding is consistent (all hex).
655-659: 003F clock for 444444… — encoding is consistent (all hex).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
tests/stick_test_data.py (4)
3-3: Make utc_now explicitly UTC-awarePrevents accidental local-time drift and matches the comment.
-from datetime import datetime, timedelta +from datetime import datetime, timedelta, timezone @@ -utc_now = datetime(2025, 4, 3, 22, 0, 0) # datetime.now(tz=UTC).replace(tzinfo=UTC) +utc_now = datetime(2025, 4, 3, 22, 0, 0, tzinfo=timezone.utc) # frozen UTCAlso applies to: 9-9
23-27: Eliminate float rounding in minute-of-month hex encodingUse integer math; drop redundant int().
- f"{int((delta_month.days * 1440) + (delta_month.seconds / 60)):04x}", + f"{(delta_month.days * 1440 + delta_month.seconds // 60):04x}",
615-615: Fix label typo: remove leading “00” in MACAligns description with the actual MAC.
- "clock for 0011111111111111", + "clock for 1111111111111111",
601-607: Clarify base: 003A uses decimal, 003F uses hexAdd a brief note to prevent confusion for future edits.
+ b"0098765432101234" # mac + # Note: 003A (Realtime clock) uses decimal ASCII for SS,MM,HH,WD,DD,MM,YY (2 digits each). + # 003F (clock) entries below use ASCII hex for those fields. + bytes(f"{(utc_now + timedelta(seconds=10)).second:02d}", pw_constants.UTF8) # noqa: UP031
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/stick_test_data.py(6 hunks)
⏰ 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 (2)
tests/stick_test_data.py (2)
619-623: Remove +10s offset in 003F clock for 111111…Keep 003F payloads consistent with the others (2222/3333/4444).
- + bytes(f"{(utc_now + timedelta(seconds=10)).second:02x}", pw_constants.UTF8) # noqa: UP031 + + bytes(f"{utc_now.second:02x}", pw_constants.UTF8) # noqa: UP031
609-613: LGTM: Circle+ realtime set‑clock entryAck ID and structure align with existing ClockAccepted (00D7) usage.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle_plus.py (3)
5-5: Add timedelta import (needed for nearest-date selection).Required for the candidate-date approach below.
-from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta
79-97: Anchor date drift check to response.timestamp to avoid midnight edge mismatches.Use the response timestamp’s date instead of wall-clock now, and compare date objects directly.
- dt_now = datetime.now(tz=UTC) - dt_now_date = dt_now.replace(hour=0, minute=0, second=0, microsecond=0) - response_date = datetime( - response.date.value.year, - response.date.value.month, - response.date.value.day, - hour=0, - minute=0, - second=0, - microsecond=0, - tzinfo=UTC, - ) - if dt_now_date != response_date: + base_ts = response.timestamp.replace(microsecond=0) + base_date = base_ts.date() + device_date = response.date.value + if base_date != device_date: _LOGGER.warning( - "Reset realtime clock of node %s because time has drifted %s days", + "Reset realtime clock of node %s because time has drifted %s days", self._mac_in_str, - int(abs((dt_now_date - response_date).days)), + abs((base_date - device_date).days), ) return await self._send_clock_set_req()
110-115: Fix stray parenthesis in log message.Minor formatting nit.
- _LOGGER.warning( - "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds)", + _LOGGER.warning( + "Reset realtime clock of node %s because time has drifted %s seconds while max drift is set to %s seconds",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
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.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle_plus.py (3)
plugwise_usb/messages/requests.py (19)
CirclePlusRealTimeClockGetRequest(970-989)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(501-510)send(531-542)send(555-564)send(577-586)send(610-619)send(656-665)send(708-717)send(764-773)send(797-806)send(837-846)send(869-878)send(901-910)CirclePlusRealTimeClockSetRequest(935-967)plugwise_usb/nodes/node.py (2)
mac(235-237)_available_update_state(423-451)plugwise_usb/messages/properties.py (13)
value(76-80)value(96-100)value(125-129)value(159-163)value(187-191)value(207-211)value(251-255)value(276-280)value(301-305)value(330-334)value(359-363)value(380-384)value(403-407)
⏰ 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/nodes/circle_plus.py (3)
70-78: LGTM: Robust request/availability handling.Good early return on no response and updating availability using response timestamp.
99-107: Avoid false large offsets near midnight; pick nearest date around response timestamp.Combine the reported time with base_date±1 and choose the closest to response.timestamp.
- circle_plus_timestamp: datetime = dt_now.replace( - hour=response.time.value.hour, - minute=response.time.value.minute, - second=response.time.value.second, - microsecond=0, - tzinfo=UTC, - ) - clock_offset = response.timestamp.replace(microsecond=0) - circle_plus_timestamp + reported_time = response.time.value # datetime.time + base_date = base_ts.date() + candidates = [ + datetime.combine(base_date - timedelta(days=1), reported_time).replace(tzinfo=UTC), + datetime.combine(base_date, reported_time).replace(tzinfo=UTC), + datetime.combine(base_date + timedelta(days=1), reported_time).replace(tzinfo=UTC), + ] + circle_plus_dt: datetime = min( + candidates, key=lambda d: abs((base_ts - d).total_seconds()) + ) + clock_offset = base_ts - circle_plus_dt
123-124: ACK check may fail without enum cast; cast ack_id to NodeResponseType.Matches pattern used in enable_auto_join; prevents false negatives when ack_id is an int.
- if (node_response := await set_request.send()) is not None: - return node_response.ack_id == NodeResponseType.CLOCK_ACCEPTED + if (node_response := await set_request.send()) is not None: + return NodeResponseType(node_response.ack_id) == NodeResponseType.CLOCK_ACCEPTED
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.
negative offsets.
bouwew
changed the title
bouwew
changed the title
bouwew
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
896-905: Tweak log formatting and fix typo in error message
- No need to str() the int drift.
- Typo “en” -> “when”.
- _LOGGER.warning( - "Sync clock of node %s because time drifted %s seconds", - self._mac_in_str, - str(int(abs(clock_offset.total_seconds()))), - ) + _LOGGER.warning( + "Sync clock of node %s because time drifted %s seconds", + self._mac_in_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" + )
1008-1018: Start scheduler only after successful initialize to avoid orphaned tasks on init failureIf calibration or relay-init fails afterwards, the scheduler remains running with _initialized=False.
- if self._clock_synchronize_task is None or self._clock_synchronize_task.done(): - self._clock_synchronize_task = create_task( - self._clock_synchronize_scheduler() - )Move the above block to after super().initialize(), just before returning True:
@@ - await super().initialize() - return True + await 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() + ) + return True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
plugwise_usb/nodes/circle.py(6 hunks)plugwise_usb/nodes/circle_plus.py(1 hunks)pyproject.toml(2 hunks)tests/stick_test_data.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 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.pyplugwise_usb/nodes/circle_plus.py
🧬 Code graph analysis (2)
plugwise_usb/nodes/circle.py (3)
plugwise_usb/nodes/circle_plus.py (1)
clock_synchronize(68-115)plugwise_usb/messages/requests.py (19)
CircleClockGetRequest(998-1017)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(501-510)send(531-542)send(555-564)send(577-586)send(610-619)send(656-665)send(708-717)send(764-773)send(797-806)send(837-846)send(869-878)send(901-910)CircleClockSetRequest(720-773)plugwise_usb/messages/responses.py (1)
NodeResponseType(56-73)
plugwise_usb/nodes/circle_plus.py (4)
plugwise_usb/messages/requests.py (19)
CirclePlusRealTimeClockGetRequest(970-989)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(501-510)send(531-542)send(555-564)send(577-586)send(610-619)send(656-665)send(708-717)send(764-773)send(797-806)send(837-846)send(869-878)send(901-910)CirclePlusRealTimeClockSetRequest(935-967)plugwise_usb/nodes/node.py (2)
_available_update_state(423-451)name(218-222)plugwise_usb/messages/properties.py (13)
value(76-80)value(96-100)value(125-129)value(159-163)value(187-191)value(207-211)value(251-255)value(276-280)value(301-305)value(330-334)value(359-363)value(380-384)value(403-407)plugwise_usb/messages/responses.py (1)
NodeResponseType(56-73)
🔇 Additional comments (10)
plugwise_usb/nodes/circle_plus.py (3)
70-77: Good: availability handling on GET responseKeeping availability in sync with the GET response timestamp is correct.
99-107: Fix midnight/day-boundary drift: build nearest-date candidate from response.timestampUsing dt_now.replace(...) with the reported time miscomputes around midnight and can trigger false resets. Derive the device datetime by combining the reported time with the nearest of (D-1, D, D+1) around response.timestamp.
Apply:
- circle_plus_timestamp: datetime = dt_now.replace( - hour=response.time.value.hour, - minute=response.time.value.minute, - second=response.time.value.second, - microsecond=0, - tzinfo=UTC, - ) - clock_offset = response.timestamp.replace(microsecond=0) - circle_plus_timestamp + base_ts = response.timestamp.replace(microsecond=0) + reported_time = response.time.value # datetime.time + base_date = base_ts.date() + candidates = [ + datetime.combine(base_date - timedelta(days=1), reported_time).replace(tzinfo=UTC), + datetime.combine(base_date, reported_time).replace(tzinfo=UTC), + datetime.combine(base_date + timedelta(days=1), reported_time).replace(tzinfo=UTC), + ] + circle_plus_timestamp: datetime = min( + candidates, key=lambda d: abs((base_ts - d).total_seconds()) + ) + clock_offset = base_ts - circle_plus_timestampAlso ensure timedelta is imported:
-from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta
117-125: ACK compare should cast to NodeResponseTypeCast ack_id before equality to avoid mismatches when ack_id is raw bytes/int.
- if (node_response := await set_request.send()) is not None: - return node_response.ack_id == NodeResponseType.CLOCK_ACCEPTED + if (node_response := await set_request.send()) is not None: + return NodeResponseType(node_response.ack_id) == NodeResponseType.CLOCK_ACCEPTEDplugwise_usb/nodes/circle.py (5)
147-149: State var for scheduler looks goodTracking the scheduler Task avoids duplicate tasks and enables clean shutdown.
1108-1112: Good: clean shutdown of schedulerCancel + gather with return_exceptions avoids “Task exception was never retrieved”.
860-874: Don’t swallow CancelledError in scheduler loopInner except Exception will trap CancelledError and prevent task shutdown.
try: while True: await sleep(CLOCK_SYNC_PERIOD + (random.uniform(-5, 5))) - try: - await self.clock_synchronize() - except Exception: + try: + await self.clock_synchronize() + except CancelledError: + raise + except Exception: _LOGGER.exception( "Clock synchronization failed for %s", self._mac_in_str )
882-893: Fix unsafe date math and midnight drift; use nearest-date candidateReplacing day with dt_now.day + days_diff may overflow month and mis-handle near midnight. Compute the device datetime against response.timestamp using nearest (D-1, D, D+1).
- dt_now = datetime.now(tz=UTC) - days_diff = (response.day_of_week.value - dt_now.weekday()) % 7 - circle_timestamp: datetime = dt_now.replace( - day=dt_now.day + days_diff, - hour=response.time.value.hour, - minute=response.time.value.minute, - second=response.time.value.second, - microsecond=0, - tzinfo=UTC, - ) - clock_offset = response.timestamp.replace(microsecond=0) - circle_timestamp + base_ts = response.timestamp.replace(microsecond=0) + reported_time = response.time.value # datetime.time + base_date = base_ts.date() + candidates = [ + datetime.combine(base_date - timedelta(days=1), reported_time).replace(tzinfo=UTC), + datetime.combine(base_date, reported_time).replace(tzinfo=UTC), + datetime.combine(base_date + timedelta(days=1), reported_time).replace(tzinfo=UTC), + ] + device_dt = min(candidates, key=lambda d: abs((base_ts - d).total_seconds())) + clock_offset = base_ts - device_dt
906-915: ACK compare should cast to NodeResponseTypeCast ack_id for consistent, reliable equality.
- if (node_response := await set_request.send()) is not None: - return node_response.ack_id == NodeResponseType.CLOCK_ACCEPTED + if (node_response := await set_request.send()) is not None: + return NodeResponseType(node_response.ack_id) == NodeResponseType.CLOCK_ACCEPTEDtests/stick_test_data.py (2)
617-621: 003F payload: remove the +10s offset for consistencyThis one payload still adds +10s while others don’t. Make seconds use fixed_time.second.
- + bytes(f"{(fixed_time + timedelta(seconds=10)).second:02x}", pw_constants.UTF8) + + bytes(f"{fixed_time.second:02x}", pw_constants.UTF8)
599-606: Ignore hex conversion suggestion for 003A: uses decimal ASCII per RealClockTime/RealClockDate
RealClockTime and RealClockDate use IntDec (decimal ASCII) and the +10s offset matches device behavior; the existing test fixture aligns with the parser.Likely an incorrect or invalid review comment.
dirixmjm
left a comment
dirixmjm
left a comment
There was a problem hiding this comment.
Good solution to the testing!
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle_plus.py (1)
119-125: Explicitly log and handle missing or unexpected ACKs
• Ifawait set_request.send()returnsNone, log “Failed to sync the clock of %s (no response)” and return False
• Ifnode_response.ack_id != NodeResponseType.CLOCK_ACCEPTED, log “Clock sync for %s rejected: ACK=%s” and return False
• Otherwise return True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/nodes/circle.py(9 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.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_plus.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle_plus.py (4)
plugwise_usb/messages/requests.py (19)
CirclePlusRealTimeClockGetRequest(970-989)response(124-128)send(357-366)send(379-388)send(430-432)send(468-477)send(501-510)send(531-542)send(555-564)send(577-586)send(610-619)send(656-665)send(708-717)send(764-773)send(797-806)send(837-846)send(869-878)send(901-910)CirclePlusRealTimeClockSetRequest(935-967)plugwise_usb/nodes/node.py (2)
_available_update_state(423-451)name(218-222)plugwise_usb/messages/properties.py (13)
value(76-80)value(96-100)value(125-129)value(159-163)value(187-191)value(207-211)value(251-255)value(276-280)value(301-305)value(330-334)value(359-363)value(380-384)value(403-407)plugwise_usb/messages/responses.py (2)
ack_id(134-136)NodeResponseType(56-73)
🔇 Additional comments (3)
plugwise_usb/nodes/circle_plus.py (3)
70-77: Good early-fail handling and availability updatesRequest/response handling and availability updates on success/failure look correct.
110-115: LGTM on drift loggingClear, actionable info-level log on drift magnitude.
79-107: Fix midnight/date-boundary drift bug by picking nearest date around response timestampBuilding “today’s date + reported time” can yield ~±24h errors around midnight. Instead, derive the device datetime by combining the reported time with the nearest of (D-1, D, D+1) around the response timestamp and compute drift against that. This also removes the need for the separate day-drift branch.
Apply this diff:
- dt_now = datetime.now(tz=UTC) - dt_now_date = dt_now.replace(hour=0, minute=0, second=0, microsecond=0) - response_date = datetime( - response.date.value.year, - response.date.value.month, - response.date.value.day, - hour=0, - minute=0, - second=0, - microsecond=0, - tzinfo=UTC, - ) - if dt_now_date != response_date: - _LOGGER.info( - "Sync clock of node %s because time has drifted %s days", - self._mac_in_str, - int(abs((dt_now_date - response_date).days)), - ) - return await self._send_clock_set_req() - - circle_plus_timestamp: datetime = dt_now.replace( - hour=response.time.value.hour, - minute=response.time.value.minute, - second=response.time.value.second, - microsecond=0, - tzinfo=UTC, - ) - clock_offset = response.timestamp.replace(microsecond=0) - circle_plus_timestamp + base_ts = response.timestamp.replace(microsecond=0) + reported_time = response.time.value + base_date = base_ts.date() + candidates = [ + datetime.combine(base_date - timedelta(days=1), reported_time).replace(tzinfo=UTC), + datetime.combine(base_date, reported_time).replace(tzinfo=UTC), + datetime.combine(base_date + timedelta(days=1), reported_time).replace(tzinfo=UTC), + ] + circle_plus_dt: datetime = min( + candidates, key=lambda d: abs((base_ts - d).total_seconds()) + ) + clock_offset = base_ts - circle_plus_dt if abs(clock_offset.total_seconds()) < MAX_TIME_DRIFT: return TrueAdditional change outside this hunk (imports at top of file):
from datetime import UTC, datetime, timedelta
Summary by CodeRabbit