Improve EnergyLogs collection #311
Conversation
WalkthroughReworks PlugwiseCircle energy-log collection: adds MAX_LOG_HOURS, changes energy_log_update to return a single bool indicating if any recent record was stored, adds per-record recency checks, simplifies initial collection/rollover flow, tightens cache writes and task cancellation. Changes
Sequence Diagram(s)sequenceDiagram
actor Scheduler
participant Circle as PlugwiseCircle
participant Node
participant Cache
Scheduler->>Circle: energy_update()
Circle->>Node: get_current_log_address()
alt rollover suspected
Circle->>Node: get_previous_log_address()
end
loop addresses to collect
Circle->>Node: request EnergyLogs(address)
Node-->>Circle: EnergyLogs(records)
Circle->>Circle: _check_timestamp_is_recent(address, slot, ts)
alt recent & non-empty
Circle->>Cache: store record(s) (if save_cache=True)
Note right of Circle: any_record_stored = True
else
Circle->>Circle: store empty record / skip stale
end
end
Circle-->>Scheduler: return True if any_record_stored else False
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #311 +/- ##
==========================================
+ Coverage 80.54% 80.61% +0.06%
==========================================
Files 36 36
Lines 7540 7536 -4
==========================================
+ Hits 6073 6075 +2
+ Misses 1467 1461 -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: 1
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle.py (3)
77-77: Consider documenting the constant purposeWhile
MAX_LOG_HOURSis clearly derived fromDAY_IN_HOURS, it would be helpful to add a comment explaining that this represents the maximum age threshold for energy log records (24 hours).+# Maximum age (in hours) for energy log records to be considered recent MAX_LOG_HOURS = DAY_IN_HOURS
467-467: Improve timestamp calculation clarityThe calculation
int(floor(datetime.now(tz=UTC).hour / 4) + 1)could be more self-documenting by using parentheses to make the order of operations explicit.- total_addresses = int(floor(datetime.now(tz=UTC).hour / 4) + 1) + total_addresses = int(floor(datetime.now(tz=UTC).hour / 4)) + 1
590-590: Consider extracting the seconds conversion constantThe magic number
3600could be replaced with a constant or use a more explicit calculation for better readability.+from ..constants import HOUR_IN_SECONDS # Add to imports if not exists - if age_seconds > MAX_LOG_HOURS * 3600: + if age_seconds > MAX_LOG_HOURS * HOUR_IN_SECONDS:If
HOUR_IN_SECONDSdoesn't exist in constants, you could define it there or use:- if age_seconds > MAX_LOG_HOURS * 3600: + if age_seconds > MAX_LOG_HOURS * 60 * 60: # hours to seconds
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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 (4)
plugwise_usb/messages/responses.py (1)
log_address(770-772)plugwise_usb/messages/requests.py (18)
CircleEnergyLogsRequest(1131-1165)response(124-128)send(366-375)send(388-397)send(439-441)send(477-486)send(513-522)send(543-554)send(567-576)send(589-598)send(622-631)send(668-677)send(720-729)send(776-785)send(809-818)send(849-858)send(881-890)send(913-922)plugwise_usb/nodes/helpers/counter.py (4)
add_empty_log(70-72)update(147-176)update(286-328)timestamp(179-187)plugwise_usb/nodes/helpers/pulses.py (1)
add_empty_log(389-421)
🔇 Additional comments (4)
plugwise_usb/nodes/circle.py (4)
471-477: LGTM! Improved energy log collection logicThe refactored logic correctly stops initial collection when all slots are empty or outdated, preventing unnecessary storage of stale data.
513-579: Well-designed energy log update refactoringThe refactored
energy_log_updatemethod effectively:
- Returns a single boolean to indicate success (at least one recent record stored)
- Implements time-based filtering via
_check_timestamp_is_recent- Properly handles empty slots when timestamps are outdated
- Updates cache only when records are actually stored
This change successfully prevents storing stale energy log data as intended.
544-551: Good defensive programming with recency checkThe addition of the recency check before storing energy log records is a crucial improvement that aligns with the PR objective to filter out data outside the configured time interval.
581-600: LGTM! Clear and effective timestamp recency validationThe
_check_timestamp_is_recenthelper method:
- Correctly calculates age in seconds with UTC timezone handling
- Provides clear warning messages with all relevant context
- Returns appropriate boolean result for filtering decisions
The implementation effectively supports the PR's goal of preventing storage of outdated energy log data.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
503-513: Fix cancellation flow to avoid awaiting canceled tasks (break early and cancel only pending tasks).Awaiting tasks after canceling them will raise CancelledError. Also, canceling already-finished tasks is unnecessary. Break out once the first failure is observed and cancel only the remaining, not-yet-awaited tasks.
Apply this diff:
- tasks = [ - create_task(self.energy_log_update(address)) - for address in missing_addresses - ] - for task in tasks: - await task - # When an energy log collection task returns False, do not execute the remaining tasks - if not task.result(): - for t in tasks: - t.cancel() + tasks = [ + create_task(self.energy_log_update(address)) + for address in missing_addresses + ] + for idx, task in enumerate(tasks): + result = await task + # When an energy log collection task returns False, stop and cancel the remaining tasks + if not result: + for t in tasks[idx + 1 :]: + t.cancel() + break
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle.py (3)
457-465: Docstring is outdated relative to new collection strategy.The docstring still states “from the last 10 log addresses” while the implementation collects a dynamic number of addresses. Update the docstring to avoid confusion.
Suggested replacement for the method docstring (apply outside this range):
async def _get_initial_energy_logs(self) -> None: """Collect initial energy logs for recent hours up to MAX_LOG_HOURS (or hours elapsed today)."""
548-553: Fix comment typo and phrasing.Small nit to improve readability.
Apply this diff:
- # Don't store an old log-record; store am empty record instead + # Don't store an old log record; store an empty record instead
585-605: Consider lowering log level for outdated records to avoid log noise.Outdated logs can be normal when backfilling or after rollovers. Emitting a warning per slot may spam logs. Suggest info or debug.
Apply this diff:
- _LOGGER.warning( + _LOGGER.info( "EnergyLog from Node %s | address %s | slot %s | timestamp %s is outdated, ignoring...", self._mac_in_str, address, slot, timestamp, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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 (8)
plugwise_usb/nodes/helpers/pulses.py (2)
calc_log_address(20-36)add_empty_log(389-421)plugwise_usb/messages/responses.py (1)
log_address(770-772)plugwise_usb/messages/requests.py (18)
CircleEnergyLogsRequest(1131-1165)response(124-128)send(366-375)send(388-397)send(439-441)send(477-486)send(513-522)send(543-554)send(567-576)send(589-598)send(622-631)send(668-677)send(720-729)send(776-785)send(809-818)send(849-858)send(881-890)send(913-922)plugwise_usb/nodes/helpers/counter.py (1)
add_empty_log(70-72)plugwise_usb/api.py (1)
save_cache(368-371)plugwise_usb/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/node.py (1)
save_cache(705-719)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)
⏰ 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 (5)
plugwise_usb/nodes/circle.py (5)
371-374: Double-check intent: gate on “requested” vs. “successful” for the initial last-log fetch.Setting _last_energy_log_requested to the boolean result means a failed attempt will keep re-requesting on every energy_update call. If the goal is to “request once at startup” regardless of success, set the flag before awaiting.
Optional change if you want to avoid repeated requests:
- if not self._last_energy_log_requested: - self._last_energy_log_requested = await self.energy_log_update( - self._current_log_address - ) + if not self._last_energy_log_requested: + self._last_energy_log_requested = True + await self.energy_log_update(self._current_log_address)
384-393: LGTM: boolean handling across rollover and single-missing flows is consistent.The refactor cleanly propagates success/failure from energy_log_update and logs address-specific failures. This aligns the control flow with the new return contract.
Also applies to: 397-405, 418-427
517-535: LGTM: simplified return contract and early exit behavior.Clear True/False semantics, guarded request, and consistent availability updates improve readability and robustness.
523-527: LGTM: request logging and cache-save gating.
- Clear, address-specific request logging.
- Save-to-cache only when new records were stored prevents unnecessary writes.
- Returning the aggregated boolean matches the stated contract.
Also applies to: 536-536, 577-584
28-28: LGTM: explicit MAX_LOG_HOURS constant and import.Defining MAX_LOG_HOURS and basing it on DAY_IN_HOURS makes the recency policy explicit and easily adjustable.
Also applies to: 77-78
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
plugwise_usb/nodes/circle.py (5)
76-76: Declare MAX_LOG_HOURS as a Final constantThis constant appears to be intended as a module-level constant. Make that explicit to aid type-checkers and readability.
-MAX_LOG_HOURS = DAY_IN_HOURS +MAX_LOG_HOURS: Final = DAY_IN_HOURS
516-573: Energy log update: remove dead state and tighten comments
- The local variable last_energy_timestamp_collected is declared but never used.
- Comment typo: “store am empty record” -> “store an empty record”.
- last_energy_timestamp_collected = 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 ) if ( log_timestamp is None or log_pulses is None - # Don't store an old log record; store am empty record instead + # Don't store an old log record; store an empty record instead or not self._check_timestamp_is_recent(address, _slot, log_timestamp) ): self._energy_counters.add_empty_log(response.log_address, _slot) continue
574-594: Clarify recency docstring; consider reducing log level for outdated records
- Docstring wording is ambiguous: “newer than MAX_LOG_HOURS” -> “within MAX_LOG_HOURS hours”.
- Optional: Logging every outdated slot as warning can be noisy on first runs. Consider info or debug unless this is truly exceptional.
- """Check if the timestamp of the received log-record is recent. - - A timestamp newer than MAX_LOG_HOURS is considered recent. - """ + """Return True if the record timestamp is within MAX_LOG_HOURS hours from now.""" @@ - if age_seconds > MAX_LOG_HOURS * 3600: - _LOGGER.warning( + if age_seconds > MAX_LOG_HOURS * 3600: + _LOGGER.info( "EnergyLog from Node %s | address %s | slot %s | timestamp %s is outdated, ignoring...", self._mac_in_str, address, slot, timestamp, ) return False return True
369-371: Variable naming mismatch: _last_energy_log_requested now stores success stateYou now set _last_energy_log_requested to the boolean result of energy_log_update. That makes it a “last update succeeded” flag rather than “requested” status. If intentional, consider renaming to avoid confusion.
Suggested rename (follow-up, outside this diff):
- _last_energy_log_requested -> _last_energy_log_update_succeeded
523-536: Log messages: consistent phrasingMinor nit: “EnergyLogs from node …” and “Requesting EnergyLogs …” mix singular/plural and capitalization. For consistency and easier grep, consider standardizing to “Energy logs” (lowercase) across messages.
Example:
- "Requesting energy logs from node %s, address=%s"
- "Energy logs from node %s, address=%s"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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 (7)
plugwise_usb/nodes/helpers/pulses.py (2)
calc_log_address(20-36)add_empty_log(389-421)plugwise_usb/messages/responses.py (1)
log_address(770-772)plugwise_usb/messages/requests.py (18)
CircleEnergyLogsRequest(1131-1165)response(124-128)send(366-375)send(388-397)send(439-441)send(477-486)send(513-522)send(543-554)send(567-576)send(589-598)send(622-631)send(668-677)send(720-729)send(776-785)send(809-818)send(849-858)send(881-890)send(913-922)plugwise_usb/nodes/helpers/counter.py (4)
add_empty_log(70-72)update(147-176)update(286-328)timestamp(179-187)plugwise_usb/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/node.py (1)
save_cache(705-719)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)
🔇 Additional comments (2)
plugwise_usb/nodes/circle.py (2)
383-404: Rollover handling LGTMThe two-step rollover fetch (current then previous address) aligns with address-slot semantics and the recency filter. Returning None on failure keeps the flow consistent.
416-425: Single missing address flow LGTMUsing the boolean return to decide whether to proceed and update power is consistent with the new contract.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
546-546: Remove unused variable to fix Ruff F841
last_energy_timestamp_collectedis assigned but never used.Apply this diff:
- last_energy_timestamp_collected = False
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
552-559: Nit: comment typo and clarity“store am empty record” → “store an empty record”. Keeps logs and comments professional.
Apply this diff:
- # Don't store an old log record; store am empty record instead + # Don't store an old log record; store an empty record instead
579-586: Nit: docstring wording for recency“A timestamp newer than MAX_LOG_HOURS is considered recent” is ambiguous. Suggest explicitly stating “not older than MAX_LOG_HOURS”.
Apply this diff:
- """Check if the timestamp of the received log-record is recent. - - A timestamp newer than MAX_LOG_HOURS is considered recent. - """ + """Check whether the log-record timestamp is recent. + + A timestamp not older than MAX_LOG_HOURS (in hours) is considered recent. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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 (6)
plugwise_usb/nodes/helpers/pulses.py (2)
calc_log_address(20-36)add_empty_log(389-421)plugwise_usb/messages/responses.py (1)
log_address(770-772)plugwise_usb/messages/requests.py (18)
CircleEnergyLogsRequest(1131-1165)response(124-128)send(366-375)send(388-397)send(439-441)send(477-486)send(513-522)send(543-554)send(567-576)send(589-598)send(622-631)send(668-677)send(720-729)send(776-785)send(809-818)send(849-858)send(881-890)send(913-922)plugwise_usb/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/node.py (1)
save_cache(705-719)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)
🪛 GitHub Actions: Latest commit
plugwise_usb/nodes/circle.py
[error] 513-513: Ruff: PLC0415 'import' should be at the top-level of a file. (Command: 'ruff check plugwise_usb/ tests/').
[error] 546-546: Ruff: F841 local variable 'last_energy_timestamp_collected' assigned to but never used. (Command: 'ruff check plugwise_usb/ tests/').
🔇 Additional comments (7)
plugwise_usb/nodes/circle.py (7)
27-27: LGTM: Importing DAY_IN_HOURS for bounded recency windowImport is used to derive MAX_LOG_HOURS and matches the PR goal to time-bound energy logs.
76-76: LGTM: MAX_LOG_HOURS aligns recency filtering with a full dayDefining MAX_LOG_HOURS = DAY_IN_HOURS is clear and centralizes the policy.
369-371: Re-attempt behavior: confirm repeated last-log requests are intendedSetting
_last_energy_log_requestedto the boolean result ofenergy_log_updatewill re-request the last logs on everyenergy_update()call until a “recent” record is stored. This could be noisy if data remains outdated for a while.Would you prefer tracking “attempted once” separately to avoid repeated requests, or is the current retry-until-recent behavior desired? I can propose a small refactor if you want to avoid retries when no recent data is available.
383-391: LGTM: Rollover handling for current addressThe retry on
_current_log_addresswith proper logging and early return on failure looks consistent with the new boolean contract.
395-404: LGTM: Previous-address retry after rolloverUsing
calc_log_address(self._current_log_address, 1, -4)to step one address back is correct for hourly-address stepping (4 slots).
417-425: LGTM: Single missing address pathDirectly awaiting
energy_log_updateand proceeding topower_updateon success matches the new API and short-circuits appropriately.
571-576: LGTM: Save cache only when at least one recent record was storedThis reduces unnecessary cache writes and aligns with the recency filter intent.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
531-552: Guard against None timestamp before updating availabilityCircleEnergyLogsResponse may not always carry a timestamp. Mirror the defensive pattern used in power_update.
Apply this diff:
- await self._available_update_state(True, response.timestamp) + if response.timestamp is not None: + await self._available_update_state(True, response.timestamp) + else: + await self._available_update_state(True)
🧹 Nitpick comments (5)
plugwise_usb/nodes/circle.py (5)
77-79: Minor consistency: annotate MAX_LOG_HOURS as FinalFor consistency with MAX_ADDRESSES_COLLECTED, annotate MAX_LOG_HOURS with Final.
Apply this diff:
-MAX_LOG_HOURS = DAY_IN_HOURS +MAX_LOG_HOURS: Final = DAY_IN_HOURS
371-374: Name mismatch: _last_energy_log_requested tracks success, not just “requested”After the change, this flag indicates “successful recent log fetch” (True/False), not merely that a request was sent. Consider renaming to avoid confusion in future maintenance (e.g., _initial_energy_log_fetched).
517-526: Log canceled addresses when halting batch; keep gather drainYou correctly cancel and drain remaining tasks. Consider logging which addresses are being canceled for better traceability during debugging.
Apply this diff:
for idx, task in enumerate(tasks): result = await task # When an energy log collection task returns False, stop and cancel the remaining tasks if not result: to_cancel = tasks[idx + 1 :] + _LOGGER.debug( + "async_energy_update | %s | Batch update halting; canceling remaining addresses: %s", + self._mac_in_str, + missing_addresses[idx + 1 :], + ) for t in to_cancel: t.cancel() # Drain cancellations to avoid "Task exception was never retrieved" await gather(*to_cancel, return_exceptions=True) break
561-569: Nit: fix typo and clarify comment
- “store am empty record” → “store an empty record”.
Apply this diff:
- # Don't store an old log record; store am empty record instead + # Don't store an old log record; store an empty record instead
588-607: Clarify docstring wording and keep semantics unchanged“A timestamp newer than MAX_LOG_HOURS” is ambiguous. Suggest “not older than MAX_LOG_HOURS hours” for clarity.
Apply this diff:
def _check_timestamp_is_recent( self, address: int, slot: int, timestamp: datetime ) -> bool: - """Check if the timestamp of the received log-record is recent. - - A timestamp newer than MAX_LOG_HOURS is considered recent. - """ + """Check if the log-record timestamp is recent. + + A timestamp not older than MAX_LOG_HOURS hours is considered recent. + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(8 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
🔇 Additional comments (6)
plugwise_usb/nodes/circle.py (6)
5-5: LGTM: Top-level imports and constants usage
- Importing gather at top-level resolves PLC0415 and is correct for the cancellation-drain code.
- ceil import is used appropriately for address rounding.
- DAY_IN_HOURS import for deriving MAX_LOG_HOURS aligns with the PR objective.
Also applies to: 11-11, 28-28
385-393: Rollover: consistent boolean handling on failureThis path correctly treats a False return from energy_log_update as a failure and aborts the update. Matches the new boolean contract.
397-405: Rollover retry: previous address handling is correctRetrying prev_log_address on rollover is correct and uses calc_log_address appropriately.
419-427: Single missing address path is correctCorrectly updates the single missing address and proceeds to power_update only on success.
479-487: Initial collection early-stop is sensibleStopping when an address yields only empty/outdated slots avoids unnecessary requests and matches the PR objective.
580-587: Cache save conditioned on actual stored records is goodOnly saving to cache when a recent non-empty record was stored reduces unnecessary I/O.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
551-552: Guard against None timestamp when updating availabilityCircleEnergyLogsResponse may not always carry a timestamp. Other paths (e.g., power_update) guard for None. Apply a defensive check here too.
- await self._available_update_state(True, response.timestamp) + if response.timestamp is not None: + await self._available_update_state(True, response.timestamp) + else: + await self._available_update_state(True)
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
561-569: Nit: fix comment typo and tighten wordingSmall clarity tweak; no behavior change.
- if ( - log_timestamp is None - or log_pulses is None - # Don't store an old log record; store am empty record instead - or not self._check_timestamp_is_recent(address, _slot, log_timestamp) - ): + if ( + log_timestamp is None + or log_pulses is None + # Don't store an old log record; store an empty record instead + or not self._check_timestamp_is_recent(address, _slot, log_timestamp) + ):
588-607: Clarify recency docstring; confirm boundary semanticsThe docstring “newer than MAX_LOG_HOURS is considered recent” is ambiguous. Prefer explicitly stating “within the last MAX_LOG_HOURS hours.” Also, if you intend to exclude records exactly at the boundary (age == MAX_LOG_HOURS hours), change ‘>’ to ‘>=’.
Docstring improvement:
- """Check if the timestamp of the received log-record is recent. - - A timestamp newer than MAX_LOG_HOURS is considered recent. - """ + """Check if a log record timestamp is within the last MAX_LOG_HOURS hours."""If boundary should be exclusive (i.e., 24h-old should be considered outdated), change the comparison:
- if age_seconds > MAX_LOG_HOURS * 3600: + if age_seconds >= MAX_LOG_HOURS * 3600:Please confirm whether the inclusive or exclusive boundary is desired for MAX_LOG_HOURS.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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 (6)
plugwise_usb/nodes/helpers/pulses.py (2)
calc_log_address(20-36)add_empty_log(389-421)plugwise_usb/messages/responses.py (2)
log_address(770-772)log_data(775-794)plugwise_usb/messages/requests.py (18)
CircleEnergyLogsRequest(1131-1165)response(124-128)send(366-375)send(388-397)send(439-441)send(477-486)send(513-522)send(543-554)send(567-576)send(589-598)send(622-631)send(668-677)send(720-729)send(776-785)send(809-818)send(849-858)send(881-890)send(913-922)plugwise_usb/api.py (1)
save_cache(368-371)plugwise_usb/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)
🔇 Additional comments (3)
plugwise_usb/nodes/circle.py (3)
5-5: Top-level asyncio.gather import fix looks goodImporting gather at module scope resolves PLC0415 and keeps cancellation draining clean.
371-373: Storing the boolean result for the “last log requested” flag is correctThis aligns the initial fetch logic with the new boolean contract of energy_log_update.
517-526: Good: cancel-and-drain pattern avoids “Task exception was never retrieved”Creating all tasks at once, short-circuiting on first False, and draining cancellation via gather(return_exceptions=True) is robust.
bouwew
force-pushed
the
improve-energy-collection
branch
from
18ea6e9 to
8378501
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
plugwise_usb/nodes/circle.py (2)
549-551: Guard against None timestamp before updating availabilityThis mirrors the defensive pattern used in power_update and avoids passing a None timestamp.
Apply:
- await self._available_update_state(True, response.timestamp) + if response.timestamp is not None: + await self._available_update_state(True, response.timestamp) + else: + await self._available_update_state(True)
456-475: Fix “consumption-only” vs “production+consumption” factor when production is disabled via NO_PRODUCTION_INTERVALCurrently, production disabled via NO_PRODUCTION_INTERVAL is treated as “production enabled,” yielding factor=2 instead of 4. This halves the intended hours-per-address and over-collects addresses.
Use an explicit check that treats both None and NO_PRODUCTION_INTERVAL as “consumption-only.”
Apply:
- # When only consumption is measured, 1 address contains data from 4 hours - # When both consumption and production are measured, 1 address contains data from 2 hours - factor = 4 if self.energy_production_interval is None else 2 + # When only consumption is measured, 1 address contains data from 4 hours + # When both consumption and production are measured, 1 address contains data from 2 hours + cons_only = self.energy_production_interval in (None, NO_PRODUCTION_INTERVAL) + factor = 4 if cons_only else 2 max_addresses_to_collect = MAX_LOG_HOURS // factor total_addresses = min( max_addresses_to_collect, ceil(datetime.now(tz=UTC).hour / factor) + 1 )
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
418-426: Single missing address fast-path is OK; consider logging on FalseOptional: add a small debug log when result is False to aid troubleshooting before falling back to the background task.
Apply:
if len(missing_addresses) == 1: result = await self.energy_log_update(missing_addresses[0]) if result: await self.power_update() _LOGGER.debug( "async_energy_update for %s | single energy log is missing | %s", self._mac_in_str, missing_addresses, ) return self._energy_counters.energy_statistics + else: + _LOGGER.debug( + "async_energy_update for %s | single-address update returned False, deferring to task", + self._mac_in_str, + )
587-604: Optional: avoid repeated datetime.now calls during per-slot checksMinor micro-optimization: compute “now” once per response to reduce time calls inside the hot loop.
Apply within energy_log_update before the for-loop:
- for _slot in range(4, 0, -1): + now_utc = datetime.now(tz=UTC) + for _slot in range(4, 0, -1):and change _check_timestamp_is_recent to accept an optional now_utc parameter:
- def _check_timestamp_is_recent( - self, address: int, slot: int, timestamp: datetime - ) -> bool: + def _check_timestamp_is_recent( + self, address: int, slot: int, timestamp: datetime, now_utc: datetime | None = None + ) -> bool: """Check if a log record timestamp is within the last MAX_LOG_HOURS hours.""" - age_seconds = ( - datetime.now(tz=UTC) - timestamp.replace(tzinfo=UTC) - ).total_seconds() + reference = now_utc or datetime.now(tz=UTC) + age_seconds = (reference - timestamp.replace(tzinfo=UTC)).total_seconds()and call it as:
- or not self._check_timestamp_is_recent(address, _slot, log_timestamp) + or not self._check_timestamp_is_recent(address, _slot, log_timestamp, now_utc)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(8 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 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/helpers/pulses.py (2)
calc_log_address(20-36)add_empty_log(389-421)plugwise_usb/nodes/node.py (2)
energy_consumption_interval(179-185)energy_production_interval(189-195)plugwise_usb/messages/responses.py (2)
log_address(770-772)log_data(775-794)plugwise_usb/messages/requests.py (18)
CircleEnergyLogsRequest(1131-1165)response(124-128)send(366-375)send(388-397)send(439-441)send(477-486)send(513-522)send(543-554)send(567-576)send(589-598)send(622-631)send(668-677)send(720-729)send(776-785)send(809-818)send(849-858)send(881-890)send(913-922)plugwise_usb/nodes/helpers/counter.py (4)
timestamp(179-187)add_empty_log(70-72)update(147-176)update(286-328)
⏰ 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 (11)
plugwise_usb/nodes/circle.py (11)
5-5: Top-level asyncio import for gather resolves PLC0415 and is correctImporting gather at module scope is the right fix and matches its usage below.
11-11: Use of ceil for address grouping aligns with hour-to-address mappingUsing ceil(...) for computing the number of addresses to cover the current day is appropriate.
77-78: MAX_LOG_HOURS derived from DAY_IN_HOURS is clear and matches PR goalKeeps recency filtering bounded to one day by default, as intended.
370-372: Initial last-log request now follows boolean contractStoring the boolean result from energy_log_update into _last_energy_log_requested is consistent and keeps retry semantics intact.
384-391: Rollover path: boolean handling and diagnostics look goodChecking the boolean result and logging the address on failure provides useful signal without exceptions.
397-405: Prev-address retry on rollover is correctcalc_log_address(..., -4) for the previous address is appropriate; logging on failure is helpful.
479-486: Early stop on empty/outdated address is sensibleHalting initial collection when a whole address is stale prevents unnecessary older fetches and matches the MAX_LOG_HOURS constraint.
516-525: Cancel-and-drain pattern avoids “Task exception was never retrieved”Good: cancel remaining tasks and drain them with gather(..., return_exceptions=True).
560-567: Empty or outdated slot handling is correctMarking empty/outdated slots explicitly keeps PulseCollection ranges coherent for missing-address computation.
569-576: import_only=True is appropriate during batch importDefers intermediate statistics recalculations until after the loop, then calls update() once.
579-585: Write cache only when a new record was storedPrevents unnecessary I/O when the response was entirely empty/outdated.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
471-474: Production-interval check misses NO_PRODUCTION_INTERVALWhen production logging is disabled via NO_PRODUCTION_INTERVAL (not None), the current check treats it as “production present” and uses factor=2, leading to over-collection. Consider both None and NO_PRODUCTION_INTERVAL as “consumption-only.”
- cons_only = self.energy_production_interval is None - factor = 4 if cons_only else 2 + cons_only = self.energy_production_interval in (None, NO_PRODUCTION_INTERVAL) + factor = 4 if cons_only else 2
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle.py (3)
456-457: Docstring wording doesn’t match behavior (today vs recent hours)The logic can include records from the previous day as long as they are within MAX_LOG_HOURS. To avoid confusion, update the docstring to “recent hours” rather than “hours elapsed today”.
- """Collect initial energy logs for the hours elapsed today up to MAX_LOG_HOURS.""" + """Collect initial energy logs for recent hours up to MAX_LOG_HOURS."""
531-549: Avoid redundant cache writes from batch updatesenergy_log_update saves the cache when any record is stored, and get_missing_energy_logs subsequently saves the entire collection again. This can cause unnecessary I/O churn during batch updates. Consider allowing the caller to suppress the per-call save and rely on the aggregated save at the end of the batch.
- async def energy_log_update(self, address: int | None) -> bool: + async def energy_log_update(self, address: int | None, *, save_cache: bool = True) -> bool: @@ - if any_record_stored and self._cache_enabled: + if any_record_stored and self._cache_enabled and save_cache: _LOGGER.debug( "Saving energy record update to cache for %s", self._mac_in_str ) await self.save_cache()And call with save_cache=False from batch/initial collection paths:
- result = await self.energy_log_update(log_address) + result = await self.energy_log_update(log_address, save_cache=False)- tasks = [ - create_task(self.energy_log_update(address)) + tasks = [ + create_task(self.energy_log_update(address, save_cache=False)) for address in missing_addresses ]Also applies to: 580-587
588-595: Minor: future timestamps due to drift — treat as recent explicitlyIf node clocks are slightly ahead, age_seconds can be negative. Explicitly clamping to 0 avoids edge-case surprises and keeps intent clear.
- age_seconds = ( - datetime.now(tz=UTC) - timestamp.replace(tzinfo=UTC) - ).total_seconds() + age_seconds = max( + 0.0, + (datetime.now(tz=UTC) - timestamp.replace(tzinfo=UTC)).total_seconds(), + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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
📚 Learning: 2025-08-13T13:20:25.164Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#311
File: plugwise_usb/nodes/circle.py:550-551
Timestamp: 2025-08-13T13:20:25.164Z
Learning: In the plugwise_usb library, the PlugwiseMessage class always provides a timestamp in responses, so defensive null checks for response.timestamp are not necessary when calling _available_update_state().
Applied to files:
plugwise_usb/nodes/circle.py
🔇 Additional comments (8)
plugwise_usb/nodes/circle.py (8)
5-5: Top-level asyncio.gather and math.ceil import — good changeResolves PLC0415 and enables proper batching and rounding logic later.
Also applies to: 11-11
28-28: MAX_LOG_HOURS tied to DAY_IN_HOURS — aligned with PR objectiveClear, single source of truth for the recency window. Good.
Also applies to: 77-78
370-372: Correctly adopting the new boolean contract from energy_log_updateThis aligns the initial “last log” request with the simplified True/False return.
384-391: Rollover handling now logs addresses and retries previous address — solidThe explicit logging and retry on previous address improves debuggability and robustness during rollover.
Also applies to: 396-404
417-426: Single-missing-address fast path looks goodShort-circuiting when the single update succeeds reduces overhead and matches the new return semantics.
474-476: Confirm intended scope for initial collection (today-only vs last N hours)The formula
ceil(datetime.now(tz=UTC).hour / factor) + 1can pull an extra previous-day address during the early hours (e.g., at 01:xx with consumption-only). If your intent is “recent hours within MAX_LOG_HOURS,” this is fine and the docstring should reflect that (see previous comment). If you actually want “today-only,” considerceil((datetime.now(tz=UTC).hour + 1) / factor)instead.
479-487: Early break on first non-recent/empty address — good stop conditionPrevents scanning further back once the current address has no usable data (handles rollover gracefully).
517-526: Properly awaiting canceled tasksCanceling and draining with gather(return_exceptions=True) avoids “Task exception was never retrieved” and resource leaks. Nice.
bouwew
force-pushed
the
improve-energy-collection
branch
from
cbd8902 to
f6c4d36
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
456-477: Consider NO_PRODUCTION_INTERVAL when determining consumption-only factorWhen production logging is disabled, energy_production_interval may be None or equal to NO_PRODUCTION_INTERVAL. The current check only handles None, which will misclassify “consumption-only” as “production-enabled” when the interval equals NO_PRODUCTION_INTERVAL, halving the initial address count.
Apply:
- cons_only = self.energy_production_interval is None + cons_only = self.energy_production_interval in (None, NO_PRODUCTION_INTERVAL) factor = 4 if cons_only else 2This matches the documented mapping:
- consumption-only → 1 address covers 4 hours
- consumption+production → 1 address covers 2 hours
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle.py (3)
370-372: Avoid repeated last-log re-requests: set the flag independent of storage successCurrently, _last_energy_log_requested is set to True only when energy_log_update returns True (i.e., when at least one recent, non-empty record was stored). If the last address contains no recent data, this will keep re-requesting the same last address on every energy_update call, adding network churn with no benefit.
Set the flag after the first attempt to prevent repeated calls; cache/save behavior is already controlled by save_cache and the subsequent flow.
Proposed diff:
- self._last_energy_log_requested = await self.energy_log_update( - self._current_log_address, save_cache=False - ) + await self.energy_log_update(self._current_log_address, save_cache=False) + self._last_energy_log_requested = TrueIf you intentionally want to keep retrying until a recent record is present, consider renaming the attribute to reflect that behavior (e.g., _last_energy_log_present) to avoid confusion.
531-587: Minor: use response.log_address in recency check for accuracy in logsIf the device ever responds with a different address than requested (rare, but the code already trusts response.log_address for storage), use response.log_address in _check_timestamp_is_recent for consistent logging context.
Proposed diff:
- or not self._check_timestamp_is_recent(address, _slot, log_timestamp) + or not self._check_timestamp_is_recent( + response.log_address, _slot, log_timestamp + )Also, the docstring says “Return True only when at least one recent, non-empty record was stored.” The code sets any_record_stored = True for every recent, non-empty record processed, regardless of whether it changed the cache (dedup). That’s fine if “stored” refers to in-memory counters; if you intend “stored in cache,” consider OR-ing the boolean returned by _energy_log_record_update_state instead.
588-605: Reduce log level for expected “outdated” records to avoid noiseOutdated records are expected when enforcing a sliding window and during rollovers. Warning-level logs can be noisy. Recommend downgrading to debug (or info if you want visibility by default).
Proposed diff:
- _LOGGER.warning( + _LOGGER.debug( "EnergyLog from Node %s | address %s | slot %s | timestamp %s is outdated, ignoring...", self._mac_in_str, address, slot, timestamp, )If desired, I can add unit tests targeting:
- Boundary behavior at exactly MAX_LOG_HOURS age (should be accepted).
- Slightly over the limit (should be rejected).
- Clock skew scenarios (negative age clamped to zero).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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
📚 Learning: 2025-08-13T13:20:25.164Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#311
File: plugwise_usb/nodes/circle.py:550-551
Timestamp: 2025-08-13T13:20:25.164Z
Learning: In the plugwise_usb library, the PlugwiseMessage class always provides a timestamp in responses, so defensive null checks for response.timestamp are not necessary when calling _available_update_state().
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/circle.py (7)
plugwise_usb/api.py (1)
save_cache(368-371)plugwise_usb/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/node.py (3)
save_cache(705-719)energy_consumption_interval(179-185)energy_production_interval(189-195)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)plugwise_usb/nodes/helpers/pulses.py (2)
calc_log_address(20-36)add_empty_log(389-421)plugwise_usb/messages/responses.py (2)
log_address(770-772)log_data(775-794)plugwise_usb/messages/requests.py (18)
CircleEnergyLogsRequest(1131-1165)response(124-128)send(366-375)send(388-397)send(439-441)send(477-486)send(513-522)send(543-554)send(567-576)send(589-598)send(622-631)send(668-677)send(720-729)send(776-785)send(809-818)send(849-858)send(881-890)send(913-922)
🔇 Additional comments (4)
plugwise_usb/nodes/circle.py (4)
5-5: MAX_LOG_HOURS introduction and import adjustments look good
- Importing gather and ceil is appropriate for the new task-draining and address math.
- Using DAY_IN_HOURS to define MAX_LOG_HOURS aligns the code to a clear, single source of truth for “last 24h”.
Also applies to: 11-11, 28-28, 77-78
384-391: Rollover handling path is consistent and clear
- Correctly retries current and previous addresses and handles failures by exiting early.
- Logging provides sufficient context for diagnosing rollover issues.
Also applies to: 396-405
479-486: Early stop on empty/outdated initial address is sensibleBreaking the initial collection when the address yields only empty/outdated slots avoids unnecessary requests and aligns with the recency constraint.
514-526: Good: cancel and drain remaining tasks on first False result
- Properly cancels remaining tasks and awaits them with gather(..., return_exceptions=True), preventing “Task exception was never retrieved” issues.
- Top-level import of gather resolves PLC0415.
Don't store energy-log data when it's outside the time-interval of interest (MAX_LOG_HOURS).
Summary by CodeRabbit
New Features
Reliability
Documentation