Improve reading energy_logs from cache #314
Conversation
WalkthroughReworks energy-log cache handling in plugwise_usb/nodes/circle.py to a newest-first, pipe-delimited per-address/per-slot format with timestamped entries; adds _collect_records parser, prunes entries older than MAX_LOG_HOURS on load, prefixes new records on update, and bumps project version and CHANGELOG. Changes
Sequence Diagram(s)sequenceDiagram
participant Cache
participant CircleNode
participant EnergyCounter as Energy Counter
Note over CircleNode: Load cached energy logs
CircleNode->>Cache: read cached string
Cache-->>CircleNode: "addr:slot:YYYY-MM-DD-HH-MM-SS:pulses|..."
CircleNode->>CircleNode: _collect_records -> {addr: {slot: (ts,pulses), ...}}
CircleNode->>CircleNode: prune entries older than MAX_LOG_HOURS
loop newest-first per address/slot
CircleNode->>EnergyCounter: add_pulse_log(timestamp, pulses, import_only=True)
end
sequenceDiagram
participant CircleNode
participant Cache
Note over CircleNode: On new energy record
CircleNode->>CircleNode: build "addr:slot:YYYY-MM-DD-HH-MM-SS:pulses"
CircleNode->>Cache: read existing cache string
CircleNode->>CircleNode: if not present → prepend new record + "|" + existing
CircleNode->>Cache: save updated cache string via save_cache(trigger_only=True)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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 #314 +/- ##
==========================================
- Coverage 80.70% 80.58% -0.12%
==========================================
Files 36 36
Lines 7536 7547 +11
==========================================
Hits 6082 6082
- Misses 1454 1465 +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
🔭 Outside diff range comments (3)
plugwise_usb/nodes/circle.py (3)
619-640: Harden cache parsing to skip malformed entries instead of raising.As written, any malformed cache entry (bad int/format) will raise and abort loading. Guard parsing to keep loading resilient.
Apply this diff to defensively parse and skip bad lines:
- log_data = cache_data.split("|") - for log_record in log_data: - log_fields = log_record.split(":") - if len(log_fields) == 4: - timestamp_energy_log = log_fields[2].split("-") - if len(timestamp_energy_log) == 6: - address = int(log_fields[0]) - slot = int(log_fields[1]) - pulses = int(log_fields[3]) - timestamp = datetime( - year=int(timestamp_energy_log[0]), - month=int(timestamp_energy_log[1]), - day=int(timestamp_energy_log[2]), - hour=int(timestamp_energy_log[3]), - minute=int(timestamp_energy_log[4]), - second=int(timestamp_energy_log[5]), - tzinfo=UTC, - ) - if restored_logs.get(address) is None: - restored_logs[address] = {} - restored_logs[address][slot] = (timestamp, pulses) + log_data = cache_data.split("|") + for log_record in log_data: + rec = log_record.strip() + if not rec: + continue + log_fields = rec.split(":") + if len(log_fields) != 4: + continue + try: + address = int(log_fields[0]) + slot = int(log_fields[1]) + ts_parts = log_fields[2].split("-") + if len(ts_parts) != 6: + continue + timestamp = datetime( + year=int(ts_parts[0]), + month=int(ts_parts[1]), + day=int(ts_parts[2]), + hour=int(ts_parts[3]), + minute=int(ts_parts[4]), + second=int(ts_parts[5]), + tzinfo=UTC, + ) + pulses = int(log_fields[3]) + except (ValueError, IndexError) as exc: + _LOGGER.debug( + "Skipping malformed energy log cache entry %r (%s)", rec, exc + ) + continue + if restored_logs.get(address) is None: + restored_logs[address] = {} + restored_logs[address][slot] = (timestamp, pulses)
720-735: Fix duplicate detection to avoid substring collisions and trailing delimiter.Current check relies on substring presence, which can falsely treat “12:…” as present due to “112:…”. Also, concatenating with a trailing "|" when cache is empty produces a dangling empty entry on load.
Apply this diff to compare whole entries and avoid trailing pipe:
- if (cached_logs := self._get_cache(CACHE_ENERGY_COLLECTION)) is not None: - if log_cache_record not in cached_logs: - _LOGGER.debug( - "Adding logrecord (%s, %s) to cache of %s", - str(address), - str(slot), - self._mac_in_str, - ) - self._set_cache( - CACHE_ENERGY_COLLECTION, log_cache_record + "|" + cached_logs - ) - return True + 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, + ) + new_cache = ( + f"{log_cache_record}|{cached_logs}" + if cached_logs + else log_cache_record + ) + self._set_cache(CACHE_ENERGY_COLLECTION, new_cache) + return TrueConsider adding a unit test that adds entries for addresses 12 and 112 with identical slot/timestamp to verify both are cached.
Do you want me to add a focused test for this edge case?
692-701: Use list-and-join for performant log serializationThe
logsdict you get fromget_pulse_logs()is already built in newest-first order (seePulseCollection.logs), so you don’t need an extra sort—just switch to collecting entries in a list and thenjointhem to avoid O(n²) string concatenation:Consider replacing the block around lines 692–701 in
circle.pywith:- # build cached_logs via repeated += (O(n²)) - cached_logs = "" - for address, record in logs.items(): - for slot in record: - log = record[slot] - if cached_logs != "": - cached_logs += "|" - cached_logs += f"{address}:{slot}:{log.timestamp.year}" - cached_logs += f"-{log.timestamp.month}-{log.timestamp.day}" - cached_logs += f"-{log.timestamp.hour}-{log.timestamp.minute}" - cached_logs += f"-{log.timestamp.second}:{log.pulses}" + # Efficiently serialize newest-first (logs is already sorted) + records: list[str] = [] + for address, record in logs.items(): + for slot, log in record.items(): + records.append( + f"{address}:{slot}:{log.timestamp.year}" + f"-{log.timestamp.month}-{log.timestamp.day}" + f"-{log.timestamp.hour}-{log.timestamp.minute}" + f"-{log.timestamp.second}:{log.pulses}" + ) + cached_logs = "|".join(records)
- This eliminates the quadratic behavior of repeated
+=on strings.- Relies on
PulseCollection.logs’ reverse-sorting—no extrasorted(...)needed unless you want a defensive double-check.
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
641-662: Prefer MAX_LOG_HOURS for pruning horizon and minor readability.Given MAX_LOG_HOURS = DAY_IN_HOURS, use the semantic constant here for consistency with _check_timestamp_is_recent and helpers.pulses.
- skip_before = datetime.now(tz=UTC) - timedelta(hours=DAY_IN_HOURS) + skip_before = datetime.now(tz=UTC) - timedelta(hours=MAX_LOG_HOURS)
📜 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 (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/nodes/node.py (1)
_get_cache(652-656)plugwise_usb/nodes/helpers/counter.py (2)
timestamp(179-187)add_pulse_log(74-89)plugwise_usb/nodes/helpers/pulses.py (1)
logs(110-124)
⏰ 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: Build and publish Python 🐍 distributions 📦 to TestPyPI
🔇 Additional comments (1)
plugwise_usb/nodes/circle.py (1)
8-8: Import change looks correct.Adding timedelta is appropriate for pruning logic later in the file.
bouwew
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
612-645: Enhance energy log cache parsing resilienceThe current loader in
plugwise_usb/nodes/circle.py(_energy_log_records_load_from_cache, lines 612–645) will drop any record that doesn’t match the exact “Y-M-D-H-M-S” pattern and offers no diagnostics. To avoid data loss and aid debugging:
- Wrap all
int()and timestamp parsing intry/except- Accept both hyphen-only and space+colon timestamp formats
- Count & log skipped records once at the end
Proposed diff in
plugwise_usb/nodes/circle.py:- log_data = cache_data.split("|") - for log_record in log_data: - log_fields = log_record.split(":") - if len(log_fields) == 4: - timestamp_energy_log = log_fields[2].split("-") - if len(timestamp_energy_log) == 6: - address = int(log_fields[0]) - slot = int(log_fields[1]) - pulses = int(log_fields[3]) - timestamp = datetime( - year=int(timestamp_energy_log[0]), - month=int(timestamp_energy_log[1]), - day=int(timestamp_energy_log[2]), - hour=int(timestamp_energy_log[3]), - minute=int(timestamp_energy_log[4]), - second=int(timestamp_energy_log[5]), - tzinfo=UTC, - ) - if restored_logs.get(address) is None: - restored_logs[address] = {} - restored_logs[address][slot] = (timestamp, pulses) + log_data = cache_data.split("|") + invalid_records = 0 + for log_record in log_data: + if not log_record: + continue + fields = log_record.split(":") + if len(fields) != 4: + invalid_records += 1 + continue + try: + address = int(fields[0]) + slot = int(fields[1]) + pulses = int(fields[3]) + ts_str = fields[2] + timestamp = None + # Try new and legacy timestamp formats + for fmt in ("%Y-%m-%d-%H-%M-%S", "%Y-%m-%d %H:%M:%S"): + try: + timestamp = datetime.strptime(ts_str, fmt).replace(tzinfo=UTC) + break + except ValueError: + continue + # Fallback manual split for un-padded components + if timestamp is None: + parts = ts_str.split("-") + if len(parts) == 6: + timestamp = datetime( + year=int(parts[0]), + month=int(parts[1]), + day=int(parts[2]), + hour=int(parts[3]), + minute=int(parts[4]), + second=int(parts[5]), + tzinfo=UTC, + ) + if timestamp is None: + invalid_records += 1 + continue + restored_logs.setdefault(address, {})[slot] = (timestamp, pulses) + except (ValueError, TypeError): + invalid_records += 1 + continue + if invalid_records: + _LOGGER.debug( + "Skipped %d invalid energy_log_record entr%s for %s", + invalid_records, + "y" if invalid_records == 1 else "ies", + self._mac_in_str, + )Also consider adding a unit test that builds both the hyphen-only and space+colon cache strings to confirm parsing succeeds.
🧹 Nitpick comments (3)
plugwise_usb/nodes/circle.py (3)
535-537: Clearer contract for energy_log_update’s save_cache behaviorSignature and docstring align. Minor nit: consider renaming save_cache to persist_cache for clarity, but not required.
646-657: Pruning and sorting: correct but can be simplified and made saferLogic is right. Two suggestions:
- Use setdefault to reduce branching.
- Consider capping entries per address to 4 latest slots to avoid pathological caches.
Optional micro-simplification:
- sorted_logs: dict[int, dict[int, tuple[datetime, int]]] = {} + sorted_logs: dict[int, dict[int, tuple[datetime, int]]] = {} skip_before = datetime.now(tz=UTC) - timedelta(hours=MAX_LOG_HOURS) sorted_addresses = sorted(restored_logs.keys(), reverse=True) for address in sorted_addresses: sorted_slots = sorted(restored_logs[address].keys(), reverse=True) for slot in sorted_slots: if restored_logs[address][slot][0] > skip_before: - if sorted_logs.get(address) is None: - sorted_logs[address] = {} - sorted_logs[address][slot] = restored_logs[address][slot] + sorted_logs.setdefault(address, {})[slot] = restored_logs[address][slot]
729-743: Avoid O(n) split for membership test on every updateSplitting the entire cache string to check membership is O(n) per write. Two lightweight options:
- Exact-match check using sentinels avoids split: f"|{log_cache_record}|" in f"|{cached_logs}|".
- Maintain a small in-memory set of cache entries initialized from the cache at load time and update it on writes.
Minimal change within this hunk:
- entries = cached_logs.split("|") if cached_logs else [] - if log_cache_record not in entries: + if not cached_logs or f"|{log_cache_record}|" not in f"|{cached_logs}|": _LOGGER.debug( "Adding logrecord (%s, %s) to cache of %s", str(address), str(slot), self._mac_in_str, ) - new_cache = ( - f"{log_cache_record}|{cached_logs}" - if cached_logs - else log_cache_record - ) + new_cache = f"{log_cache_record}|{cached_logs}" if cached_logs else log_cache_record self._set_cache(CACHE_ENERGY_COLLECTION, new_cache) return TrueIf you prefer the set-based approach, I can wire a small helper to hydrate and maintain it alongside the string cache.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(9 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#255
File: plugwise_usb/nodes/circle.py:477-482
Timestamp: 2025-06-19T06:38:04.702Z
Learning: In plugwise_usb/nodes/circle.py, the timestamp comparison `prev_address_timestamp - self._last_collected_energy_timestamp` in the `get_missing_energy_logs` method is intentional. The code iterates backwards through time-ordered energy log addresses, where `prev_address_timestamp` contains the more recent timestamp from the previous iteration, and this subtraction correctly calculates the time gap to determine when to stop collecting outdated data.
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.
📚 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/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/node.py (4)
save_cache(705-719)_get_cache(652-656)name(218-222)_set_cache(689-703)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)plugwise_usb/nodes/helpers/pulses.py (1)
logs(110-124)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run pytest using Python 3.13
🔇 Additional comments (5)
plugwise_usb/nodes/circle.py (5)
8-8: LGTM: timedelta import is correct and usedThe additional timedelta import is appropriate for pruning cache entries by age.
384-386: LGTM: explicit save_cache=True is harmlessPassing save_cache=True explicitly is fine (default already True). No functional change.
420-422: LGTM: explicit save_cache=True is harmlessSame note as above — no functional change.
599-600: LGTM: robust age calculation in UTCClamps negative age to 0 and normalizes to UTC. This avoids false “outdated” positives.
658-667: LGTM: import-only rehydration into EnergyCountersUsing import_only=True prevents spurious updates. Ordering (newest-first addresses/slots) matches the intended cache semantics.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
731-734: Mixed timestamp formats cause duplicate cache entries_update_state writes non-zero-padded timestamps, while save_to_cache uses zero-padded strftime. Since dedup relies on string equality, the same record may be stored twice using different formats.
Unify to the zero-padded format used elsewhere.
Apply this diff:
- log_cache_record = f"{address}:{slot}:{timestamp.year}" - log_cache_record += f"-{timestamp.month}-{timestamp.day}" - log_cache_record += f"-{timestamp.hour}-{timestamp.minute}" - log_cache_record += f"-{timestamp.second}:{pulses}" + ts = timestamp.strftime("%Y-%m-%d-%H-%M-%S") + log_cache_record = f"{address}:{slot}:{ts}:{pulses}"Optionally, for large caches, consider using a set for membership checks to avoid O(n) scans, but that’s non-blocking given the bounded horizon.
Also applies to: 736-754
♻️ Duplicate comments (1)
plugwise_usb/nodes/circle.py (1)
702-714: Consistent, zero-padded serialization and immediate persistenceUsing strftime('%Y-%m-%d-%H-%M-%S') yields a stable format, and immediate save minimizes data loss risk. Relying on get_pulse_logs()’s newest-first order is correct per helpers/pulses.py.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)plugwise_usb/nodes/circle.py(9 hunks)pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🧰 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.324Z
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-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.324Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.324Z
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 (7)
plugwise_usb/api.py (2)
save_cache(368-371)name(288-289)plugwise_usb/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/node.py (4)
save_cache(705-719)_get_cache(652-656)name(218-222)_set_cache(689-703)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)plugwise_usb/nodes/helpers/counter.py (1)
timestamp(179-187)plugwise_usb/__init__.py (1)
name(143-145)plugwise_usb/nodes/helpers/pulses.py (1)
logs(110-124)
⏰ 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: Prepare
🔇 Additional comments (7)
pyproject.toml (1)
7-7: Version bump LGTMPre-release bump to 0.44.11a3 fits the cache refactor. No further config changes detected.
plugwise_usb/nodes/circle.py (6)
8-8: Necessary imports addedUTC, datetime, and timedelta are required for the new cache parsing/pruning logic.
383-386: Formatting-only changes in energy_updateCall-site formatting changes are fine and improve readability without functional impact.
Also applies to: 420-422
599-600: Age calculation is correct and timezone-safeUsing UTC consistently and clamping at 0 avoids negative ages due to clock skew.
653-664: Pruning and explicit reverse sorting are correctTime-bounding via skip_before and reverse ordering by address/slot align with the intended newest-first behavior.
665-673: Import-only rehydration path is soundFeeding add_pulse_log with import_only=True for each pruned, reverse-sorted record correctly repopulates in-memory counters.
535-537: Allenergy_log_updatecall sites include the newsave_cacheargumentI’ve verified every internal call in
plugwise_usb/nodes/circle.pyexplicitly passessave_cache(False or True). There are no remaining invocations relying on the old signature. No further updates are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
731-751: Unify cache record format to prevent duplicate entries_save_to_cache uses zero-padded strftime, while _energy_log_record_update_state builds non-padded timestamps. This causes the membership check to miss existing entries and prepend duplicates in a different format. Use the same strftime format here.
Apply this diff:
- log_cache_record = f"{address}:{slot}:{timestamp.year}" - log_cache_record += f"-{timestamp.month}-{timestamp.day}" - log_cache_record += f"-{timestamp.hour}-{timestamp.minute}" - log_cache_record += f"-{timestamp.second}:{pulses}" + log_cache_record = ( + f"{address}:{slot}:{timestamp.strftime('%Y-%m-%d-%H-%M-%S')}:{pulses}" + )Optionally, after setting a new cache value, you can trigger a deferred save for durability:
self._set_cache(CACHE_ENERGY_COLLECTION, new_cache) + await self.save_cache(trigger_only=True) return True
♻️ Duplicate comments (2)
plugwise_usb/nodes/circle.py (2)
628-648: Harden parsing: guard int conversions and fallback timestamp parsingint() conversions for address/slot/pulses and the fallback timestamp parts can raise ValueError/TypeError, aborting cache load on a single malformed entry. Wrap these in try/except and skip bad records.
Apply this diff:
- address = int(log_fields[0]) - slot = int(log_fields[1]) - pulses = int(log_fields[3]) - # Parse zero-padded timestamp, fallback to manual split - try: - timestamp = datetime.strptime( - log_fields[2], "%Y-%m-%d-%H-%M-%S" - ).replace(tzinfo=UTC) - except ValueError: - parts = log_fields[2].split("-") - if len(parts) != 6: - continue - timestamp = datetime( - year=int(parts[0]), - month=int(parts[1]), - day=int(parts[2]), - hour=int(parts[3]), - minute=int(parts[4]), - second=int(parts[5]), - tzinfo=UTC, - ) + # Parse numeric fields safely + try: + address = int(log_fields[0]) + slot = int(log_fields[1]) + pulses = int(log_fields[3]) + except (ValueError, TypeError): + # Skip malformed cache entry + continue + # Parse zero-padded timestamp, fallback to manual split + ts_str = log_fields[2] + try: + timestamp = datetime.strptime( + ts_str, "%Y-%m-%d-%H-%M-%S" + ).replace(tzinfo=UTC) + except ValueError: + parts = ts_str.split("-") + if len(parts) != 6: + continue + try: + timestamp = datetime( + year=int(parts[0]), + month=int(parts[1]), + day=int(parts[2]), + hour=int(parts[3]), + minute=int(parts[4]), + second=int(parts[5]), + tzinfo=UTC, + ) + except (ValueError, TypeError): + continue
649-651: Preserve newest-first when duplicates exist: don’t overwrite first occurrenceCache is serialized newest-first. On load, repeated address/slot entries will currently overwrite the earlier (newer) one, potentially restoring an older value. Keep the first occurrence per slot and skip subsequent duplicates.
Apply this diff:
- if restored_logs.get(address) is None: - restored_logs[address] = {} - restored_logs[address][slot] = (timestamp, pulses) + bucket = restored_logs.setdefault(address, {}) + # Keep first occurrence (newest-first cache guarantees newest appears first) + if slot not in bucket: + bucket[slot] = (timestamp, pulses)
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
619-623: Empty cache path behaviorReturning False on an empty cache is acceptable, but note this bubbles up as a "failed to load energy_log_records" in _load_from_cache. If you want empty cache to be considered a successful load (e.g., right after an energy-reset), consider returning True here instead.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
plugwise_usb/nodes/circle.py(9 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.324Z
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-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.324Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.324Z
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 (5)
plugwise_usb/api.py (2)
save_cache(368-371)name(288-289)plugwise_usb/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/node.py (4)
save_cache(705-719)_get_cache(652-656)name(218-222)_set_cache(689-703)plugwise_usb/nodes/helpers/counter.py (2)
timestamp(179-187)add_pulse_log(74-89)plugwise_usb/nodes/helpers/pulses.py (1)
logs(110-124)
⏰ 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
🔇 Additional comments (5)
plugwise_usb/nodes/circle.py (5)
598-600: Timestamp recency calculation looks goodThe normalization with tz-aware now() and max(0.0, …) avoids negative ages and is correct.
653-664: Sorted, time-bounded reconstruction is correctReverse-sorting addresses and slots and pruning older-than-MAX_LOG_HOURS before rehydrating via import_only add_pulse_log aligns with the objectives and prevents stale data from polluting counters.
665-673: Import-only repopulation path is appropriateUsing add_pulse_log(import_only=True) here avoids spurious updates and matches the intended cache-restore semantics.
702-714: Deterministic serialization and immediate persistence: good
- Using strftime('%Y-%m-%d-%H-%M-%S') ensures zero-padded, stable timestamps.
- Relying on get_pulse_logs()’s newest-first ordering keeps output deterministic without extra sorting (per prior discussion).
- Immediate persistence via save_cache(trigger_only=True) is appropriate for batching disk writes.
383-386: Formatting-only changesSignature and call-site formatting changes are neutral and fine.
Also applies to: 419-422, 535-537
this fits better to the reverse sorting of the data in the cache
bouwew
force-pushed
the
improve-energy-caching
branch
from
410b023 to
b6f4e81
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
593-624: Initialize and accumulate cache_updated to avoid UnboundLocalErrorcache_updated is used after the loop but only conditionally assigned inside it. If all slots are empty/outdated, the variable is undefined and will raise UnboundLocalError at Line 619.
# energy pulses collected during the previous hour of given timestamp - for _slot in range(4, 0, -1): + cache_updated = False + for _slot in range(4, 0, -1): @@ - 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 @@ - if cache_updated and save_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()
♻️ Duplicate comments (3)
plugwise_usb/nodes/circle.py (3)
90-93: Harden parsing: wrap int conversions to skip malformed entries instead of aborting loadA single malformed cache entry will raise ValueError and abort the entire cache load. Guard and continue.
- address = int(log_fields[0]) - slot = int(log_fields[1]) - pulses = int(log_fields[3]) + try: + address = int(log_fields[0]) + slot = int(log_fields[1]) + pulses = int(log_fields[3]) + except (TypeError, ValueError): + _LOGGER.debug("Skipping malformed energy log record: %r", log_fields) + continue
111-114: Preserve newest-first semantics on duplicate address/slot entriesWhen the cache contains multiple entries for the same address/slot, the later (older) one overwrites the earlier (newer) one. Keep the first occurrence per slot.
- if logs.get(address) is None: - logs[address] = {} - logs[address][slot] = (timestamp, pulses) + bucket = logs.setdefault(address, {}) + # Keep the first occurrence (cache is newest-first) + if slot not in bucket: + bucket[slot] = (timestamp, pulses)
731-750: Serialize cache mutations with an asyncio.Lock to prevent lost updates under concurrencyMultiple energy_log_update tasks can run concurrently and race on read-modify-write of the cache string, dropping entries. Serialize mutations with a lock; also persist on the “cache is empty” path for consistency.
Within this function:
- 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: + 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, ) - new_cache = ( - f"{log_cache_record}|{cached_logs}" - if cached_logs - else log_cache_record - ) - self._set_cache(CACHE_ENERGY_COLLECTION, new_cache) - await self.save_cache(trigger_only=True) - return True + new_cache = ( + f"{log_cache_record}|{cached_logs}" + if cached_logs + else log_cache_record + ) + async with self._cache_lock: + self._set_cache(CACHE_ENERGY_COLLECTION, new_cache) + await self.save_cache(trigger_only=True) + return True @@ - _LOGGER.debug( + _LOGGER.debug( "Cache is empty, adding new logrecord (%s, %s) for %s", str(address), str(slot), self._mac_in_str, ) - self._set_cache(CACHE_ENERGY_COLLECTION, log_cache_record) + async with self._cache_lock: + self._set_cache(CACHE_ENERGY_COLLECTION, log_cache_record) + await self.save_cache(trigger_only=True) return TrueAnd outside this function (required supporting changes):
- Add Lock to imports:
-from asyncio import Task, create_task, gather +from asyncio import Task, create_task, gather, Lock
- Initialize the lock in init:
self._last_energy_log_requested: bool = False + self._cache_lock: Lock = Lock()
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(9 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.324Z
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#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.760Z
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-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.324Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.324Z
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.760Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.760Z
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-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
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/circle.py (5)
plugwise_usb/nodes/helpers/pulses.py (2)
logs(110-124)collected_logs(100-107)plugwise_usb/messages/responses.py (1)
log_data(775-794)plugwise_usb/api.py (1)
save_cache(368-371)plugwise_usb/nodes/node.py (1)
save_cache(705-719)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)
🔇 Additional comments (2)
plugwise_usb/nodes/circle.py (2)
94-110: Timestamp parsing with strptime + fallback looks solidZero-padded, deterministic parsing via strptime with a graceful fallback to the legacy format is correct and robust.
702-715: Serialization format and immediate persistence: LGTM
- Correctly relies on get_pulse_logs() newest-first ordering (per prior learning), no redundant sorting.
- Uses zero-padded strftime for stable parsing.
- Immediately persists via save_cache(trigger_only=True).
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
768-769: Persist immediately when initializing cache to avoid lost updatesWhen the cache is empty, you set the first record but don’t save here, relying on the caller to flush. If the caller skips the outer flush (e.g., due to cache_updated tracking or future changes), this update may not hit disk promptly. Align with the non-empty path and persist immediately.
self._set_cache(CACHE_ENERGY_COLLECTION, log_cache_record) + await self.save_cache(trigger_only=True) return True
♻️ Duplicate comments (5)
plugwise_usb/nodes/circle.py (5)
111-114: Preserving newest-first on duplicates is correctKeeping the first occurrence per (address, slot) ensures the newest record survives when cache is newest-first. This addresses the earlier overwrite bug.
657-678: LGTM: cache loading now prunes old entries and guards missing slots
- Using _collect_records simplifies parsing.
- Guarding absent slots avoids KeyError.
- Pruning with skip_before is correct.
611-626: Accumulate cache_updated across slots or remove redundant flushcache_updated currently reflects only the last slot’s result, which is misleading and can skip the post-loop flush even if earlier slots updated the cache. Either accumulate across slots, or drop the post-loop flush since _energy_log_record_update_state() already persists.
Option A (accumulate and keep one post-loop flush):
@@ - for _slot in range(4, 0, -1): + cache_updated = False + for _slot in range(4, 0, -1): @@ - 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 @@ - if cache_updated and save_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()Option B (avoid double writes): remove the post-loop save entirely and rely on the inner save in _energy_log_record_update_state(). If you choose this, delete lines 620–624.
90-93: Harden parsing: guard int() conversions to skip malformed cache recordsA single bad address/slot/pulses value will raise ValueError and abort the entire load. Wrap these conversions and continue on failure.
Apply this diff:
- address = int(log_fields[0]) - slot = int(log_fields[1]) - pulses = int(log_fields[3]) + try: + address = int(log_fields[0]) + slot = int(log_fields[1]) + pulses = int(log_fields[3]) + except (TypeError, ValueError): + _LOGGER.debug( + "Skipping malformed energy log record: %r", log_record + ) + continue
739-755: Race condition on cache updates; serialize read-modify-write with an asyncio.LockMultiple energy_log_update tasks run concurrently (created in get_missing_energy_logs). This block reads, modifies, and writes the shared cache string without synchronization, risking lost updates.
Introduce an asyncio.Lock and wrap cache mutations.
Apply these diffs:
- Import Lock:
-from asyncio import Task, create_task, gather +from asyncio import Task, create_task, gather, Lock
- Initialize the lock in init:
self._energy_counters = EnergyCounters(mac) self._retrieve_energy_logs_task: None | Task[None] = None self._last_energy_log_requested: bool = False + self._cache_lock: Lock = Lock()
- Guard the mutation block:
- 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: + async with self._cache_lock: + cached_logs = self._get_cache(CACHE_ENERGY_COLLECTION) + if cached_logs 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, ) - new_cache = ( - f"{log_cache_record}|{cached_logs}" - if cached_logs - else log_cache_record - ) - self._set_cache(CACHE_ENERGY_COLLECTION, new_cache) - await self.save_cache(trigger_only=True) - return True + new_cache = ( + f"{log_cache_record}|{cached_logs}" + if cached_logs + else log_cache_record + ) + self._set_cache(CACHE_ENERGY_COLLECTION, new_cache) + await self.save_cache(trigger_only=True) + return True _LOGGER.debug( "Energy logrecord already present for %s, ignoring", self._mac_in_str ) return False
🧹 Nitpick comments (1)
plugwise_usb/nodes/circle.py (1)
633-636: Align cutoff semantics with loader (<= vs >) to avoid edge inconsistenciesLoader prunes entries when timestamp <= skip_before. Here, “outdated” is only when age_seconds > MAX_LOG_HOURS*3600, allowing equality at the boundary. Consider using >= for consistency; otherwise, a record exactly on the boundary is accepted live but pruned on reload.
- if age_seconds > MAX_LOG_HOURS * 3600: + if age_seconds >= MAX_LOG_HOURS * 3600:
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (2)
plugwise_usb/nodes/circle.py(9 hunks)pyproject.toml(1 hunks)
🚧 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:695-705
Timestamp: 2025-08-13T17:29:32.324Z
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#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.760Z
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-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.324Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.324Z
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.760Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.760Z
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-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
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/circle.py (6)
plugwise_usb/nodes/helpers/pulses.py (1)
logs(110-124)plugwise_usb/messages/responses.py (2)
log_data(775-794)log_address(770-772)plugwise_usb/api.py (1)
save_cache(368-371)plugwise_usb/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/node.py (1)
save_cache(705-719)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Ruff check and force
🔇 Additional comments (6)
plugwise_usb/nodes/circle.py (6)
8-8: LGTM: timedelta import is appropriateAdding timedelta is correct for cutoff computations.
95-110: Robust timestamp parsing with strptime + fallback looks goodZero-padded primary format with backward-compatible fallback is solid.
420-422: LGTM: explicit multi-line call improves readabilityNo behavioral change; clarity improved.
456-458: LGTM: explicit multi-line call for single missing addressReadability improved, behavior unchanged.
571-573: LGTM: signature reflow onlyNo functional change.
707-719: LGTM: standardized timestamp formatting and immediate persistence
- strftime('%Y-%m-%d-%H-%M-%S') ensures consistent zero-padded timestamps.
- Relying on get_pulse_logs() ordering is fine given it already returns newest-first (per learnings).
- Immediate save with trigger_only is appropriate.
dirixmjm
left a comment
dirixmjm
left a comment
There was a problem hiding this comment.
Nice work! This will reduce a lot of start-up problems.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
plugwise_usb/nodes/circle.py (1)
597-626: cache_updated only reflects the last slot; accumulate across slotsCurrently, only the final slot’s result controls the post-loop save. If earlier slots updated the cache but the last did not, you’ll skip the save here. Accumulate via OR to reflect any update made in the loop.
Apply this diff:
- cache_updated = False + cache_updated = False @@ - cache_updated = await self._energy_log_record_update_state( + cache_updated = cache_updated or await self._energy_log_record_update_state( response.log_address, _slot, log_timestamp.replace(tzinfo=UTC), log_pulses, import_only=True, ) @@ - if cache_updated and save_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()Note: _energy_log_record_update_state already persists on update (trigger_only), so this remains an extra flush. Keeping it is okay; accumulating correctness is the key fix.
♻️ Duplicate comments (3)
plugwise_usb/nodes/circle.py (3)
705-717: Cache serialization: stable format and immediate persistence
- Using get_pulse_logs() preserves newest-first ordering (as that method already returns reverse-sorted data).
- strftime('%Y-%m-%d-%H-%M-%S') ensures zero-padded timestamps.
- Immediate save (trigger_only) is a good safeguard.
LGTM.
734-767: Serialize cache mutations to avoid lost updates; persist on empty-cache pathMultiple concurrent energy_log_update tasks can interleave the read-modify-write of the shared cache string, causing lost updates. Also, when the cache is currently empty, a new entry is set but not persisted immediately.
Wrap cache mutation in an asyncio.Lock and persist on the empty-cache path.
Within this method, 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: + async with self._cache_lock: + cached_logs = self._get_cache(CACHE_ENERGY_COLLECTION) + if cached_logs 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, ) - new_cache = ( - f"{log_cache_record}|{cached_logs}" - if cached_logs - else log_cache_record - ) - self._set_cache(CACHE_ENERGY_COLLECTION, new_cache) - await self.save_cache(trigger_only=True) - return True - _LOGGER.debug( - "Energy logrecord already present for %s, ignoring", self._mac_in_str - ) - return False + new_cache = ( + f"{log_cache_record}|{cached_logs}" + if cached_logs + else log_cache_record + ) + self._set_cache(CACHE_ENERGY_COLLECTION, new_cache) + await self.save_cache(trigger_only=True) + return True + _LOGGER.debug( + "Energy logrecord already present for %s, ignoring", + self._mac_in_str, + ) + return False @@ - _LOGGER.debug( - "Cache is empty, adding new logrecord (%s, %s) for %s", - str(address), - str(slot), - self._mac_in_str, - ) - self._set_cache(CACHE_ENERGY_COLLECTION, log_cache_record) - return True + async with self._cache_lock: + _LOGGER.debug( + "Cache is empty, adding new logrecord (%s, %s) for %s", + str(address), + str(slot), + self._mac_in_str, + ) + self._set_cache(CACHE_ENERGY_COLLECTION, log_cache_record) + await self.save_cache(trigger_only=True) + return TrueAdd the lock to the class:
# at top of file from asyncio import Task, create_task, gather, Lock # in PlugwiseCircle.__init__ self._cache_lock: Lock = Lock()
83-116: Harden cache parser: guard integer conversions and skip malformed entriesA single malformed cache entry (non-integer address/slot/pulses) will raise and abort the entire load. Wrap int() conversions in try/except and continue on error.
Apply this diff within this function:
- if len(log_fields) == 4: - address = int(log_fields[0]) - slot = int(log_fields[1]) - pulses = int(log_fields[3]) + if len(log_fields) == 4: + try: + address = int(log_fields[0]) + slot = int(log_fields[1]) + pulses = int(log_fields[3]) + except (ValueError, TypeError): + _LOGGER.debug("Skipping malformed energy log record: %r", log_fields) + continue
🧹 Nitpick comments (2)
plugwise_usb/nodes/circle.py (2)
574-574: Docstring intent vs. behavior: consider aligning the textThe function returns True for successful processing (including empty/duplicate records), which is intentional per prior clarification. Consider updating the docstring to prevent confusion.
Apply this diff:
- """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 when records were processed and stored in memory (may include duplicates or empty slots). Return False only on request/processing failure."""
83-116: Optional: add minimal unit tests for _collect_recordsConsider small tests covering:
- Proper parsing of both timestamp formats
- Skipping malformed entries
- Duplicate address/slot entries preserving the newest-first occurrence
I can draft parameterized tests for these cases if helpful.
📜 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 settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
plugwise_usb/nodes/circle.py(10 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 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.324Z
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#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.760Z
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-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.324Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:695-705
Timestamp: 2025-08-13T17:29:32.324Z
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.760Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#314
File: plugwise_usb/nodes/circle.py:570-573
Timestamp: 2025-08-14T12:26:12.760Z
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-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
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/circle.py (7)
plugwise_usb/nodes/helpers/pulses.py (2)
logs(110-124)collected_logs(100-107)plugwise_usb/messages/responses.py (2)
log_data(775-794)log_address(770-772)plugwise_usb/api.py (1)
save_cache(368-371)plugwise_usb/network/cache.py (1)
save_cache(27-34)plugwise_usb/nodes/node.py (1)
save_cache(705-719)plugwise_usb/nodes/helpers/cache.py (1)
save_cache(40-46)plugwise_usb/messages/requests.py (1)
response(124-128)
⏰ 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
🔇 Additional comments (9)
plugwise_usb/nodes/circle.py (9)
8-8: Importing timedelta is correct and necessaryRequired for MAX_LOG_HOURS cutoff. No issues.
95-110: Robust, zero-padded timestamp parsing with fallback looks goodUsing strptime('%Y-%m-%d-%H-%M-%S') ensures stable formatting, with a compatible fallback for older cache entries.
111-115: Correct duplicate handling (keep first/newest) in loaderPreserving the first occurrence per slot aligns with newest-first cache semantics and prevents older records from overriding newer ones.
419-422: Rollover retry behavior preservedRe-requesting the current address after rollover and saving cache explicitly is sensible.
456-458: Single missing-address retrieval path is clearExplicitly saving to cache on this path is appropriate.
634-645: Timezone normalization and log level change are appropriateReplacing tzinfo=UTC before age calculation is correct. Downgrading to info for outdated logs reduces noise.
658-676: Loader: prune by MAX_LOG_HOURS and iterate newest-first
- Using _collect_records centralizes parsing and handles duplicates.
- Filtering with skip_before and iterating in reverse-sorted address/slot order matches the cache’s newest-first format.
Looks good.
647-695: Cache-load control flow aligns with initialization semanticsReturning False when CACHE_ENERGY_COLLECTION is missing/empty or when log_addresses_missing is None is consistent with the initialization flow.
702-713: Newest-first, per-address/slot serialization matches the new cache formatClear, pipe-delimited records and consistent formatting will simplify forward/backward compatibility.
Summary by CodeRabbit
Bug Fixes
Documentation
Chores