Add ws66i core integration#56094
Conversation
|
Looks like a cool device (super expensive though). |
|
I wasn't sure about translations; I thought they were created manually. I included them because it is the same as the monoprice integration, except I swapped "port" with "ip address". Those will have to be checked... Or I just remove them altogether. |
|
You should remove all the translation files (but keep |
|
Can we run automated tests to verify my PR meets all requirements? |
|
Thanks for the review @bdraco. I pushed changes based on it. |
I suggest removing the services for now and working through them in a followup PR. |
|
Sorry, I was getting ahead of myself. I was trying to continue to make improvements during a PR and that just makes it more difficult for you, the reviewer. About custom services, these services should stay however; they are vital components to the integration. A basic automation will, for example, listen for an event, such as a doorbell rang event, snapshot the zones, play a notification indicating someone is at the door, and then restore the zones. In this use case, the snapshot will no longer be needed because a new snapshot will be taken whenever a notification needs to be played. Come to think of it, snapshots don't need to be in non-volatile memory at all. I won't be making any more changes. In a future PR, I'll look into discovery. I hope this is acceptable. |
…e ZoneStatus for attributes
… service if no snapshot
|
Updated to use pyws66i v1.1. Reviewed the quality scale and upgraded my integration to Silver. |
|
LGTM once #56094 (comment) is addressed |
MartinHjelmare
left a comment
MartinHjelmare
left a comment
There was a problem hiding this comment.
Please address the comments in a new PR. Thanks!
| return unload_ok | ||
|
|
||
|
|
||
| async def _update_listener(hass: HomeAssistant, entry: ConfigEntry): |
There was a problem hiding this comment.
Missing return value typing. Please type the whole signature when adding type annotations.
| } | ||
|
|
||
|
|
||
| async def validate_input(hass: core.HomeAssistant, input_data): |
There was a problem hiding this comment.
Please add type annotations to the whole signature.
| await hass.async_add_executor_job(ws66i.open) | ||
| # No exception. run a simple test to make sure we opened correct port | ||
| # Test on FIRST_ZONE because this zone will always be valid | ||
| ret_val = await hass.async_add_executor_job(ws66i.zone_status, FIRST_ZONE) |
There was a problem hiding this comment.
Please combine multiple calls that need to run in the executor into one function that we schedule once on the executor. Switching context is expensive.
| zones=zones, | ||
| ) | ||
|
|
||
| def shutdown(event): |
There was a problem hiding this comment.
If close is async safe we should decorate the callback with @callback.
| try: | ||
| info = await validate_input(self.hass, user_input) | ||
| # Data is valid. Add default values for options flow. | ||
| return self.async_create_entry( |
There was a problem hiding this comment.
Please only wrap the line that can raise in the try... except block. We can use an else: block if needed.
| "unknown": "[%key:common::config_flow::error::unknown%]" | ||
| }, | ||
| "abort": { | ||
| "already_configured": "[%key:common::config_flow::abort::already_configured_device%]" |
There was a problem hiding this comment.
This abort reason is never used. We don't abort for any reason currently in the config flow.
There was a problem hiding this comment.
Do I update the translation file as well?
There was a problem hiding this comment.
You can run the translations develop script if you want to test the flow manually. It's not needed to update the translation files for merging a PR. Our infrastructure will update the translation files separately by fetching from Lokalise.
|
|
||
| async def test_form(hass): | ||
| """Test we get the form.""" | ||
| await setup.async_setup_component(hass, "persistent_notification", {}) |
There was a problem hiding this comment.
We don't need to set up the persistent_notification integration anymore.
| await hass.services.async_call(DOMAIN, name, service_data=data, blocking=True) | ||
|
|
||
|
|
||
| async def test_cannot_connect(hass): |
There was a problem hiding this comment.
This test should move to a test_init.py module.
| hass, SERVICE_SELECT_SOURCE, {"entity_id": ZONE_1_ID, "source": "three"} | ||
| ) | ||
|
|
||
| # await coordinator.async_refresh() |
There was a problem hiding this comment.
Commented code. Please remove it.
| hass, SERVICE_SELECT_SOURCE, {"entity_id": ZONE_1_ID, "source": "one"} | ||
| ) | ||
|
|
||
| ws66i_data = hass.data[DOMAIN][config_entry.entry_id] |
There was a problem hiding this comment.
Please don't access integration details like hass.data or the coordinator in the test. Move time forward to make the coordinator refresh data.
Lines 375 to 379 in f50681e
https://developers.home-assistant.io/docs/development_testing#writing-tests-for-integrations
| platform.async_register_entity_service( | ||
| SERVICE_SNAPSHOT, | ||
| {}, | ||
| "snapshot", | ||
| ) | ||
|
|
||
| platform.async_register_entity_service( | ||
| SERVICE_RESTORE, | ||
| {}, | ||
| "async_restore", | ||
| ) |
There was a problem hiding this comment.
Came here from the Docs PR: Why does this integration provide its own logic for handling creating snapshot/restoring of entity state?
We have scenes in Home Assistant providing that functionality.
There was a problem hiding this comment.
I have these two services because I modeled this integration after the monoprice integration.
I was just playing with scenes after you mentioned this. I couldn't see a way for the scene to save the state of the entity at the time of an automation being triggered. A use case of the snapshots/restore is:
- User is listening to background music in the kitchen on source 3, volume 10
- Doorbell is rung
- An automation is triggered
a. The snapshot service is called to save the state
b. A scene is loaded, which sets the source to 4, volume to 20
c. A media file is played ("ding-dong" notification sound)
d. The restore service is called to revert state back to source 3, volume 10
The user is notified that someone is at the door, and the background music resumes. Can this behavior be replicated without these services?
There was a problem hiding this comment.
That is exactly the services scene provides and was created for. Scenes allow you to capture states and restore states (it actually uses state restore under the hood).
For example, you could create a snapshot of a speaker, before doing anything:
- service: scene.create
data:
scene_id: snapshot_of_sonos_office
snapshot_entities:
- media_player.sonos_officeNext, do the stuff one wants... and after that sequence is done, restore the previous snapped scene:
- service: scene.turn_on
target:
entity_id: scene.snapshot_of_sonos_officeThere was a problem hiding this comment.
Perfect! Thanks for the thorough response. Those custom services are not needed. I'll remove them and add that change to the PR that @MartinHjelmare is requesting.
|
@MartinHjelmare , yours and Franck's requested changes are in PR #71717 |
6 participants
Proposed change
I've added a new core integration called ws66i. This is based off of the https://www.home-assistant.io/integrations/monoprice integration but uses an updated amplifier that enables control via a different protocol (Telnet). Created and added new dependency https://pypi.org/project/pyws66i/ to manage the device interface.
WS66i product page: https://www.soundavo.com/products/ws-66i
This is my first time working with Python. I'm a Bluetooth Software Engineer and I program in C mainly.
Type of change
Additional information
Checklist
black --fast homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all..coveragerc. ALL MY FILES HAVE BEEN TESTEDThe integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: