-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add handle_announcement_finished callback #954
Add handle_announcement_finished callback #954
Conversation
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces a new parameter, Changes
Possibly related PRs
Tip New featuresWalkthrough comment now includes:
Notes:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #954 +/- ##
=======================================
Coverage 99.88% 99.88%
=======================================
Files 17 17
Lines 2635 2640 +5
=======================================
+ Hits 2632 2637 +5
Misses 3 3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this 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, codebase verification and nitpick comments (1)
aioesphomeapi/client.py (1)
Line range hint
1292-1389
: Addition of handle_announcement_finished callback is well-integrated.The integration of the
handle_announcement_finished
callback within thesubscribe_voice_assistant
function is well done. The callback is properly set up to be invoked when aVoiceAssistantAnnounceFinished
message is received, which aligns with the PR's objectives to handle announcement completions effectively.However, there is a static analysis hint indicating a potential issue at line 1380 with "None" not callable. This suggests that there might be a scenario where
handle_announcement_finished
could beNone
when the callback is attempted to be called. This needs to be addressed to prevent runtime errors.Consider adding a check to ensure
handle_announcement_finished
is notNone
before invoking it:if handle_announcement_finished is not None: self._create_background_task(handle_announcement_finished(finished))Tools
GitHub Check: py 3.12 on ubuntu-latest (skip_cython)
[failure] 1380-1380:
"None" not callable [misc]
The
VoiceAssistantAnnounceFinished
message is sent whenever the media player finished playing a TTS response (either from a pipeline or from an announcement). This message is automatically caught insidesend_voice_assistant_announcement_await_response
, which lets us know when the announcement is finished. But we won't know when a TTS response is finished without an additional callback (added here in this PR).Unfortunately, we can't just watch the state of the media player in HA because:
PLAYING