Skip to content

Conversation

@dirixmjm
Copy link
Contributor

@dirixmjm dirixmjm commented Aug 13, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Publish motion and battery configurations to subscribers during load and after wake tasks for improved state sync.
    • Prevent redundant updates when new values match current settings, reducing unnecessary device operations.
    • Adjust loading sequence so devices signal readiness before performing follow-up initialization.
  • Tests

    • Updated SED configuration test expectation for awake duration.
  • Documentation

    • Changelog entry updated to reflect configuration propagation and initialization behavior.

@coderabbitai
Copy link

coderabbitai bot commented Aug 13, 2025

Walkthrough

LOADED now fires before initialization in the scan node, and MOTION_CONFIG is always published after awake tasks. SED battery-setting setters short-circuit no-ops, mark in-memory BatteryConfig dirty on real changes and await propagation. Tests updated to expect awake_duration 20 instead of 15.

Changes

Cohort / File(s) Summary
Scan node load & awake tasks
plugwise_usb/nodes/scan.py
Moved initialization to occur after the LOADED callback; added unconditional publish of current MOTION_CONFIG to subscribers after awake-tasks.
SED node battery setters & load behavior
plugwise_usb/nodes/sed.py
Battery-related setters (awake/sleep/clock/maintenance) now return early on no-op values, update in-memory BatteryConfig with dirty=True on real changes, and await _sed_configure_update() to apply to device/cache and notify subscribers; load publishes battery when cache is clean.
Tests
tests/test_usb.py
Updated SED node test to call set_awake_duration(20) and assert awake_duration == 20 (was 15).
Changelog
CHANGELOG.md
Replaced an Ongoing entry (PR 319) with PR 312 entry describing config propagation and initialization-on-first-wakeup changes.

Sequence Diagram(s)

sequenceDiagram
  participant Cache as Cache
  participant Scan as Scan Node
  participant Subs as Subscribers

  Cache->>Scan: _load_from_cache()
  Scan->>Scan: call LOADED callback
  Scan->>Scan: perform initialization (moved after LOADED)
  Scan->>Scan: _run_awake_tasks()
  Scan->>Subs: publish NodeFeature.MOTION_CONFIG (current motion_config)
Loading
sequenceDiagram
  participant Cache as Cache
  participant SED as SED Node
  participant Device as Device
  participant Subs as Subscribers

  Cache->>SED: _load_from_cache()
  alt battery config dirty
    SED->>SED: _sed_configure_update() (apply to Device, update cache, publish)
  else battery config clean
    SED->>Subs: publish NodeFeature.BATTERY (battery_config)
  end

  Note over SED: Setter flow (example: set_awake_duration)
  SED->>SED: if new == current -> return False
  SED->>SED: update in-memory BatteryConfig (dirty=True)
  SED->>SED: await _sed_configure_update()
  SED->>Device: apply config
  SED->>Subs: publish NodeFeature.BATTERY (updated config)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • bouwew
  • ArnoutD

Poem

"I twitch my whiskers, tap the line,
LOADED leaps ahead of time.
When configs change I loudly thump,
no-ops skipped — no needless bump.
Twenty hops — a rabbit's chime! 🐇"

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.


📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 1345e0c and dcb8909.

