Add wait_for_next_message so CT002 serves fresh powermeter data#322
Merged
Add wait_for_next_message so CT002 serves fresh powermeter data#322
Conversation
The CT002's before_send callback now awaits wait_for_next_message() before reading the powermeter, ensuring push-based meters (MQTT, SMA, HomeWizard, Home Assistant) deliver a fresh measurement for every battery request instead of re-serving stale cached values.
|
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 22 minutes and 10 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 (1)
WalkthroughA new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/astrameter/powermeter/mqtt_test.py (1)
103-125: Good baseline tests; add a multi-topic cold-start case.Current coverage is single-topic only. Please add a regression test where subscriptions are multi-topic and only one topic updates first, to lock down expected
wait_for_next_messagebehavior beforeget_powermeter_watts().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/astrameter/powermeter/mqtt_test.py` around lines 103 - 125, Add a new async test that creates a multi-topic power meter via _make_pm (simulate multi-topic subscription), starts with a cold start (no pm._message_event set and no initial values), then schedule an update on only one of the subscribed topics (set its value and pm._message_event.set()) after a short sleep; call pm.wait_for_next_message(timeout=...) and assert it unblocks and that pm.get_powermeter_watts() returns the expected value for the topic that updated first. Ensure the test exercises wait_for_next_message and get_powermeter_watts with multi-topic state to lock down the cold-start behavior when only one topic produces the first message.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/astrameter/powermeter/mqtt.py`:
- Around line 176-181: The wait_for_next_message method can return after a
single topic update in multi-topic mode, leaving other phase values None and
causing get_powermeter_watts() to fail; modify wait_for_next_message to not
return until the internal multi-topic state is complete by checking the object's
phase/topic state (e.g. self._phases, self._state or expected topic list) after
the _message_event is set — use a loop that awaits _message_event.wait() with
the remaining timeout, clears the event, then verifies all expected
topics/phases are non-None before returning, and raise TimeoutError if the
overall timeout expires; keep the method name wait_for_next_message and the same
timeout semantics.
---
Nitpick comments:
In `@src/astrameter/powermeter/mqtt_test.py`:
- Around line 103-125: Add a new async test that creates a multi-topic power
meter via _make_pm (simulate multi-topic subscription), starts with a cold start
(no pm._message_event set and no initial values), then schedule an update on
only one of the subscribed topics (set its value and pm._message_event.set())
after a short sleep; call pm.wait_for_next_message(timeout=...) and assert it
unblocks and that pm.get_powermeter_watts() returns the expected value for the
topic that updated first. Ensure the test exercises wait_for_next_message and
get_powermeter_watts with multi-topic state to lock down the cold-start behavior
when only one topic produces the first message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f5cca408-45d0-4a41-a0d9-3ae795641222
📒 Files selected for processing (17)
src/astrameter/ct002/ct002.pysrc/astrameter/main.pysrc/astrameter/powermeter/base.pysrc/astrameter/powermeter/homeassistant.pysrc/astrameter/powermeter/homeassistant_test.pysrc/astrameter/powermeter/homewizard.pysrc/astrameter/powermeter/homewizard_test.pysrc/astrameter/powermeter/mqtt.pysrc/astrameter/powermeter/mqtt_test.pysrc/astrameter/powermeter/pid.pysrc/astrameter/powermeter/pid_test.pysrc/astrameter/powermeter/sma_energy_meter.pysrc/astrameter/powermeter/sma_energy_meter_test.pysrc/astrameter/powermeter/throttling.pysrc/astrameter/powermeter/throttling_test.pysrc/astrameter/powermeter/transform.pysrc/astrameter/powermeter/transform_test.py
In multi-topic configurations, wait_for_next_message could return after receiving a message on just one topic, leaving other phase values as None and causing get_powermeter_watts() to fail. Now loops with a deadline until all subscribed values are populated, matching wait_for_message semantics but always waiting for a new message first. https://claude.ai/code/session_013EAvo4q396i36rhT45LNkp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/astrameter/powermeter/mqtt_test.py`:
- Around line 103-117: The test test_wait_for_next_message_blocks_until_new
currently can pass for implementations that return early because it awaits the
setter task before asserting; change the test to assert that the setter task has
already completed immediately after await pm.wait_for_next_message(timeout=2)
(e.g., check task.done() or equivalent) and then assert pm.value == 42.0 to
ensure wait_for_next_message actually blocked until the new message was set;
locate the test and the call to pm.wait_for_next_message and add the post-wait
assertion referencing the existing task variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 280d492f-5011-4f99-85a6-5878a0f8b233
📒 Files selected for processing (3)
CHANGELOG.mdsrc/astrameter/powermeter/mqtt.pysrc/astrameter/powermeter/mqtt_test.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
Replace `await task` with `assert task.done()` after wait_for_next_message returns, so the test fails if the method returns before the setter task completes. https://claude.ai/code/session_013EAvo4q396i36rhT45LNkp
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.
The CT002's before_send callback now awaits wait_for_next_message() before reading the powermeter, ensuring push-based meters (MQTT, SMA, HomeWizard, Home Assistant) deliver a fresh measurement for every battery request instead of re-serving stale cached values.
Summary by CodeRabbit
New Features
Bug Fixes
Tests