Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add connection test feature to assist_satellite #126256

Merged
merged 14 commits into from
Sep 22, 2024
Merged

Add connection test feature to assist_satellite #126256

merged 14 commits into from
Sep 22, 2024

Conversation

emontnemery
Copy link
Contributor

@emontnemery emontnemery commented Sep 19, 2024 •
edited
Loading

Proposed change

Add connection test feature to assist_satellite

The connection test is implemented as a websocket command which will send a unique announce media id to the targeted assist_satellite entity and wait for the assist_satellite to fetch the media.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

@home-assistant
Copy link

Hey there @home-assistant/core, @synesthesiam, mind taking a look at this pull request as it has been labeled with an integration (assist_satellite) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of assist_satellite can trigger bot actions by commenting:

  • @home-assistant close Closes the pull request.
  • @home-assistant rename Awesome new title Renames the pull request.
  • @home-assistant reopen Reopen the pull request.
  • @home-assistant unassign assist_satellite Removes the current integration label and assignees on the pull request, add the integration domain after the command.
  • @home-assistant add-label needs-more-information Add a label (needs-more-information, problem in dependency, problem in custom component) to the pull request.
  • @home-assistant remove-label needs-more-information Remove a label (needs-more-information, problem in dependency, problem in custom component) on the pull request.

@synesthesiam
Copy link
Contributor

Needs #126287 to hear the sound

@synesthesiam
Copy link
Contributor

Thanks @MartinHjelmare! Feedback has been addressed.

@synesthesiam
Copy link
Contributor

Test failure seems unrelated

balloob
balloob reviewed Sep 20, 2024
View reviewed changes
homeassistant/components/assist_satellite/connection_test.py Outdated
audio_path = Path(__file__).parent / CONNECTION_TEST_FILENAME
audio_data = await hass.async_add_executor_job(audio_path.read_bytes)

async_dispatcher_send(hass, CONNECTION_TEST_SIGNAL.format(connection_id))
Copy link
Member

Choose a reason for hiding this comment

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

Personally not a fan of dispatchers anymore, as it's unclear who will be using the signal. Not sure what the official stance from core is on this. An alternative is to share a dict with events between the HTTP view and the WebSocket command.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't agree. Searching for the signal name will immediately reveal who is involved in the signal. Why reinvent the wheel?

Copy link
Member

Choose a reason for hiding this comment

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

The problem is the lack of typing on the dispatcher. The things are too loosely coupled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, that's indeed a good point 👍

Copy link
Member

Choose a reason for hiding this comment

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

Can't we add the same type mechanism for that as for hass.data?

@synesthesiam synesthesiam marked this pull request as draft September 20, 2024 18:45
@synesthesiam
Copy link
Contributor

Converting to a draft for some more discussion

balloob
balloob reviewed Sep 21, 2024
View reviewed changes
homeassistant/components/assist_satellite/websocket_api.py Outdated
return

# Send response indicating the command is accepted
connection.send_result(msg["id"])
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this should be a subscription or that actually a normal call could be enough?

balloob
balloob approved these changes Sep 21, 2024
View reviewed changes
Copy link
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

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

Ok to merge when frontend signs off on usage 👍

@balloob balloob marked this pull request as ready for review September 21, 2024 21:44
@bramkragten
Copy link
Member

Frontend approves ✅

@bramkragten bramkragten merged commit 8158ca7 into dev Sep 22, 2024
44 checks passed
@bramkragten bramkragten deleted the assist_satellite_connection_test branch September 22, 2024 14:55
zxdavb pushed a commit to zxdavb/hass that referenced this pull request Sep 22, 2024
@emontnemery @synesthesiam
@balloob @bramkragten @MartinHjelmare @zxdavb
Add connection test feature to assist_satellite (home-assistant#126256)
648bfb0 
* Add connection test feature to assist_satellite

* Add http to assist_satellite dependencies

* Remove extra logging

* Incorporate feedback

* Fix tests

* ruff

* Apply suggestions from code review

Co-authored-by: Bram Kragten <[email protected]>

* Use asyncio.Event instead of dispatcher

* Respond asap

* Update homeassistant/components/assist_satellite/websocket_api.py

Co-authored-by: Martin Hjelmare <[email protected]>

---------

Co-authored-by: Michael Hansen <[email protected]>
Co-authored-by: Paulus Schoutsen <[email protected]>
Co-authored-by: Bram Kragten <[email protected]>
Co-authored-by: Martin Hjelmare <[email protected]>
zxdavb pushed a commit to zxdavb/hass that referenced this pull request Sep 22, 2024
@emontnemery @synesthesiam
@balloob @bramkragten @MartinHjelmare @zxdavb
Add connection test feature to assist_satellite (home-assistant#126256)
48481f3 
* Add connection test feature to assist_satellite

* Add http to assist_satellite dependencies

* Remove extra logging

* Incorporate feedback

* Fix tests

* ruff

* Apply suggestions from code review

Co-authored-by: Bram Kragten <[email protected]>

* Use asyncio.Event instead of dispatcher

* Respond asap

* Update homeassistant/components/assist_satellite/websocket_api.py

Co-authored-by: Martin Hjelmare <[email protected]>

---------

Co-authored-by: Michael Hansen <[email protected]>
Co-authored-by: Paulus Schoutsen <[email protected]>
Co-authored-by: Bram Kragten <[email protected]>
Co-authored-by: Martin Hjelmare <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Sep 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bramkragten bramkragten bramkragten left review comments

@MartinHjelmare MartinHjelmare MartinHjelmare approved these changes

@synesthesiam synesthesiam synesthesiam approved these changes

@balloob balloob balloob approved these changes

Assignees

@synesthesiam synesthesiam

Labels
by-code-owner cla-signed has-tests integration: assist_satellite new-feature Quality Scale: internal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@emontnemery @synesthesiam @bramkragten @balloob @MartinHjelmare