📒 Files selected for processing (1)
  • CHANGELOG.md (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • CHANGELOG.md
⏰ 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
✨ Finishing Touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch mdi_sed

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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@codecov
Copy link

codecov bot commented Aug 13, 2025

Codecov Report

❌ Patch coverage is 91.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 80.59%. Comparing base (c9f5948) to head (389f9b4).
⚠️ Report is 21 commits behind head on main.

Files with missing lines Patch % Lines
plugwise_usb/nodes/sed.py 85.71% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #312   +/-   ##
=======================================
  Coverage   80.58%   80.59%           
=======================================
  Files          36       36           
  Lines        7547     7555    +8     
=======================================
+ Hits         6082     6089    +7     
- Misses       1465     1466    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
plugwise_usb/nodes/sed.py (1)

233-253: Avoid redundant writes in set_awake_duration by short-circuiting unchanged values

Unlike the other setters, set_awake_duration lacks an equality check. Now that it immediately persists and publishes, calls with identical values will trigger unnecessary cache writes and notifications.

Apply this diff to align with the other setters and reduce redundant work:

 async def set_awake_duration(self, seconds: int) -> bool:
@@
     if seconds < 1 or seconds > MAX_UINT_2:
         raise ValueError(
             f"Invalid awake duration ({seconds}). It must be between 1 and 255 seconds."
         )
+    if self._battery_config.awake_duration == seconds:
+        return False
@@
     self._battery_config = replace(
         self._battery_config,
         awake_duration=seconds,
         dirty=True,
     )
     await self._sed_configure_update()
     return True
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 757787c and f7d4138.

📒 Files selected for processing (2)
  • plugwise_usb/nodes/scan.py (1 hunks)
  • plugwise_usb/nodes/sed.py (6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T17:50:29.188Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.188Z
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/sed.py
  • plugwise_usb/nodes/scan.py
🧬 Code Graph Analysis (2)
plugwise_usb/nodes/sed.py (2)
plugwise_usb/nodes/helpers/subscription.py (1)
  • publish_feature_update_to_subscribers (51-66)
plugwise_usb/api.py (1)
  • NodeFeature (39-58)
plugwise_usb/nodes/scan.py (2)
plugwise_usb/nodes/helpers/subscription.py (1)
  • publish_feature_update_to_subscribers (51-66)
plugwise_usb/api.py (1)
  • NodeFeature (39-58)
🔇 Additional comments (3)
plugwise_usb/nodes/scan.py (1)

173-177: Publish MOTION_CONFIG on clean cache loads — good propagation

This ensures subscribers receive the current motion configuration even when the cache is clean, aligning behavior with SED battery config publishing. Nice consistency and avoids stale UI state after cold starts.

plugwise_usb/nodes/sed.py (2)

178-182: Publish BATTERY on clean cache loads — good consistency

Publishing the current BatteryConfig when the cache isn’t dirty aligns with the Scan node behavior and prevents subscribers from missing initial state on cold start.


251-251: Immediate cache + subscriber updates in setters — aligns with objectives

Awaiting _sed_configure_update() right after mutating BatteryConfig makes updates propagate immediately (optimistic with dirty=True) while device application remains deferred to awake. This is the right trade-off for responsiveness.

Also applies to: 273-273, 290-290, 312-312, 337-337

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
plugwise_usb/nodes/sed.py (1)

253-253: Optionally coalesce multiple setter calls to reduce duplicate save/publish churn.

If clients may call several setters in quick succession (e.g., UI workflows), each call runs save_cache() and publishes. Consider a simple batching mechanism (context manager or a small queue/debounce) to coalesce adjacent updates and publish once.

Example approach:

  • Add a short debounce (e.g., 20–50 ms) when scheduling _sed_configure_update().
  • Or provide an apply_battery_config(...) that sets multiple fields then calls _sed_configure_update() once.

This is an optimization; current behavior is correct.

Also applies to: 275-275, 292-292, 314-314, 339-339

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f7d4138 and 4817673.

📒 Files selected for processing (1)
  • plugwise_usb/nodes/sed.py (6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T17:50:29.188Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.188Z
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/sed.py
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/sed.py (3)
plugwise_usb/nodes/helpers/subscription.py (1)
  • publish_feature_update_to_subscribers (51-66)
plugwise_usb/api.py (2)
  • NodeFeature (39-58)
  • battery_config (484-495)
plugwise_usb/nodes/node.py (1)
  • battery_config (121-127)
⏰ 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/sed.py (2)

178-182: Publishing clean cached battery config on load is correct and consistent with scan-node changes.

Good call to publish the current BatteryConfig to subscribers when the cache is clean. This ensures observers always receive an initial state after load without forcing a device reconfigure.


253-253: Awaiting _sed_configure_update() in setters ensures immediate propagation to cache and subscribers.

This is the right move to make updates observable immediately while keeping the device reconfiguration deferred to the next awake cycle. Nicely aligns with the PR objective.

Also applies to: 275-275, 292-292, 314-314, 339-339

@dirixmjm dirixmjm force-pushed the mdi_sed branch 2 times, most recently from 1dac925 to bd052d6 Compare August 13, 2025 12:32
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
tests/test_usb.py (1)

2000-2011: Clarify intent and add a no-op assertion for completeness.

Setting 20 right before handling the “accepted” awake response while still asserting a final value of 15 can be confusing to future readers. Add a brief comment to explain the rationale and also assert the no-op path explicitly after the config becomes clean.

Apply this diff:

@@
         awake_response2.timestamp = awake_response1.timestamp + td(
             seconds=pw_sed.AWAKE_RETRY
         )
-        assert await test_sed.set_awake_duration(20)
+        # Intentionally use a value different from the currently staged 15 to
+        # avoid the setter's early-exit (which would return False) and validate it returns True.
+        # The accepted configuration below should still reflect the previously staged 15.
+        assert await test_sed.set_awake_duration(20)
         assert test_sed.battery_config.dirty
         mock_stick_controller.send_response = sed_config_accepted
         await test_sed._awake_response(awake_response2)  # pylint: disable=protected-access
         await asyncio.sleep(0.001)  # Ensure time for task to be executed
         assert not test_sed.battery_config.dirty
         assert test_sed.battery_config.awake_duration == 15
         assert test_sed.awake_duration == 15
+        # Verify no-op behavior explicitly for awake duration as well.
+        assert not await test_sed.set_awake_duration(15)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e1d9e38 and 1dac925.

📒 Files selected for processing (1)
  • tests/test_usb.py (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/test_usb.py (3)
plugwise_usb/api.py (1)
  • set_awake_duration (526-549)
plugwise_usb/nodes/sed.py (1)
  • set_awake_duration (233-254)
plugwise_usb/nodes/node.py (1)
  • set_awake_duration (736-742)
🔇 Additional comments (1)
tests/test_usb.py (1)

2003-2003: Good change to avoid setter early-exit no-op.

Switching to set_awake_duration(20) is correct given the new early-exit in NodeSED.set_awake_duration (returns False when the value is unchanged). Using 15 here would make the assertion fail.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
.github/workflows/codeql-analysis.yml (1)

39-39: Consider using the latest version v5 instead of the specific patch version.

While pinning to v5.0.0 provides version stability, using v5 (without the patch version) is generally preferred in GitHub workflows. This allows you to automatically receive security patches and bug fixes while maintaining compatibility with the major version.

-        uses: actions/checkout@v5.0.0
+        uses: actions/checkout@v5
plugwise_usb/nodes/circle.py (1)

719-768: Consider optimizing the cache update logic to reduce redundant string operations.

The current implementation splits and rejoins the entire cache string for each update. For large cache strings, this could be inefficient. Consider maintaining a set or dict structure for faster lookups and updates.

While the current implementation works correctly, you could optimize it by maintaining a more efficient data structure:

async def _energy_log_record_update_state(
    self,
    address: int,
    slot: int,
    timestamp: datetime,
    pulses: int,
    import_only: bool = False,
) -> bool:
    """Process new energy log record. Returns true if record is new or changed."""
    self._energy_counters.add_pulse_log(
        address, slot, timestamp, pulses, import_only=import_only
    )
    if not self._cache_enabled:
        return False

    log_cache_record = (
        f"{address}:{slot}:{timestamp.strftime('%Y-%m-%d-%H-%M-%S')}:{pulses}"
    )
    
    # Consider caching the parsed entries in a class variable to avoid repeated parsing
    if (cached_logs := self._get_cache(CACHE_ENERGY_COLLECTION)) is not None:
        # Only parse if we haven't already
        entries = set(cached_logs.split("|")) if cached_logs else set()
        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

    _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

However, this is a minor optimization and the current implementation is functionally correct.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 1dac925 and 05852e7.

📒 Files selected for processing (8)
  • .github/workflows/codeql-analysis.yml (1 hunks)
  • .github/workflows/merge.yml (1 hunks)
  • .github/workflows/verify.yml (13 hunks)
  • .pre-commit-config.yaml (1 hunks)
  • CHANGELOG.md (1 hunks)
  • plugwise_usb/nodes/circle.py (11 hunks)
  • pyproject.toml (1 hunks)
  • tests/test_usb.py (1 hunks)
✅ Files skipped from review due to trivial changes (3)
  • CHANGELOG.md
  • pyproject.toml
  • .pre-commit-config.yaml
🧰 Additional context used
🧠 Learnings (3)
📚 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
🧬 Code Graph Analysis (2)
plugwise_usb/nodes/circle.py (6)
plugwise_usb/nodes/helpers/pulses.py (2)
  • logs (110-124)
  • calc_log_address (20-36)
plugwise_usb/messages/responses.py (2)
  • log_data (775-794)
  • log_address (770-772)
plugwise_usb/nodes/helpers/counter.py (4)
  • timestamp (179-187)
  • update (147-176)
  • update (286-328)
  • add_pulse_log (74-89)
plugwise_usb/api.py (1)
  • save_cache (368-371)
plugwise_usb/nodes/node.py (1)
  • save_cache (705-719)
plugwise_usb/messages/requests.py (18)
  • CircleEnergyLogsRequest (1131-1165)
  • response (124-128)
  • send (366-375)
  • send (388-397)
  • send (439-441)
  • send (477-486)
  • send (513-522)
  • send (543-554)
  • send (567-576)
  • send (589-598)
  • send (622-631)
  • send (668-677)
  • send (720-729)
  • send (776-785)
  • send (809-818)
  • send (849-858)
  • send (881-890)
  • send (913-922)
tests/test_usb.py (3)
plugwise_usb/nodes/sed.py (4)
  • set_awake_duration (233-254)
  • battery_config (357-366)
  • dirty (345-347)
  • awake_duration (350-354)
plugwise_usb/api.py (2)
  • set_awake_duration (526-549)
  • battery_config (484-495)
plugwise_usb/nodes/node.py (2)
  • set_awake_duration (736-742)
  • battery_config (121-127)
⏰ 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: Check commit
🔇 Additional comments (12)
tests/test_usb.py (1)

2003-2010: Test value updated to match new awake_duration value.

The test correctly updates the awake_duration from 15 to 20 seconds, and the assertions properly verify this new value in both battery_config.awake_duration and awake_duration. The change aligns with the battery configuration handling improvements in the PR.

.github/workflows/merge.yml (1)

29-29: Good workflow consistency improvement!

Updating the checkout action to v5 aligns with the other workflow files in this PR, maintaining consistency across the CI/CD pipeline. This is a sensible upgrade that ensures all workflows use the same tooling versions.

.github/workflows/verify.yml (2)

26-26: Excellent addition of Python version propagation!

Adding the python-version output ensures all jobs in the workflow use the exact same Python minor version. This is a robust pattern that prevents version mismatches across different job runners.


257-261: Good artifact handling upgrade!

The update to download-artifact@v5 with the new inputs (pattern, merge-multiple, path) provides better control over artifact collection. The change from coverage*/.coverage* to artifacts/.coverage* simplifies the directory structure.

plugwise_usb/nodes/circle.py (8)

77-77: Well-designed constant for energy log retention.

The MAX_LOG_HOURS constant provides a clear, configurable boundary for energy log retention. Using DAY_IN_HOURS as its value (24 hours) is a sensible default that balances data retention with memory usage.


83-117: Robust cache record parsing with proper error handling.

The _collect_records function effectively parses the pipe-delimited cache format with good defensive programming:

  • Handles both strptime and manual date parsing as fallback
  • Validates field count before processing
  • Prevents duplicate slot entries by keeping only the first occurrence

The function correctly maintains the newest-first semantics mentioned in the learnings.


571-627: Improved method signature with boolean return and cache control.

The updated energy_log_update method signature provides better control and clearer semantics:

  • Boolean return value clearly indicates success/failure
  • save_cache parameter allows callers to control caching behavior
  • Early return for None address prevents unnecessary processing

The implementation correctly updates cache only when needed and logs appropriate debug messages.


629-646: Good addition of timestamp validation for energy logs.

The _check_timestamp_is_recent method properly filters out stale energy logs older than MAX_LOG_HOURS. The informative log message helps with debugging when old records are ignored.


647-696: Well-structured cache loading with proper pruning.

The cache loading implementation correctly:

  • Filters out entries older than MAX_LOG_HOURS using the cutoff timestamp
  • Processes entries in reverse-sorted order (newest first)
  • Creates background tasks for missing logs when needed
  • Uses the import_only flag to prevent triggering updates during cache load

697-718: Efficient cache persistence with immediate disk write.

The new cache saving implementation:

  • Serializes logs in a compact, timestamp-based format
  • Maintains newest-first ordering (as per the learnings)
  • Triggers immediate cache write with trigger_only=True to avoid blocking

The format address:slot:YYYY-MM-DD-HH-MM-SS:pulses is clear and parseable.


496-531: Smart initial energy log collection with early termination.

The refactored _get_initial_energy_logs method efficiently:

  • Calculates the appropriate number of addresses based on consumption/production intervals
  • Limits collection to MAX_LOG_HOURS worth of data
  • Stops early when encountering empty or outdated logs (avoiding unnecessary requests)
  • Uses save_cache=False during collection, then saves once at the end

535-570: Improved error handling in missing log collection.

The updated get_missing_energy_logs method now properly handles failures:

  • Creates tasks for all missing addresses
  • Cancels remaining tasks if one fails (avoiding wasted requests)
  • Properly drains cancelled tasks with gather(..., return_exceptions=True)
  • Saves cache only after all collection is complete

This is a significant improvement in error handling and efficiency.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🔭 Outside diff range comments (1)
plugwise_usb/nodes/sed.py (1)

5-5: Fix timeout handling: catch asyncio.TimeoutError from wait_for.

asyncio.wait_for raises asyncio.TimeoutError, not the built-in TimeoutError. The current except TimeoutError block won’t run, so nodes may never be marked unavailable on missed awake.

Apply this diff:

-from asyncio import CancelledError, Future, Task, gather, get_running_loop, wait_for
+from asyncio import CancelledError, Future, Task, TimeoutError, gather, get_running_loop, wait_for

No other code changes are needed since the except TimeoutError now refers to asyncio’s TimeoutError due to the import.

Also applies to: 543-557

♻️ Duplicate comments (1)
plugwise_usb/nodes/sed.py (1)

245-246: Standardize early-exit equality checks to use effective values (battery_config).

Using self._battery_config.* can be None pre-init and triggers unnecessary updates when the requested value equals the default/effective value. Compare against self.battery_config.* for consistent no-op semantics across setters.

Apply this diff:

@@
-        if self._battery_config.awake_duration == seconds:
+        if self.battery_config.awake_duration == seconds:
             return False
@@
-        if self._battery_config.clock_sync == sync:
+        if self.battery_config.clock_sync == sync:
             return False
@@
-        if self._battery_config.sleep_duration == minutes:
+        if self.battery_config.sleep_duration == minutes:
             return False

Also applies to: 286-287, 333-334

🧹 Nitpick comments (1)
plugwise_usb/nodes/sed.py (1)

248-254: DRY the repeated 'replace + dirty=True + publish' pattern via a small helper.

Each setter repeats the same sequence. A private helper improves readability and reduces future drift.

Apply this diff within the setters:

@@
-        self._battery_config = replace(
-            self._battery_config,
-            awake_duration=seconds,
-            dirty=True,
-        )
-        await self._sed_configure_update()
+        await self._apply_battery_update(awake_duration=seconds)
@@
-        self._battery_config = replace(
-            self._battery_config, clock_interval=minutes, dirty=True
-        )
-        await self._sed_configure_update()
+        await self._apply_battery_update(clock_interval=minutes)
@@
-        self._battery_config = replace(
-            self._battery_config, clock_sync=sync, dirty=True
-        )
-        await self._sed_configure_update()
+        await self._apply_battery_update(clock_sync=sync)
@@
-        self._battery_config = replace(
-            self._battery_config, maintenance_interval=minutes, dirty=True
-        )
-        await self._sed_configure_update()
+        await self._apply_battery_update(maintenance_interval=minutes)
@@
-        self._battery_config = replace(
-            self._battery_config, sleep_duration=minutes, dirty=True
-        )
-        await self._sed_configure_update()
+        await self._apply_battery_update(sleep_duration=minutes)

And add this helper (outside the ranges, e.g., near other config helpers):

async def _apply_battery_update(self, **changes: Any) -> None:
    """Replace battery config with given changes, mark dirty, persist and publish."""
    self._battery_config = replace(self._battery_config, **changes, dirty=True)
    await self._sed_configure_update()

Also applies to: 272-276, 289-293, 311-315, 336-340

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 05852e7 and 7339d83.

📒 Files selected for processing (3)
  • plugwise_usb/nodes/scan.py (1 hunks)
  • plugwise_usb/nodes/sed.py (6 hunks)
  • tests/test_usb.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • plugwise_usb/nodes/scan.py
  • tests/test_usb.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T17:50:29.188Z
Learnt from: bouwew
PR: plugwise/python-plugwise-usb#302
File: plugwise_usb/nodes/scan.py:132-142
Timestamp: 2025-08-07T17:50:29.188Z
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/sed.py
🧬 Code Graph Analysis (1)
plugwise_usb/nodes/sed.py (2)
plugwise_usb/nodes/helpers/subscription.py (1)
  • publish_feature_update_to_subscribers (51-66)
plugwise_usb/api.py (1)
  • NodeFeature (39-58)
⏰ 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 (1)
plugwise_usb/nodes/sed.py (1)

178-182: Good addition: publish non-dirty cached battery config on load.

Publishing BATTERY state in the non-dirty path ensures subscribers receive the current config after startup without waiting for the next awake. This aligns with the PR objective.

@dirixmjm dirixmjm marked this pull request as ready for review August 16, 2025 08:06
@dirixmjm dirixmjm requested a review from a team as a code owner August 16, 2025 08:06
@dirixmjm dirixmjm requested a review from bouwew August 16, 2025 08:07
Copy link
Contributor

@bouwew bouwew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@coderabbitai coderabbitai bot mentioned this pull request Aug 16, 2025
@sonarqubecloud
Copy link

@dirixmjm dirixmjm merged commit 405ec7e into main Aug 16, 2025
15 checks passed
@dirixmjm dirixmjm deleted the mdi_sed branch August 16, 2025 11:43
bouwew added a commit that referenced this pull request Aug 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants