Skip using _energy_log_records_save_to_cache() #305
Conversation
WalkthroughThe update modifies the energy log caching workflow in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (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 comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #305 +/- ##
==========================================
+ Coverage 80.92% 81.07% +0.14%
==========================================
Files 36 36
Lines 7593 7578 -15
==========================================
- Hits 6145 6144 -1
+ Misses 1448 1434 -14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
557-562: Same persistence pattern; reuse helperSame pattern as earlier. Reuse the helper for consistency.
- _LOGGER.debug( - "Saving missing energy_logs to cache for %s", self._mac_in_str - ) - await self._energy_log_records_save_to_cache() - await self.save_cache() + await self._persist_energy_logs_cache("missing")
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
405-411: Persist after rollover is good; tweak log wordingThe persistence here is correct. The debug message says “Saving initial energy_logs…”, but this is a rollover path, not strictly “initial”. Consider clarifying the message to avoid confusion.
- _LOGGER.debug( - "Saving initial energy_logs to cache for %s", self._mac_in_str - ) + _LOGGER.debug( + "Saving energy_logs to cache after rollover for %s", self._mac_in_str + )
525-530: Good: persist after initial energy log collection; consider DRY helperPersisting after initial collection is aligned with the new strategy. This same 3-line pattern appears in multiple places; a tiny helper improves consistency and reduces duplication.
Apply within this block:
- _LOGGER.debug( - "Saving initial energy_logs to cache for %s", self._mac_in_str - ) - await self._energy_log_records_save_to_cache() - await self.save_cache() + await self._persist_energy_logs_cache("initial")Add this helper (outside the selected range, e.g., near other private helpers):
async def _persist_energy_logs_cache(self, context: str) -> None: if not self._cache_enabled: return _LOGGER.debug( "Saving %s energy_logs to cache for %s", context, self._mac_in_str ) await self._energy_log_records_save_to_cache() await self.save_cache()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#252
File: plugwise_usb/__init__.py:0-0
Timestamp: 2025-06-03T09:58:44.611Z
Learning: In the plugwise_usb codebase, bouwew prefers not to add excessive validation decorators or protective error handling when they would amount to "too much protection". The existing validation level is considered sufficient.
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-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
📚 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
⏰ 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 (1)
plugwise_usb/nodes/circle.py (1)
1045-1046: LGTM: flush cache on unloadExplicitly persisting cache on unload is appropriate, complements the new bulk-save strategy.
| else: | ||
| self._energy_counters.add_pulse_log( | ||
| response.log_address, | ||
| _slot, | ||
| log_timestamp.replace(tzinfo=UTC), | ||
| log_pulses, | ||
| import_only=True, | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Regression risk: removing incremental cache save leaves single-missing-address path unsaved
Now that cache persistence is centralized outside energy_log_update, the energy_update branch where exactly one missing address is handled returns early (after calling energy_log_update) without scheduling the background task and without persisting. If the process restarts before another persistence point (rollover, bulk retrieval, unload), those newly imported logs won’t be on disk.
Fix: persist before returning in the single-missing-address branch of energy_update.
Outside the selected range, adjust around Lines 423-433:
if len(missing_addresses) == 1:
result, _ = await self.energy_log_update(missing_addresses[0])
if result:
+ if self._cache_enabled:
+ # Persist immediately when resolving a single missing address
+ await self._energy_log_records_save_to_cache()
+ await self.save_cache()
await self.power_update()
_LOGGER.debug(
"async_energy_update for %s | single energy log is missing | %s",
self._mac_in_str,
missing_addresses,
)
return self._energy_counters.energy_statisticsVerification script: ensure energy_log_update isn’t called from other places that now need explicit persistence.
🏁 Script executed:
#!/bin/bash
# Find all call sites of energy_log_update and show brief context
rg -n -A 3 -B 3 $'energy_log_update\\('
# Optional: ensure single-missing-address branch exists as expected
rg -n -A 8 -B 8 $'len\\(missing_addresses\\) == 1' plugwise_usb/nodes/circle.pyLength of output: 4690
Add immediate cache persistence for single‐missing‐address branch
In the async_energy_update method, when exactly one missing address is handled, we call energy_log_update and then return without persisting the cache. If the process exits before the next bulk‐save (rollover, multi‐fetch, or unload), those newly imported logs will be lost.
• File: plugwise_usb/nodes/circle.py
• Location: around line 423 in async_energy_update
Suggested diff:
if len(missing_addresses) == 1:
- result, _ = await self.energy_log_update(missing_addresses[0])
- if result:
- await self.power_update()
- _LOGGER.debug(
- "async_energy_update for %s | single energy log is missing | %s",
- self._mac_in_str,
- missing_addresses,
- )
- return self._energy_counters.energy_statistics
+ result, _ = await self.energy_log_update(missing_addresses[0])
+ if result:
+ # Persist immediately when resolving a single missing address
+ if self._cache_enabled:
+ await self._energy_log_records_save_to_cache()
+ await self.save_cache()
+ await self.power_update()
+ _LOGGER.debug(
+ "async_energy_update for %s | single energy log is missing | %s",
+ self._mac_in_str,
+ missing_addresses,
+ )
+ return self._energy_counters.energy_statisticsThis ensures that as soon as a solitary missing log is fetched, it’s written to disk.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In plugwise_usb/nodes/circle.py around line 423 in the async_energy_update
method, after calling energy_log_update for the single missing address case, add
code to immediately persist the cache to disk. This prevents loss of newly
imported logs if the process exits before the next bulk save. Locate the branch
handling exactly one missing address, call the cache persistence method right
after energy_log_update, and before returning.
use _energy_log_records_save_to_cache() instead
bouwew
force-pushed
the
energy_log_cache
branch
from
6c52922 to
8053b46
Compare
|
|
This idea will not be pursued. |
use _energy_log_records_save_to_cache() instead
Summary by CodeRabbit
Bug Fixes
Refactor