Alternative solution for handling EnergyLogs outside MAX_LOG_HOURS #307
Conversation
WalkthroughThe PR refactors energy log handling in plugwise_usb/nodes/circle.py: changes energy_log_update to return a boolean, introduces a recency check using DAY_IN_HOURS, simplifies initial log collection, and overhauls cache parsing/serialization to maintain reverse-sorted, recency-filtered records. pyproject.toml updates the version to 0.44.11a1. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Startup/Timer
participant Circle as PlugwiseCircle
participant Device as USB Device
participant Cache as Cache Store
Caller->>Circle: energy_log_update(address)
alt address is None
Circle-->>Caller: False
else address provided
Circle->>Device: request energy logs
Device-->>Circle: records (pulses, timestamp, slot)
loop for each record
Circle->>Circle: _check_timestamp_is_recent(slot, timestamp)
alt recent
Circle->>Cache: add_pulse_log(record)
Circle->>Cache: save reverse-sorted cache
else outdated
Circle->>Cache: add empty log for slot
end
end
Circle-->>Caller: True if any recent non-empty stored else False
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ 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 #307 +/- ##
==========================================
+ Coverage 80.54% 80.61% +0.07%
==========================================
Files 36 36
Lines 7540 7543 +3
==========================================
+ Hits 6073 6081 +8
+ Misses 1467 1462 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
plugwise_usb/nodes/circle.py (4)
549-551: Typo in comment“store am empty record instead” → “store an empty record instead”.
668-672: Ensure deterministic cache serialization orderThe comment says “logs is already sorted in reverse”, but dict ordering depends on insertion order. Unless EnergyCounters.get_pulse_logs guarantees reverse insertion, cache order may become unstable. Consider explicit sorting and iterating items directly:
- # logs is already sorted in reverse - for address, record in logs.items(): - for slot in record: - log = record[slot] + # Ensure deterministic reverse order + for address, record in sorted(logs.items(), reverse=True): + for slot, log in sorted(record.items(), reverse=True):This trades minimal CPU for stable cache diffs and simpler troubleshooting.
710-711: Avoid trailing delimiter when cache is emptyWhen cached_logs == "", the assignment produces "new|", leaving a trailing pipe. While harmless on load, it’s easy to avoid:
- self._set_cache( - CACHE_ENERGY_COLLECTION, log_cache_record + "|" + cached_logs - ) + sep = "" if cached_logs == "" else "|" + self._set_cache( + CACHE_ENERGY_COLLECTION, log_cache_record + sep + cached_logs + )
456-466: Docstring vs. implementation: off-by-one in number of addressesDocstring says “last 10 log addresses,” but total_addresses is 11, including current + 10 previous. Either update the docstring to “last 11 (current + 10 previous) log addresses” or set total_addresses = 10 to match the text.
Also applies to: 468-476
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(11 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.163Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.
📚 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-06-20T07:58:41.534Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-07T17:50:29.163Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.163Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.
Applied to files:
plugwise_usb/nodes/circle.py
⏰ 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)
plugwise_usb/nodes/circle.py (2)
27-34: Consistency of “max age” constantYou’re using DAY_IN_HOURS as the recency ceiling. If the project defines a distinct MAX_LOG_HOURS elsewhere (as the PR title suggests), consider using that to keep semantics aligned. If not, DAY_IN_HOURS is fine—just confirm intent.
Would you like me to scan constants for a MAX_LOG_HOURS (or similar) and propose a single source of truth?
Also applies to: 582-599
369-371: Verified: energy_log_update usages now consistently return a single booleanAll call sites assign the result to a single variable (or schedule a task), and no tuple unpacking remains. Ready to merge.
bouwew
force-pushed
the
ener-logs-cache-3
branch
from
ddaed7e to
dd65ed9
Compare
bouwew
force-pushed
the
ener-logs-cache-3
branch
from
dd65ed9 to
350451a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
579-598: Previous review already noted that logging every outdated slot at WARNING may spam logs during catch-up; consider downgrading to DEBUG/INFO.
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle.py (3)
468-475: Rephrase debug message for clarity“Energy None-data of outdated data …” is hard to parse; consider:
- _LOGGER.debug( - "Energy None-data of outdated data collected from log address %s, stopping collection", - log_address, - ) + _LOGGER.debug( + "All slots at log address %s are empty or outdated – stopping initial collection", + log_address, + )
511-578: Clarify return semantics in docstring
energy_log_updatenow returnsTrueonly when at least one recent, non-empty record was stored.
Update the docstring accordingly to avoid confusion for future callers.- """Request energy log statistics from node. Returns true if successful.""" + """Request energy logs and return True only when at least one recent, non-empty + record was stored; otherwise return False."""
667-676: Don’t rely on dict insertion order for cache serialization
logsis assumed “already sorted”, but dict key order is an implementation detail ofEnergyCounters. For deterministic cache output:- for address, record in logs.items(): + for address in sorted(logs.keys(), reverse=True): + record = logs[address]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(11 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 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.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.163Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.
📚 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-06-20T07:58:41.534Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:615-627
Timestamp: 2025-06-20T07:58:41.534Z
Learning: In the plugwise_usb/nodes/circle.py file, the `log_addresses_missing` property being `None` means the parameter has not been initialized yet, not that all logs are present. When `log_addresses_missing` is `None`, returning `False` from cache loading methods is correct because the system is not properly initialized and further execution would be useless.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-07T17:50:29.163Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.163Z
Learning: In the plugwise_usb codebase, the `_load_defaults` method in node classes (scan.py, switch.py, sense.py) is only called once during node initialization, not during runtime. Therefore, flags like `_sed_node_info_update_task_scheduled` don't need to be reset at the start of `_load_defaults` as there's no risk of stale state from previous loads.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/circle.py (5)
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/nodes/helpers/pulses.py (1)
add_empty_log(389-421)plugwise_usb/nodes/node.py (1)
save_cache(704-718)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: Process test coverage
🔇 Additional comments (6)
plugwise_usb/nodes/circle.py (6)
26-34: No issues – just an extra constant imported.
368-371: Looks fine; aligns flag with new boolean return.
382-390: Call-site update matches the new signature.
396-404: Call-site update matches the new signature.
417-426: Call-site update matches the new signature.
708-712: Prepended-record change looks good.
bouwew
force-pushed
the
ener-logs-cache-3
branch
from
350451a to
1704df4
Compare
this fits better to the reverse sorting of the data in the cache
improve docstrings
bouwew
force-pushed
the
ener-logs-cache-3
branch
from
1704df4 to
6776f83
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
plugwise_usb/nodes/circle.py (5)
368-371: Naming may mislead: _last_energy_log_requested now stores a success booleanSince energy_log_update returns “stored any recent record”, assigning it to _last_energy_log_requested conflates “requested” with “had data”. Consider renaming to reflect semantics (e.g., _initial_energy_logs_collected) or add a clarifying comment.
- self._last_energy_log_requested = await self.energy_log_update( + # Tracks whether this initial request yielded at least one recent, non-empty record + self._last_energy_log_requested = await self.energy_log_update( self._current_log_address )
467-475: Break-on-empty/outdated now works; check wording vs count and log style
- Behavior: good. The loop now exits when only empty/outdated slots are returned.
- Minor: the debug above says “last 10 log addresses” but total_addresses is 11; verify intended count.
- Minor: consider replacing the en dash with a hyphen in logs for consistency across environments.
Would you like to adjust total_addresses or the log message for consistency?
541-549: Typo in comment: “am empty” → “an empty”Small grammar fix.
- # Don't store an old log-record, store am empty record instead + # Don't store an old log record, store an empty record instead
569-576: Avoid redundant cache saves during batched retrievalsenergy_log_update saves the cache for each address. In get_missing_energy_logs you already save once after awaiting all tasks. To reduce churn and potential interleaving, make saving optional and disable it for batch calls.
- 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: + if any_record_stored and save_cache: _LOGGER.debug( "Saving energy record update to cache for %s", self._mac_in_str ) await self.save_cache()Then call with save_cache=False in batched contexts:
# get_missing_energy_logs tasks = [create_task(self.energy_log_update(address, save_cache=False)) for address in missing_addresses] # _get_initial_energy_logs result = await self.energy_log_update(log_address, save_cache=False)
578-597: 24h boundary bug fixed; consider lowering log level to avoid noiseUsing a direct seconds comparison removes the floor-division bug. Consider downgrading the warning to info/debug during catch-up to avoid log spam when many outdated entries are encountered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(11 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/pulses.py (2)
add_empty_log(389-421)logs(110-124)plugwise_usb/nodes/helpers/counter.py (2)
add_empty_log(70-72)timestamp(179-187)
⏰ 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 (6)
plugwise_usb/nodes/circle.py (6)
27-27: Import DAY_IN_HOURS is appropriate for 24h recency logicThe constant is used correctly in _check_timestamp_is_recent.
382-404: Rollover handling now cleanly leverages boolean resultUsing the simplified boolean from energy_log_update improves readability of failure handling and the fallback to previous address.
417-425: Single-missing-address path updated correctlySwitch to the boolean return type is consistent and correct here.
516-520: Fix for logging argument order looks good“node %s address %s” now receives (mac, address) in the correct order.
708-709: Prepending new cache records aligns with reverse chronological orderThis improves cache readability and keeps recent data first.
666-669: Confirmed reverse-sorted pulse logs — no changes needed
Theget_pulse_logs()method delegates toPulseCollection.logs, which sorts both addresses and slots in descending order before building the dict, so the iteration order assumption is valid.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
551-558: Critical: Return value still depends on cache being enabled; decouple any_record_stored.
This repeats a previously flagged issue: _energy_log_record_update_state returns False when cache is disabled, so energy_log_update will incorrectly return False even when a valid, recent record was added to in-memory counters. This breaks initial collection and other callers that rely on True meaning “we updated something.”Apply this diff to set any_record_stored independent of cache writes:
- if await self._energy_log_record_update_state( + # Store the record; treat it as "stored" regardless of cache availability + await self._energy_log_record_update_state( response.log_address, _slot, log_timestamp.replace(tzinfo=UTC), log_pulses, import_only=True, - ): - any_record_stored = True + ) + any_record_stored = True
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
542-549: Nit: fix comment typo and clarity (“am” -> “an”).
Minor, but it reads better and avoids confusion.- # Don't store an old log-record, store am empty record instead + # Don't store an old log-record, store an empty record instead
681-685: Ensure deterministic slot ordering when serializing cache.
Relying on insertion order can be fragile; explicitly sort slots in reverse to match the intended “already reversed” invariant.- # logs is already sorted in reverse - for address, record in logs.items(): - for slot in record: + # logs is already sorted in reverse on addresses; sort slots explicitly + for address, record in logs.items(): + for slot in sorted(record.keys(), reverse=True): log = record[slot]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(12 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 (3)
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/pulses.py (3)
add_empty_log(389-421)PulseLogRecord(40-45)logs(110-124)plugwise_usb/nodes/node.py (1)
save_cache(705-719)
🪛 GitHub Actions: Latest commit
plugwise_usb/nodes/circle.py
[error] 600-600: Ruff: PLR0912 Too many branches (14 > 12). Command: 'ruff check plugwise_usb/ tests/'
[error] 641-641: F821 Undefined name 'timedelta'. Ensure 'timedelta' is imported (e.g., from datetime import timedelta). Command: 'ruff check plugwise_usb/ tests/'
[error] 647-647: F821 Undefined name 'sorted_log'. Command: 'ruff check plugwise_usb/ tests/'
[error] 648-648: F821 Undefined name 'sorted_log'. Command: 'ruff check plugwise_usb/ tests/'
[error] 649-649: F821 Undefined name 'sorted_log'. Command: 'ruff check plugwise_usb/ tests/'
🔇 Additional comments (9)
plugwise_usb/nodes/circle.py (9)
27-30: Constants import looks good; matches intent to bound log retention window.
Importing DAY_IN_HOURS and MAX_LOG_HOURS is appropriate for recency and cache pruning logic.
369-371: Startup: single-boolean result usage is correct.
Storing the boolean from energy_log_update in _last_energy_log_requested aligns with the new return semantics.
383-405: Rollover handling updated correctly to the new boolean return.
Both the current and previous addresses are retried with clear logging on failure.
418-426: Missing-address single fetch path is correct.
The new boolean contract integrates cleanly and the follow-up power_update is preserved.
468-476: Initial collection: break condition now meaningful, but beware cache-coupling downstream.
Breaking on “no recent, non-empty slots” is correct. However, this depends on energy_log_update returning True when in-memory records were stored even if cache is disabled. See my separate comment on decoupling any_record_stored from cache writes.
511-521: Return semantics and logging clarified—good.
Docstring clearly defines “success,” and the node/address logging order is fixed.
525-529: Early return on missing response is correct.
Aligns with the new single-boolean contract and avoids ambiguous “success but nothing stored.”
530-531: Extra debug line is helpful for tracing.
Logging the source of EnergyLogs with node and address aids diagnostics.
571-577: Cache save trigger on actual updates—good.
Only saving on any_record_stored avoids unnecessary writes.
bouwew
force-pushed
the
ener-logs-cache-3
branch
from
989cfca to
c89456e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
599-654: Cache loading logic needs consistency with MAX_LOG_HOURSThe cache loading and pruning logic uses
DAY_IN_HOURSfor the skip_before threshold. For consistency with the PR objective and the rest of the codebase, this should useMAX_LOG_HOURS.# Sort and prune the records loaded from cache sorted_logs: dict[int, dict[int, tuple[int, datetime]]] = {} - skip_before = datetime.now(tz=UTC) - timedelta(hours=DAY_IN_HOURS) + skip_before = datetime.now(tz=UTC) - timedelta(hours=MAX_LOG_HOURS) sorted_addresses = sorted(restored_logs.keys(), reverse=True)
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
578-597: Recency check should use MAX_LOG_HOURS, not DAY_IN_HOURSPer the PR objective of handling logs outside
MAX_LOG_HOURS, the recency check should useMAX_LOG_HOURSinstead of the hardcodedDAY_IN_HOURS(24h). This ensures consistency with the pruning logic used elsewhere in the codebase.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 from within the last 24 hours is considered recent. + A timestamp newer than MAX_LOG_HOURS is considered recent. """ age_seconds = ( datetime.now(tz=UTC) - timestamp.replace(tzinfo=UTC) ).total_seconds() - if age_seconds > DAY_IN_HOURS * 3600: + if age_seconds > MAX_LOG_HOURS * 3600: _LOGGER.warning( "EnergyLog from Node %s | address %s | slot %s | timestamp %s is outdated, ignoring...", self._mac_in_str, address, slot, timestamp, ) return False return True
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
632-632: Remove debug log statementThe debug statement "HOI restored_logs=%s" appears to be a temporary debugging aid and should be removed.
- _LOGGER.debug("HOI restored_logs=%s", restored_logs)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(13 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 (3)
add_empty_log(70-72)timestamp(179-187)add_pulse_log(74-89)plugwise_usb/nodes/helpers/pulses.py (2)
add_empty_log(389-421)logs(110-124)
🔇 Additional comments (3)
plugwise_usb/nodes/circle.py (3)
510-576: Return semantics are correct and match PR objectivesThe refactored
energy_log_updatemethod correctly returnsTrueonly when at least one recent, non-empty record was stored. The recency check using_check_timestamp_is_recentensures outdated logs are filtered out as per PR objectives. The logging has been improved for clarity.
455-477: Simplified initial collection logic is cleanerThe refactored
_get_initial_energy_logsmethod is much simpler and correctly relies on the boolean return fromenergy_log_updateto determine when to stop collection. This aligns well with the PR objectives.
726-726: Cache record prepending maintains chronological orderThe change to prepend new records (
log_cache_record + "|" + cached_logs) ensures that newer records appear first in the cache, maintaining a reverse chronological order that aligns with the sorting strategy used throughout the code.
bouwew
force-pushed
the
ener-logs-cache-3
branch
from
800f307 to
ada08b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
plugwise_usb/nodes/circle.py (4)
27-34: Use MAX_LOG_HOURS for recency window (aligns with configuration and EnergyCounters)
The PR objective targets handling logs outside MAX_LOG_HOURS. Using DAY_IN_HOURS hard-codes 24h and diverges from configured behavior and the pulses logs pruning (which uses MAX_LOG_HOURS). Align both to avoid inconsistencies.Apply this diff:
from ..constants import ( - DAY_IN_HOURS, + DAY_IN_HOURS, + MAX_LOG_HOURS, @@ - A timestamp from within the last 24 hours is considered recent. + A timestamp newer than MAX_LOG_HOURS is considered recent. @@ - if age_seconds > DAY_IN_HOURS * 3600: + if age_seconds > MAX_LOG_HOURS * 3600:Optional: consider downgrading the log level to info/debug to reduce noise during normal catch-up scenarios.
Also applies to: 583-589
467-474: Initial collection loop will terminate prematurely when cache is disabled (depends on fix above)
Because energy_log_update currently ties its return value to cache writes, this loop breaks on valid-but-uncached records. After decoupling any_record_stored from caching (see earlier comment), this loop will behave as intended.
550-557: Decouple return value from cache state to avoid false negatives when cache is disabledany_record_stored is only set when _energy_log_record_update_state returns True, but that returns False when caching is disabled. This makes energy_log_update return False even when valid records were added to in-memory counters, breaking initial collection and rollover handling.
Apply this diff to set any_record_stored based on processing of recent, non-empty slots, regardless of cache writes, and avoid unnecessary save attempts when cache is disabled:
- if await self._energy_log_record_update_state( + await self._energy_log_record_update_state( response.log_address, _slot, log_timestamp.replace(tzinfo=UTC), log_pulses, import_only=True, - ): - any_record_stored = True + ) + any_record_stored = True if not last_energy_timestamp_collected: # Collect the timestamp of the most recent response self._last_collected_energy_timestamp = log_timestamp.replace( tzinfo=UTC ) @@ - if any_record_stored: + if any_record_stored and self._cache_enabled: _LOGGER.debug( "Saving energy record update to cache for %s", self._mac_in_str ) await self.save_cache()Also applies to: 570-575
634-645: Prune loaded cache using MAX_LOG_HOURS instead of DAY_IN_HOURS
Keep the same recency window everywhere to prevent reloading/pruning inconsistencies versus EnergyCounters.get_pulse_logs().- skip_before = datetime.now(tz=UTC) - timedelta(hours=DAY_IN_HOURS) + skip_before = datetime.now(tz=UTC) - timedelta(hours=MAX_LOG_HOURS)
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
632-633: Remove stray debug log (“HOI restored_logs”)
This looks like a leftover dev log and is noisy.- _LOGGER.debug("HOI restored_logs=%s", restored_logs)
541-547: Nit: fix typo in comment
Minor grammar nit in the inline comment.- # Don't store an old log-record, store am empty record instead + # Don't store an old log-record; store an empty record instead
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(13 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/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/pulses.py (2)
add_empty_log(389-421)logs(110-124)plugwise_usb/nodes/helpers/counter.py (3)
add_empty_log(70-72)timestamp(179-187)add_pulse_log(74-89)plugwise_usb/nodes/node.py (2)
_get_cache(652-656)name(218-222)
⏰ 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)
plugwise_usb/nodes/circle.py (2)
517-520: Logging placeholder order fixed — correct mapping of node and addressArgs now match the message placeholders: node MAC first, then address. Good catch.
680-687: LGTM: cache serialization uses already-pruned/sorted logs
Using EnergyCounters.get_pulse_logs(), which returns a pruned, reverse-sorted view, is the right approach and keeps cache compact.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyproject.toml(1 hunks)scripts/tests_and_coverage.sh(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (1)
📓 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.
⏰ 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: Ruff check and force
scripts/tests_and_coverage.sh
Outdated
| # PYTHONPATH=$(pwd) pytest -qx tests/ --cov='.' --no-cov-on-fail --cov-report term-missing || | ||
| PYTHONPATH=$(pwd) pytest -xrpP --log-level debug tests/ |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Retaining optional coverage and CI-friendly behavior (avoid -x in CI).
You removed coverage collection and now abort on first failure unconditionally. If CI or release gating depends on coverage (Codecov/Coveralls/thresholds), this is a regression; and -x reduces failure signal in CI. Suggest toggling coverage and -x via env or arg.
Apply this diff to restore coverage optionally and keep fast local runs:
- # PYTHONPATH=$(pwd) pytest -qx tests/ --cov='.' --no-cov-on-fail --cov-report term-missing ||
- PYTHONPATH=$(pwd) pytest -xrpP --log-level debug tests/
+ if [ "${COVERAGE:-0}" = "1" ]; then
+ # CI/coverage runs: collect coverage, run full suite (no -x)
+ PYTHONPATH=$(pwd) pytest -q tests/ --cov='.' --no-cov-on-fail --cov-report=term-missing
+ else
+ # Local fast feedback: stop on first failure with detailed reporting
+ PYTHONPATH=$(pwd) pytest -xrpP --log-level debug tests/
+ fiTo verify whether coverage is still required by your pipelines, run:
🏁 Script executed:
#!/bin/bash
# Check for coverage dependencies/config or usage in CI
set -euo pipefail
echo "Searching for coverage configs and usage..."
fd -H -a -t f '.coveragerc' || true
rg -n -A2 -S --hidden -e 'tool\.coverage|--cov|coverage xml|coverage:|codecov|coveralls' || true
echo "Inspecting GitHub Actions workflows for coverage steps..."
fd -H -a -t f '.github/workflows' | xargs -I{} rg -n -A2 -S 'coverage|codecov|coveralls' {} || true
echo "Searching pyproject.toml/tox.ini for coverage config..."
fd -a -t f 'pyproject.toml' 'tox.ini' | xargs -I{} rg -n -A2 -S 'coverage|--cov' {} || trueLength of output: 2947
Restore optional coverage runs and CI-friendly behavior
We’ve confirmed that coverage is required by CI (see pyproject.toml’s [tool.coverage.run] section and the verify.yml workflow steps running pytest with --cov and uploading to Codecov). Removing coverage collection and unconditionally using -x will break CI metrics and coverage gating.
Please update scripts/tests_and_coverage.sh (around lines 60–66) as follows:
File: scripts/tests_and_coverage.sh lines 60–66
- # PYTHONPATH=$(pwd) pytest -qx tests/ --cov='.' --no-cov-on-fail --cov-report term-missing ||
- PYTHONPATH=$(pwd) pytest -xrpP --log-level debug tests/
+ if [ "${COVERAGE:-0}" = "1" ]; then
+ # CI/coverage runs: collect coverage, no fast-fail
+ PYTHONPATH=$(pwd) pytest -q tests/ \
+ --cov='.' --no-cov-on-fail --cov-report=term-missing
+ else
+ # Local fast feedback: stop on first failure with debug logging
+ PYTHONPATH=$(pwd) pytest -xrpP --log-level debug tests/
+ fiThis preserves coverage collection for CI and retains fast local feedback.
🤖 Prompt for AI Agents
In scripts/tests_and_coverage.sh around lines 60 to 66, the current change
removed coverage collection and forces -x unconditionally which will break CI
coverage gating; restore CI-friendly behavior by running pytest with coverage
when in CI (detect via CI or GITHUB_ACTIONS env var) using PYTHONPATH=$(pwd)
pytest -qx tests/ --cov='.' --no-cov-on-fail --cov-report term-missing (so CI
metrics/upload work), and for local dev run the faster command PYTHONPATH=$(pwd)
pytest -xrpP --log-level debug tests/; ensure the script chooses the coverage
command when CI is set and preserves exit codes so failures fail the CI job.
ce3b615 to
9285048
Compare
|
|
Split in #311 and a 2nd future PR. |
Summary by CodeRabbit