Add configurable wait-for-fresh-push behavior for event-driven powermeters#330
Merged
Add configurable wait-for-fresh-push behavior for event-driven powermeters#330
Conversation
PR #322 wired wait_for_next_message into update_readings so push-based powermeters serve fresh data on every CT002 request. Two regressions followed: 1. Slow event-driven meters (e.g. Home Assistant wrapping a P1 that ticks every 10s, or any source with high THROTTLE_INTERVAL) raise TimeoutError on every request, which surfaces as "CT002 before_send failed" warnings and skips the cached value that would have been perfectly usable (issue #327). 2. MQTT multi-topic kept clearing the event whenever the waker happened to be a phase that had already published, so any topic still at None would force the full 5s timeout even though a fresh message had arrived. Treat wait_for_next_message as best-effort freshness: - update_readings caps the wait at 2s and swallows TimeoutError (cached value is served, which matches behaviour pre-#322). - Add WAIT_FOR_NEXT_MESSAGE (global [GENERAL] + per-section) so users can opt out of the wait entirely for sources that always update slower than the cap. Defaults to true. - MQTT wait_for_next_message returns on any next message instead of re-arming the event when not all topics have ever published. Refactor the closure inside run_device into a module-level read_ct_powermeter helper so the swallow-timeout behaviour is unit-testable.
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 48 minutes and 27 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThe PR introduces a new global configuration option Changes
Sequence Diagram(s)sequenceDiagram
participant Device as Device/Client
participant Main as Main App
participant Config as Config Loader
participant Powermeter as Powermeter (MQTT)
Device->>Main: CT reading request
Main->>Config: read powermeter config
Config-->>Main: (powermeter, filter, wait_flag)
Main->>Powermeter: select by ClientFilter
alt wait_flag == true
Main->>Powermeter: wait_for_next_message(timeout=2s)
Powermeter->>Powermeter: wait for MQTT event
alt fresh data available
Powermeter-->>Main: event received
else timeout
Powermeter-->>Main: TimeoutError (suppressed)
end
end
Main->>Powermeter: get_powermeter_watts()
Powermeter-->>Main: watt values
Main->>Main: pad to 3 phases
Main-->>Device: CT readings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
config.ini.example (1)
206-217:⚠️ Potential issue | 🟡 MinorRemove the duplicate Home Assistant throttle example.
This section now shows both
#THROTTLE_INTERVAL = 1and#THROTTLE_INTERVAL = 2, which makes the recommended Home Assistant value ambiguous.📝 Proposed cleanup
## Per-powermeter throttling override (optional) -#THROTTLE_INTERVAL = 1 +## HomeAssistant typically needs 2-3 seconds due to network latency +#THROTTLE_INTERVAL = 2 ## Skip the up-to-2s wait for a fresh push from this powermeter and always ## serve the last-known value. Useful when the underlying sensor (e.g. P1 ## smart meter) updates slower than 2s, so the wait would always time out ## and the cached value would be served anyway. Defaults to the global ## [GENERAL] WAIT_FOR_NEXT_MESSAGE setting (true). `#WAIT_FOR_NEXT_MESSAGE` = false - -## Per-powermeter throttling override (optional) -## HomeAssistant typically needs 2-3 seconds due to network latency -#THROTTLE_INTERVAL = 2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.ini.example` around lines 206 - 217, Remove the duplicate Home Assistant throttle example by deleting the first per-powermeter block that shows "#THROTTLE_INTERVAL = 1" (the block immediately above the WAIT_FOR_NEXT_MESSAGE example) so only the single Home Assistant example with "#THROTTLE_INTERVAL = 2" remains; ensure the remaining comment still explains this is a per-powermeter throttling override and optional, and that it references the global [GENERAL] WAIT_FOR_NEXT_MESSAGE behavior if present.src/astrameter/shelly/shelly.py (1)
204-212:⚠️ Potential issue | 🟠 MajorHonor
WAIT_FOR_NEXT_MESSAGEin the Shelly request path.Line 204 discards the new flag, so
DEVICE_TYPE=shelly*still serves immediately while the CT path applies the 2s best-effort wait. Capture the flag and mirror the CT behavior before Line 212.🐛 Proposed fix
- for pm, client_filter, _ in self._powermeters: + wait_for_next = False + for pm, client_filter, wait_flag in self._powermeters: if client_filter.matches(addr[0]): powermeter = pm + wait_for_next = wait_flag break if powermeter is None: logger.warning(f"No powermeter found for client {addr[0]}") return + if wait_for_next: + try: + await powermeter.wait_for_next_message(timeout=2) + except TimeoutError: + logger.debug( + "Powermeter %s produced no fresh message within 2s; " + "serving last known value", + type(powermeter).__name__, + ) + powers = await powermeter.get_powermeter_watts()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/astrameter/shelly/shelly.py` around lines 204 - 212, The code currently ignores the WAIT_FOR_NEXT_MESSAGE flag when selecting a powermeter and immediately calls powermeter.get_powermeter_watts(); capture the flag from the incoming request (same place CT reads it), store it in a local variable (e.g., wait_for_next_message), and if true mirror the CT path by awaiting the same best-effort wait (2s or the helper used by CT) before calling powermeter.get_powermeter_watts(); use the existing symbols powermeter, addr, and self._powermeters to locate where to insert the check and await so behavior matches the CT implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@config.ini.example`:
- Around line 206-217: Remove the duplicate Home Assistant throttle example by
deleting the first per-powermeter block that shows "#THROTTLE_INTERVAL = 1" (the
block immediately above the WAIT_FOR_NEXT_MESSAGE example) so only the single
Home Assistant example with "#THROTTLE_INTERVAL = 2" remains; ensure the
remaining comment still explains this is a per-powermeter throttling override
and optional, and that it references the global [GENERAL] WAIT_FOR_NEXT_MESSAGE
behavior if present.
In `@src/astrameter/shelly/shelly.py`:
- Around line 204-212: The code currently ignores the WAIT_FOR_NEXT_MESSAGE flag
when selecting a powermeter and immediately calls
powermeter.get_powermeter_watts(); capture the flag from the incoming request
(same place CT reads it), store it in a local variable (e.g.,
wait_for_next_message), and if true mirror the CT path by awaiting the same
best-effort wait (2s or the helper used by CT) before calling
powermeter.get_powermeter_watts(); use the existing symbols powermeter, addr,
and self._powermeters to locate where to insert the check and await so behavior
matches the CT implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a35c6e64-d630-480a-9f7a-14c3ea2b21ed
📒 Files selected for processing (11)
README.mdconfig.ini.examplesrc/astrameter/config/config_loader.pysrc/astrameter/config/config_loader_test.pysrc/astrameter/main.pysrc/astrameter/main_test.pysrc/astrameter/powermeter/mqtt.pysrc/astrameter/powermeter/mqtt_test.pysrc/astrameter/shelly/shelly.pysrc/astrameter/shelly/shelly_udp_test.pysrc/astrameter/web_config.py
Adds a "Wait For Fresh Home Assistant Reading" toggle (default true) to the HA app so users with slow-updating source sensors (e.g. P1 smart meters) can opt out of the up-to-2s freshness wait and serve the last-known value immediately.
The Shelly UDP handler was ignoring the per-powermeter wait flag, so a Shelly-emulated device would never wait for a fresh push from an event-driven powermeter (behaviour matched pre-#322). Mirror the CT path: read the flag when matching addr to powermeter, then best-effort await ``wait_for_next_message(timeout=2)`` with ``TimeoutError`` swallowed. Also consolidate the duplicate ``[HOMEASSISTANT]`` per-powermeter throttle example in config.ini.example: keep the more informative ``THROTTLE_INTERVAL = 2`` block (with the "2-3 seconds due to network latency" note) and drop the stray ``= 1`` duplicate that predated the ``WAIT_FOR_NEXT_MESSAGE`` example.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR introduces a configurable
WAIT_FOR_NEXT_MESSAGEsetting that controls whether astrameter waits (up to 2 seconds) for fresh data from event-driven powermeters before responding to battery requests. This prevents stale-data timeouts from breaking CT002 responses when upstream meters update slower than the freshness window.Key Changes
New
read_ct_powermeter()function inmain.py: Extracted and refactored the powermeter reading logic fromupdate_readings()into a testable, reusable function that:ClientFilterTimeoutErrorby serving the last-known cached valueConfiguration support: Added
WAIT_FOR_NEXT_MESSAGEsetting with:[GENERAL]section (defaults totrueto preserve PR Add wait_for_next_message so CT002 serves fresh powermeter data #322 behavior)Powermeter tuple expansion: Updated all powermeter tuple signatures from
(Powermeter, ClientFilter)to(Powermeter, ClientFilter, bool)to carry the wait flag throughout the codebaseSimplified MQTT wait logic in
mqtt.py: Removed the complex multi-phase consistency loop fromwait_for_next_message()— now it returns on any fresh message event (consistency checks belong inwait_for_message()/get_powermeter_watts())Comprehensive test coverage: Added 8 new tests covering:
Notable Implementation Details
read_ct_powermeter()and is not user-configurable (intentional design choice for simplicity)TimeoutErrorfrom slow push meters is silently swallowed with debug logging to prevent battery control loops from seeing stale-meter errorsupdate_readings()callback now simply delegates toread_ct_powermeter(), improving code clarity and testabilityhttps://claude.ai/code/session_01H5W8HEXpcYorYrZCxJcBvS
Summary by CodeRabbit
New Features
WAIT_FOR_NEXT_MESSAGEconfiguration option to control powermeter update waiting behavior; configurable both globally and per-powermeter with a 2-second timeout (defaults to true).Improvements
Documentation
WAIT_FOR_NEXT_MESSAGEsetting and examples.