Line up energy_logs cache-content with energy_logs collection in memory #329
Conversation
preventing the storage of outdated data in the cache-file
WalkthroughRefactors energy log handling in plugwise_usb/nodes/circle.py: energy_log_update now returns (result, slots_empty), centralizes cache persistence via save_cache(), updates startup/rollover/initial/missing-log flows and unload to use new semantics, bumps project version, and updates CHANGELOG. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as Controller
participant Circle as PlugwiseCircle
participant Mem as In-memory Cache
participant Disk as Disk (save_cache)
Note over Circle,Mem: Startup / single update
App->>Circle: energy_log_update(addr, save_cache=false)
activate Circle
Circle->>Mem: update in-memory slots
Circle-->>App: (result, slots_empty)
deactivate Circle
rect rgba(230,245,255,0.5)
Note over App,Circle: Rollover handling
App->>Circle: energy_log_update(cur_addr, save_cache=false)
Circle-->>App: (cur_ok, cur_empty)
alt cur_ok
App->>Circle: energy_log_update(prev_addr, save_cache=false)
Circle-->>App: (prev_ok, prev_empty)
alt cur_empty == false or prev_empty == false
App->>Circle: save_cache()
Circle->>Disk: persist cache
Disk-->>Circle: ok
else
Note over App: no disk write
end
else
Note over App: halt on failure
end
end
rect rgba(235,255,235,0.5)
Note over App,Circle: Missing logs (parallel)
par for each addr
App->>Circle: energy_log_update(addr, save_cache=false)
Circle-->>App: (ok, slots_empty)
and ...
end
alt any slots not empty
App->>Circle: save_cache()
Circle->>Disk: persist cache
Disk-->>Circle: ok
else
Note over App: skip persistence
end
end
rect rgba(255,240,230,0.5)
Note over App,Circle: Unload
App->>Circle: save_cache()
Circle->>Disk: persist cache
Disk-->>Circle: ok
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 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)
⏰ 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)
✨ 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 #329 +/- ##
==========================================
+ Coverage 80.56% 80.70% +0.13%
==========================================
Files 36 36
Lines 7564 7560 -4
==========================================
+ Hits 6094 6101 +7
+ Misses 1470 1459 -11 ☔ 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
597-626: Fix cache_updated aggregation so cache flush isn’t skipped.Inside energy_log_update(), cache_updated is overwritten per slot; if an earlier slot updated but a later didn’t, save_cache won’t trigger, risking loss on restart.
Apply:
- cache_updated = False - for _slot in range(4, 0, -1): + cache_updated = 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 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 - cache_updated = await self._energy_log_record_update_state( + updated = await self._energy_log_record_update_state( response.log_address, _slot, log_timestamp.replace(tzinfo=UTC), log_pulses, import_only=True, ) + cache_updated = cache_updated or updated
🧹 Nitpick comments (4)
plugwise_usb/nodes/circle.py (4)
534-535: Optional: force synchronous flush at batch ends.Initial and “missing logs” bulk imports end with await self.save_cache(), which schedules a write (trigger_only=True). If you want stronger durability immediately after batch imports, flush synchronously.
- await self.save_cache() + await self.save_cache(trigger_only=False)Also applies to: 569-570
744-745: Avoid re-serializing the full energy log cache per record._energy_log_record_update_state() serializes the entire logs set for every new record, multiplying work during bulk imports.
Batch once per address:
- _LOGGER.debug( - "Adding logrecord (%s, %s) to cache of %s", - str(address), - str(slot), - self._mac_in_str, - ) - await self._energy_log_records_save_to_cache() + _LOGGER.debug( + "Adding logrecord (%s, %s) to cache of %s", + str(address), + str(slot), + self._mac_in_str, + ) return True @@ - _LOGGER.debug( + _LOGGER.debug( "Cache is empty, adding new logrecord (%s, %s) for %s", str(address), str(slot), self._mac_in_str, ) - await self._energy_log_records_save_to_cache() return TrueAnd, in energy_log_update(), save the cache string once per address when anything changed:
- self._energy_counters.update() - if cache_updated and save_cache: + self._energy_counters.update() + if cache_updated: + await self._energy_log_records_save_to_cache() + if cache_updated and save_cache: _LOGGER.debug( "Saving energy record update to cache for %s", self._mac_in_str ) await self.save_cache()Also applies to: 758-759
574-574: Docstring doesn’t match intended semantics.Per prior learning (PR #314), True indicates successful processing/storage in memory, not necessarily “at least one recent, non-empty record stored.”
Using that learning: update the docstring for clarity.
- """Request energy logs and return True only when at least one recent, non-empty record was stored; otherwise return False.""" + """Request energy logs; return True if processing succeeded (records stored in memory), regardless of whether new entries were added. Return False on transport or address errors."""
1182-1184: Typo in log message.“cachefor” → “cache for”.
- "Saving relay_init state update to cachefor %s", self._mac_in_str + "Saving relay_init state update to cache for %s", self._mac_in_str
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
plugwise_usb/nodes/circle.py(5 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
📚 Learning: 2025-08-13T17:29:32.348Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-14T12:26:12.780Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (2)
plugwise_usb/nodes/node.py (1)
save_cache(706-720)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)
🔇 Additional comments (3)
pyproject.toml (1)
7-7: Version bump looks fine.Pre-release suffix aligns with draft status; no issues spotted.
plugwise_usb/nodes/circle.py (2)
444-445: Good: explicit save after rollover.Guarantees persistence after dual-address rollover fetches; aligns cache with memory.
1105-1109: Remove redundant disk flush on unload. The base PlugwiseBaseNode.unload() already callssave_cache(trigger_only=False, full_write=True)to synchronously write the cache to disk, so adding anotherawait self.save_cache(trigger_only=False)is unnecessary.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
plugwise_usb/nodes/circle.py (4)
600-616: Bug: cache_updated is overwritten per slot; use OR to accumulateAs written, a later False can clobber an earlier True, skipping persistence when at least one slot updated. Accumulate instead.
Apply:
- cache_updated = False + cache_updated = False for _slot in range(4, 0, -1): log_timestamp, log_pulses = response.log_data[_slot] @@ - cache_updated = await self._energy_log_record_update_state( + slot_updated = await self._energy_log_record_update_state( response.log_address, _slot, log_timestamp.replace(tzinfo=UTC), log_pulses, import_only=True, ) + cache_updated |= slot_updated
623-629: Batch-flush in-memory cache once per address (avoid per-slot rebuild + races)Call _energy_log_records_save_to_cache() once after processing all 4 slots, then save to disk if requested. This reduces O(N) rebuilds per slot and avoids interleaved writers from concurrent tasks.
Apply:
- self._energy_counters.update() - if cache_updated and save_cache: + self._energy_counters.update() + if cache_updated and self._cache_enabled: + await self._energy_log_records_save_to_cache() + if cache_updated and save_cache: _LOGGER.debug( "Saving energy record update to cache for %s", self._mac_in_str ) await self.save_cache()
735-748: Avoid per-slot in-memory cache rebuilds inside _energy_log_record_update_stateEach new/changed slot triggers a full-string rebuild via _energy_log_records_save_to_cache(), which is O(N) and can interleave across concurrent tasks. Let the caller batch-flush once.
Apply:
if (cached_logs := self._get_cache(CACHE_ENERGY_COLLECTION)) is not None: entries = cached_logs.split("|") if cached_logs else [] if log_cache_record not in entries: _LOGGER.debug( "Adding logrecord (%s, %s) to cache of %s", str(address), str(slot), self._mac_in_str, ) - await self._energy_log_records_save_to_cache() return True
755-762: Same here: batch-flush instead of rebuilding per-slotRemove the immediate rebuild; let the caller handle it once.
Apply:
_LOGGER.debug( "Cache is empty, adding new logrecord (%s, %s) for %s", str(address), str(slot), self._mac_in_str, ) - await self._energy_log_records_save_to_cache() return True
🧹 Nitpick comments (5)
plugwise_usb/nodes/circle.py (5)
700-719: Docstring/log message are misleading: this function no longer writes to fileThe helper now only updates the in-memory cache string. Fix wording to prevent confusion.
Apply:
- async def _energy_log_records_save_to_cache(self) -> None: - """Save currently collected energy logs to cached file.""" + async def _energy_log_records_save_to_cache(self) -> None: + """Update the in-memory energy log cache string (no file I/O).""" @@ - _LOGGER.debug("Saving energy logrecords to cache for %s", self._mac_in_str) + _LOGGER.debug("Updating in-memory energy log records for %s", self._mac_in_str)
535-537: Initial collection: optionally flush in-memory before saving to diskIf you adopt the batch-flush pattern above, add an in-memory flush here prior to save_cache().
Apply:
- if self._cache_enabled: - await self.save_cache() + if self._cache_enabled: + await self._energy_log_records_save_to_cache() + await self.save_cache()
571-573: Missing-logs task: flush in-memory before disk save (align with new pattern)Same rationale as initial collection.
Apply:
- if self._cache_enabled: - await self.save_cache() + if self._cache_enabled: + await self._energy_log_records_save_to_cache() + await self.save_cache()
574-581: Docstring of energy_log_update no longer matches semanticsImplementation returns True for any successfully processed response (including empty/outdated slots). Update wording to match behavior and the prior learning.
Apply:
- ) -> bool: - """Request energy logs and return True only when at least one recent, non-empty record was stored; otherwise return False.""" + ) -> bool: + """Request energy logs and return True when logs were processed into memory; False only on request/parameter failure."""
717-717: Minor: fix log wording“logrecords” → “log records”.
Apply:
- _LOGGER.debug("Saving energy logrecords to cache for %s", self._mac_in_str) + _LOGGER.debug("Saving energy log records to cache for %s", self._mac_in_str)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(7 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
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-08-13T17:29:32.348Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-14T12:26:12.780Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (3)
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)
⏰ 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)
419-446: Rollover path: deferring disk writes is correctSwitching both energy_log_update calls to save_cache=False and issuing a single save_cache() afterward keeps disk I/O bounded. LGTM.
1108-1111: Incorrect: super().unload already persists cache to disk
PlugwiseBaseNode.unload checks _cache_enabled, then callsawait self.save_cache(trigger_only=False, full_write=True)which synchronously writes the Node’s cache to disk . Therefore no additional save is needed here.
Likely an incorrect or invalid review comment.
bouwew
force-pushed
the
energy_log_cache
branch
from
29a63d7 to
6b3d887
Compare
bouwew
force-pushed
the
energy_log_cache
branch
from
6b3d887 to
31b8f39
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
903-906: Clock drift check uses timedelta.seconds (bug) — use total_seconds()timedelta.seconds is modulo 24h and non-negative; the current condition is effectively always True. Compare abs(clock_offset.total_seconds()) against MAX_TIME_DRIFT.
- if (clock_offset.seconds < MAX_TIME_DRIFT) or ( - clock_offset.seconds > -(MAX_TIME_DRIFT) - ): + if abs(clock_offset.total_seconds()) < MAX_TIME_DRIFT: return True
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
711-714: Docstring nit: this writes to the in-memory cache, not the file_save_to_cache() updates the node’s cached state; disk persistence happens via save_cache().
- """Save currently collected energy logs to cached file.""" + """Serialize in-memory energy logs into the node's in-memory cache. + + Note: call save_cache() elsewhere to persist to disk. + """
526-533: Message/behavior mismatch: “stopping initial collection” log won’t triggerenergy_log_update() now returns False only on transport/address errors, not when all slots are empty/outdated. The “stopping initial collection” message won’t occur. Either:
- detect “all slots were empty/outdated” explicitly and break, or
- reword the message to reflect the current behavior.
Example (detect empties via a returned flag):
- result = await self.energy_log_update(log_address, save_cache=False) - if not result: + processed, had_recent = await self.energy_log_update(log_address, save_cache=False) + if not had_recent: _LOGGER.debug( "All slots at log address %s are empty or outdated – stopping initial collection", log_address, ) break(If changing the signature is undesirable, consider a lightweight helper on EnergyCounters to detect “had any recent for address/slot”.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
plugwise_usb/nodes/circle.py(7 hunks)pyproject.toml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
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-08-14T12:26:12.780Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-13T17:29:32.348Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (2)
plugwise_usb/nodes/node.py (1)
save_cache(706-720)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)
🔇 Additional comments (4)
plugwise_usb/nodes/circle.py (4)
421-423: Good: defer cache writes during rollover fetchesPassing save_cache=False here is the right move to avoid extra disk churn.
435-437: Rollover safety looks correctRetrying the previous address with calc_log_address(..., 1, -4) is consistent with the 4-slot page.
718-725: Newest-first serialization aligns with EnergyCounters orderUsing get_pulse_logs() order directly matches the “newest-first” guarantee (per learnings), so no extra sorting is needed.
1193-1194: Minor log text fix: LGTMThe relay_init cache message reads well now.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
plugwise_usb/nodes/circle.py (4)
446-448: Gate and flush before persisting; avoid unconditional disk I/OUnconditionally calling
save_cache()can write when nothing changed and also skips flushing the in-memory serialization first. Per maintainer guidance, persist only when cache content actually changed, and flush the in-memory cache string first.Apply:
- if self._cache_enabled: - await self.save_cache() + if self._cache_enabled: + prev = self._get_cache(CACHE_ENERGY_COLLECTION) or "" + await self._energy_log_records_save_to_cache() + if self._get_cache(CACHE_ENERGY_COLLECTION) != prev: + await self.save_cache()
537-539: Same here: only save if cache actually changed; flush firstMirror the rollover fix for initial collection completion.
- if self._cache_enabled: - await self.save_cache() + if self._cache_enabled: + prev = self._get_cache(CACHE_ENERGY_COLLECTION) or "" + await self._energy_log_records_save_to_cache() + if self._get_cache(CACHE_ENERGY_COLLECTION) != prev: + await self.save_cache()
573-575: And for missing-logs task completionPrevent no-op writes; serialize first and compare.
- if self._cache_enabled: - await self.save_cache() + if self._cache_enabled: + prev = self._get_cache(CACHE_ENERGY_COLLECTION) or "" + await self._energy_log_records_save_to_cache() + if self._get_cache(CACHE_ENERGY_COLLECTION) != prev: + await self.save_cache()
579-583: Handle empty/outdated data explicitly — update call-sites usingif not result:
The changed return semantics (True even for empty/duplicate logs, False only on transport/address errors) means theseif not result:checks no longer stop or retry on “no or outdated” data as intended. Update at least the following locations to distinguish real errors from empty/outdated slots (e.g., inspect returned records length or introduce a new status):
- plugwise_usb/nodes/circle.py:405 (initial startup)
- plugwise_usb/nodes/circle.py:423 (first collection attempt)
- plugwise_usb/nodes/circle.py:438 (rollover retry)
- plugwise_usb/nodes/circle.py:525 (initial loop stop)
🧹 Nitpick comments (4)
plugwise_usb/nodes/circle.py (4)
631-637: Nit: improve debug message wording“Saving and storing energy cache” is redundant. Consider “Persisting energy log updates for %s”.
- _LOGGER.debug( - "Saving and storing energy cache for %s", self._mac_in_str - ) + _LOGGER.debug("Persisting energy log updates for %s", self._mac_in_str)
726-726: Nit: consider logging record countOptionally include the number of serialized records to aid diagnostics (e.g., “Updating in-memory energy log records (N entries) …”).
1192-1192: Nit: unify log phrasingKeep wording consistent with other “Saving … to cache” messages (e.g., “Saving relay-init state update to cache for %s”).
446-448: Optional: factor persistence into a small helper to avoid repetitionIf you prefer DRY, introduce a helper and call it at the three sites.
Add:
async def _persist_energy_cache_if_changed(self) -> None: if not self._cache_enabled: return prev = self._get_cache(CACHE_ENERGY_COLLECTION) or "" await self._energy_log_records_save_to_cache() if self._get_cache(CACHE_ENERGY_COLLECTION) != prev: await self.save_cache()Then replace each site with:
await self._persist_energy_cache_if_changed()Also applies to: 537-539, 573-575
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(9 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.627Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
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-08-30T10:46:42.627Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.627Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-14T12:26:12.780Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-13T17:29:32.348Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (2)
plugwise_usb/nodes/node.py (1)
save_cache(706-720)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 (3)
plugwise_usb/nodes/circle.py (3)
419-437: Rollover path defers disk writes correctlyUsing
save_cache=Falseon both rollover fetches centralizes persistence to a single point. This aligns with the PR’s objective to control write locations.
621-629: Import-only path and update flag look goodProcessing only recent, non-empty records with
import_only=Trueand accumulatingcache_updatedis consistent with centralized persistence.
710-710: Good: clarify no file I/O hereExplicitly documenting this as in-memory-only helps reasoning about where disk writes happen.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
plugwise_usb/nodes/circle.py (2)
521-539: Inverted condition stops initial collection on data-present instead of empty/outdatedPer your docstring, slots_empty=False means at least one recent slot updated. The current check breaks when data exists; it should break when all slots are empty/outdated.
- if result and not slots_empty: - # (address with outdated data in slots is stored as with None data!) - # Stop initial log collection when an address contains no (None) or outdated data + if result and slots_empty: + # Stop initial log collection when an address contains no (None) or only outdated data _LOGGER.debug( "All slots at log address %s are empty or outdated – stopping initial collection", log_address, ) breakOptionally, avoid needless disk I/O by persisting only if any address had updates:
log_address = self._current_log_address + any_updates = False while total_addresses > 0: result, slots_empty = await self.energy_log_update( log_address, save_cache=False ) - if result and not slots_empty: + if result and slots_empty: ... break + any_updates |= (not slots_empty) log_address, _ = calc_log_address(log_address, 1, -4) total_addresses -= 1 - if self._cache_enabled: - await self.save_cache() + if self._cache_enabled and any_updates: + await self.save_cache()
563-576: Return-value regression: tasks await a tuple; cancellation never triggers and save is unconditionalenergy_log_update now returns (result, slots_empty). Using “result = await task” makes result a non-empty tuple, so “if not result” is always False. Also gate save_cache on actual updates.
- tasks = [ + tasks = [ create_task(self.energy_log_update(address, save_cache=False)) for address in missing_addresses ] - for idx, task in enumerate(tasks): - result = await task + any_updates = False + for idx, task in enumerate(tasks): + result, slots_empty = await task # When an energy log collection task returns False, stop and cancel the remaining tasks if not result: to_cancel = tasks[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 + any_updates |= (not slots_empty) - if self._cache_enabled: - await self.save_cache() + if self._cache_enabled and any_updates: + await self.save_cache()
🧹 Nitpick comments (5)
plugwise_usb/nodes/circle.py (5)
404-409: Fix typo in comment"acutally" -> "actually".
- # address is acutally reported by the node even when all slots at that address are empty. + # address is actually reported by the node even when all slots at that address are empty.
420-446: Avoid unnecessary cache writes after rollover; gate save on actual updatesOnly persist when at least one of the two updates stored recent data (i.e., slots_empty is False). This aligns with your “only save when there were updates” approach.
- # Try collecting energy-stats from _current_log_address - result, _ = await self.energy_log_update( + # Try collecting energy-stats from _current_log_address + result, slots_empty_cur = await self.energy_log_update( self._current_log_address, save_cache=False ) if not result: ... # Retry with previous log address as Circle node pointer to self._current_log_address # could be rolled over while the last log is at previous address prev_log_address, _ = calc_log_address(self._current_log_address, 1, -4) - result, _ = await self.energy_log_update(prev_log_address, save_cache=False) + result, slots_empty_prev = await self.energy_log_update(prev_log_address, save_cache=False) if not result: ... - if self._cache_enabled: - await self.save_cache() + if self._cache_enabled and (not slots_empty_cur or not slots_empty_prev): + await self.save_cache()
578-586: Clarify and fix typos in the new return-value docstringMinor grammar/typo fixes; explicitly mention “empty or outdated”.
- Return first bool as True if processing succeeded: records stored in memory, also with empty slots. - Return fist bool as False on transport or address errors. - Return second bool as False when all slots are empty otherwise as True + Return first bool as True if processing succeeded (records stored in memory, possibly all-empty). + Return first bool as False on transport or address errors. + Return second bool as False when all slots are empty or outdated; otherwise True.
612-633: Simplify cache_updated logic; remove sticky slot_updated stateslot_updated persists across iterations; directly set cache_updated=True when a recent slot is stored.
- slot_updated = 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: self._energy_counters.add_empty_log(response.log_address, _slot) elif self._check_timestamp_is_recent( response.log_address, _slot, log_timestamp.replace(tzinfo=UTC) ): self._energy_counters.add_pulse_log( response.log_address, _slot, log_timestamp.replace(tzinfo=UTC), log_pulses, import_only=True, ) - slot_updated = True - - cache_updated |= slot_updated + cache_updated = True
635-645: Tighten debug message wording“Saving and storing” is redundant.
- _LOGGER.debug( - "Saving and storing energy cache for %s", self._mac_in_str - ) + _LOGGER.debug("Saving energy cache for %s", self._mac_in_str)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(11 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.627Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
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-08-14T12:26:12.780Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-13T17:29:32.348Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-30T10:46:42.627Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.627Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (4)
plugwise_usb/nodes/node.py (1)
save_cache(706-720)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/nodes/helpers/counter.py (5)
timestamp(179-187)add_empty_log(70-72)add_pulse_log(74-89)update(147-176)update(286-328)
🔇 Additional comments (3)
plugwise_usb/nodes/circle.py (3)
458-461: LGTM: single missing address path persists via energy_log_updateCalling with save_cache=True delegates persistence correctly inside energy_log_update.
715-733: LGTM: in-memory-only serialization helperGood separation of concerns; no file I/O here and relies on get_pulse_logs() ordering.
1156-1156: LGTM: message clarity nit fixed“cache for” wording looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
582-645: Fix docstring: second bool name/semantics invertedCode returns slots_empty=True when all slots are empty/outdated and False when at least one recent record was stored. The docstring currently states the opposite. Correct it to prevent misuse at call sites.
Apply:
- ) -> tuple[bool, bool]: - """Request energy logs from node and store them. - - Return first bool as True if processing succeeded (records stored in memory, possibly all-empty). - Return first bool as False on transport or address errors. - Return second bool as False when all slots are empty or outdated; otherwise True. - """ + ) -> tuple[bool, bool]: + """Request energy logs from node and store them. + + First bool: True when processing succeeded (records stored in memory, possibly all-empty); + False only on transport or address errors. + Second bool: slots_empty — True when all four slots at the address are empty or outdated; + False when at least one recent, non-empty record was stored. + """Nit: you can also normalize the timestamp once to avoid repeated replace calls:
- elif self._check_timestamp_is_recent( - response.log_address, _slot, log_timestamp.replace(tzinfo=UTC) - ): + elif self._check_timestamp_is_recent( + response.log_address, _slot, log_timestamp.replace(tzinfo=UTC) + ): self._energy_counters.add_pulse_log( response.log_address, _slot, - log_timestamp.replace(tzinfo=UTC), + log_timestamp.replace(tzinfo=UTC), log_pulses, import_only=True, )
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
521-541: Initial collection: consider aborting on transport errorCurrent loop only breaks on “all slots empty/outdated”. If request fails (result is False), continuing likely won’t help and can spam the stick. Suggest breaking on error too.
Apply:
- if result and slots_empty: + if result and slots_empty: # Stop initial log collection when an address contains no (None) or outdated data # Outdated data can indicate a EnergyLog address rollover: from address 6014 to 0 _LOGGER.debug( "All slots at log address %s are empty or outdated – stopping initial collection", log_address, ) break + if not result: + _LOGGER.debug( + "Initial collection: request failed at log address %s – stopping", + log_address, + ) + break
564-580: Parallel fetches: optionally cap concurrencyLaunching a task per missing address can create bursts. If devices report many gaps, consider limiting concurrency (e.g., 4–8) with a semaphore or batched gathers to protect the link and reduce retries.
I can provide a minimal semaphore-based refactor if you want it in this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(13 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
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#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.627Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
📚 Learning: 2025-08-14T12:26:12.780Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-13T17:29:32.348Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-30T10:46:42.627Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.627Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (5)
plugwise_usb/nodes/node.py (1)
save_cache(706-720)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 (1)
log_address(770-772)plugwise_usb/nodes/helpers/counter.py (4)
add_empty_log(70-72)add_pulse_log(74-89)update(147-176)update(286-328)
🔇 Additional comments (7)
plugwise_usb/nodes/circle.py (7)
404-409: Startup: one-shot “last address” probe looks goodFetching once with save_cache=False is aligned with the new tuple return and avoids needless I/O. No further action.
420-446: Rollover handling is correct and I/O-minimal
- Probing both current and previous addresses prevents missing the tail record at rollover.
- Saving only when at least one address wasn’t empty matches the cache policy. Since energy_log_update() flushes the in-memory cache string on updates, an extra flush here isn’t needed.
459-461: Single-missing-address fast pathCalling energy_log_update(..., save_cache=True) directly here is the right tradeoff to persist immediately.
714-733: Cache serialization: newest-first and in-memory only looks right
- Uses get_pulse_logs() ordering (newest-first), matching prior guidance.
- No disk I/O here, as intended.
1078-1081: Unload: confirm persistence pathYou only refresh the in-memory cache string on unload; no save_cache() is called here. If PlugwiseBaseNode.unload() does not persist the node cache afterwards, the last in-memory snapshot could be lost. Please confirm super().unload() writes the cache; if not, consider saving here.
Would you like me to inspect PlugwiseBaseNode.unload() and wire a safe, non-redundant final save if needed?
1155-1158: Minor log string fix“cache for” wording corrected; looks good.
582-589: Call-site audit complete: energy_log_update unpacked correctly
All invocations unpack both returned values; no single-value assignments detected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
624-637: Consider explicit logging for empty slotsWhile the logic correctly adds empty logs for slots with
Nonetimestamp or pulses, it might be helpful to add debug logging when empty slots are detected for troubleshooting.if log_timestamp is None or log_pulses is None: self._energy_counters.add_empty_log(address, _slot) + _LOGGER.debug( + "Empty slot detected at address=%s, slot=%s", address, _slot + ) else:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)plugwise_usb/nodes/circle.py(13 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.627Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
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-08-14T12:26:12.780Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.780Z
Learning: In plugwise_usb/nodes/circle.py, the energy_log_update method's return value of True indicates successful processing and storing of energy log records in memory (via _energy_counters), regardless of whether the records were new, duplicates, or empty. The docstring "return True only when at least one recent, non-empty record was stored" refers to successful processing/storage in the energy counters system, not necessarily new cache entries.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-06-19T06:38:04.702Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-13T17:29:32.348Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.348Z
Learning: In plugwise_usb/nodes/circle.py, the method self._energy_counters.get_pulse_logs() already returns data in reverse-sorted order (newest-first), so no additional sorting is required when serializing energy log records to cache.
Applied to files:
plugwise_usb/nodes/circle.py
📚 Learning: 2025-08-30T10:46:42.627Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#329
File: plugwise_usb/nodes/circle.py:0-0
Timestamp: 2025-08-30T10:46:42.627Z
Learning: In plugwise_usb/nodes/circle.py, the save_cache() method should only be called when both cache_updated AND save_cache parameters are True. If no cache updates occurred (cache_updated = False), there's no meaningful data to persist to disk, even if the caller requested saving (save_cache = True). This prevents unnecessary disk I/O operations.
Applied to files:
plugwise_usb/nodes/circle.py
🧬 Code graph analysis (1)
plugwise_usb/nodes/circle.py (3)
plugwise_usb/nodes/node.py (1)
save_cache(706-720)plugwise_usb/nodes/helpers/pulses.py (2)
calc_log_address(20-36)add_empty_log(389-421)plugwise_usb/nodes/helpers/counter.py (4)
add_empty_log(70-72)add_pulse_log(74-89)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: Run pytest using Python 3.13
🔇 Additional comments (10)
plugwise_usb/nodes/circle.py (10)
404-409: LGTM! Energy log update now returns tuple with empty-slot indicationThe change to capture both the result and slots_empty state aligns well with the new return semantics of
energy_log_update.
446-447: Cache save condition correctly implements the requirementGood implementation! The cache is saved only when there's actual non-empty data from either the current or previous log address, which prevents unnecessary disk I/O for all-empty records.
Based on the retrieved learnings, this correctly follows the pattern where
save_cache()should only be called when there's meaningful data to persist.
461-463: Minor: Inconsistent destructuring patternFor consistency with other calls, consider using underscore for the unused
slots_emptyvalue.- result, _ = await self.energy_log_update( + result, _ = await self.energy_log_update(Actually, the code is already using underscore correctly. No change needed.
523-542: Well-structured initial energy log collectionThe logic correctly:
- Stops collection when all slots are empty (preventing unnecessary fetches)
- Tracks whether any non-empty data was found
- Only saves to cache when there's actual data
This aligns perfectly with the PR objectives of improving cache efficiency.
566-581: Excellent handling of concurrent task resultsThe implementation correctly:
- Unpacks the tuple results from each task
- Cancels remaining tasks on failure (good for efficiency)
- Properly drains cancellations to avoid "Task exception was never retrieved" warnings
- Tracks and saves cache only when there's non-empty data
583-648: Method signature and logic update aligns with PR objectivesThe updated
energy_log_updatemethod now:
- Returns a tuple
(result, slots_empty)providing clearer semantics- Properly tracks empty vs non-empty slots
- Only updates cache when there's actual data to save
The docstring clearly explains the return value semantics, which helps API consumers understand the difference between transport/address errors and successful processing of empty data.
718-736: Documentation update correctly reflects new behaviorThe updated comment clearly indicates this method now only updates the in-memory cache string without performing file I/O, which aligns with the centralized disk write approach via
save_cache().
1159-1159: Typo fix in log messageGood catch on fixing the typo from "cachefor" to "cache for".
1082-1084: Unload behavior correctly uses save_cache()The change from calling
_energy_log_records_save_to_cache()to just ensuring cache is enabled before unload aligns with the centralized persistence model. The parent class'sunload()method will handle the actualsave_cache()call.
420-444: energy_update correctly returns None for empty log fetches and all callers handle None as expected
|
dirixmjm
left a comment
dirixmjm
left a comment
There was a problem hiding this comment.
Great improvements, let's keep those caches manageable.
This PR implements my alternative idea to store the contents of
self.logsinto theenergy-collectioncache entry: a copy of the (limited to 24h) energy-log data in memory is stored on disk instead of a separate collection that keeps on growing because no pruning of older data was implemented.Also, an inconsistency (re-)introduced in
energy_log_update()was solved.Finally, various suggestions from the Rabbit were implemented.
Summary by CodeRabbit