Use pyblu library in bluesound #117257

LouisChrist · 2024-05-11T12:39:26Z

Breaking change

Sources behave slightly different. Radio station providers(e.g. TuneIn, Radio Paradise) are not part of source_list anymore as they are not selectable as source. Presets which play a specific radio station can be created in the BluOS app and are displayed as sources in Home Assistant. The currently selected source tries to match those presets and falls back to the radio station provider(e.g. TuneIn, Radio Paradise).

Proposed change

Adds pypi library for all device calls: pyblu

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:

Requirement for other PR: PR: Add config_flow to bluesound

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:

Documentation added/updated for www.home-assistant.io

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.
Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

home-assistant · 2024-05-11T12:39:32Z

Hey there @thrawnarn, mind taking a look at this pull request as it has been labeled with an integration (bluesound) you are listed as a code owner for? Thanks!

Code owner commands

Code owners of bluesound 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 bluesound 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.

joostlek · 2024-05-11T12:56:27Z

homeassistant/components/bluesound/media_player.py

+        with suppress(TimeoutError):
+            await self.async_update_sync_status()
+            await self.async_update_presets()
+            await self.async_update_captures()


Why do we suppress these now?

send_bluesound_command(which i replaced by the library) was called with raise_timeout=False which suppressed TimeoutError. I pulled that logic one level up and put it here.

joostlek · 2024-05-11T12:57:07Z

homeassistant/components/bluesound/media_player.py

+        if self._sync_status is None:
+            return None


When is sync status None?

SyncStatus is one endpoint of the device. It starts with None and might be None if the device is not reachable.

If the device is not reachable, I think we should raise an issue like PlatformNotReady I think

I changed the return None to raise PlatformNotReady.

No I mean, in the async_setup or async_setup_platform. If we can't fetch the state, that means the device doesn't respond and we can't get data. Right? In that case we should raise platform during setup

Ah ok. I can do that.

There is one caveat: This integrations supports multiple devices. Throwing a PlatformNotReady would result in none of those working if one of the devices is not reachable.

Right, let me ask some others how they did this back in the day

Any updates on this?

And as some background information: Whatever we decide here will probably be removed in my other MR, which introduces a config flow and removes the yaml configuration.

I did ask someone but got no reply, and I forgot to follow up as I was busy with my thesis, but let me ask someone else instead.

joostlek · 2024-05-11T12:58:46Z

homeassistant/components/bluesound/media_player.py


    async def async_media_stop(self) -> None:
        """Send stop command."""
        if self.is_grouped and not self.is_master:
            return

-        return await self.send_bluesound_command("Pause")
+        async with timeout(BluesoundPlayer.COMMAND_TIMEOUT):


Why don't we put this timeout in the library instead?

The old code did not use the timeout functionality of aiohttp and used async with timeout(...) instead and i kept it that way.

So it essentially does the same?

Yes it should be equivalent. I had a quick look at aiohttp and it seems to use asyncio.timeout_at internally.

Honestly, if we can already make the code cleaner (by moving connection specific things to the library, like timeouts), go for it

It’s always better to put the time out as far down in the code as possible so the least amount of has to worry about a cancellation.

In this case, it’s much much better for it to be done in aiohttp

I moved the timeouts into the library. Most of the explicit timeouts have been replaced by a default timeout for all requests.

homeassistant/components/bluesound/media_player.py

joostlek · 2024-07-05T12:30:04Z

@thrawnarn Are you able to test this change?

frenck

Oh hi there @LouisChrist 👋

There is now a merge conflict on this PR, could you take a look?

I've drafted the PR for that reason. Please un-draft it once the CI passes and it is ready for review again by clicking the "Ready for review" button.

Thanks! 👍

../Frenck

Learn more about our pull request process.

This reverts commit a649a6b.

Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>

joostlek · 2024-07-19T13:23:19Z

Oh one final thing:

Sources behave slightly different. Radio station providers(e.g. TuneIn, Radio Paradise) are not part of source_list anymore as they are not selectable as source. Presets which play a specific radio station can be created in the BluOS app and are displayed as sources in Home Assistant. The currently selected source tries to match those presets and falls back to the radio station provider(e.g. TuneIn, Radio Paradise).

How come this difference?

LouisChrist · 2024-07-19T15:16:08Z

How come this difference?

The source_list contained those radio station providers before (e.g. TuneIn). But selecting them just stopped the music and did not start playing anything. I could not fix them, therefore i removed them from the list.

The problem is the underlying API. There is no consistent concept of sources. I have these "services" on my device:

Captures - physical sources - e.g. HDMI
Spotify - special service for spotify
TuneIn - radio service
RadioParadise - radio service
Presets - presets which can be created by the user

Captures can be selected and they play music. Spotify works in a similar way. Presets have a different API endpoint, but can also be selected.
Radio services on the other hand can not be selected on their own. A specific station has to be selected.

The API endpoints for selecting and listing those services are inconsistent in what they return. And the same endpoint returns different things for different services.

To summarize: It's a big mess. And this is the best solution i could find.

MartinHjelmare

Please address the comments in a new PR. Thanks!

MartinHjelmare · 2024-08-28T18:00:24Z

homeassistant/components/bluesound/media_player.py

-            return
-        await self.async_set_volume_level(current_vol + 0.01)
+        if self.volume_level is None:
+            return None


This method shouldn't return anything.

MartinHjelmare · 2024-08-28T18:00:35Z

homeassistant/components/bluesound/media_player.py

+
+        new_volume = self.volume_level + 0.01
+        new_volume = min(1, new_volume)
+        return await self.async_set_volume_level(new_volume)


home-assistant bot added cla-signed code-quality integration: bluesound labels May 11, 2024

home-assistant bot added the Quality Scale: No score label May 11, 2024

LouisChrist changed the title ~~Integrate pypi libraray: pyblu~~ Integrate pypi library: pyblu May 11, 2024

LouisChrist mentioned this pull request May 11, 2024

Add config_flow to bluesound integration #115207

Merged

20 tasks

joostlek reviewed May 11, 2024

View reviewed changes

MartinHjelmare changed the title ~~Integrate pypi library: pyblu~~ Use pyblu library in bluesound May 12, 2024

LouisChrist force-pushed the bluesound_pypi_library branch from 7b6e1a1 to 4c5a889 Compare May 29, 2024 19:13

edenhaus added the breaking-change label Jul 5, 2024

joostlek reviewed Jul 5, 2024

View reviewed changes

homeassistant/components/bluesound/media_player.py Outdated Show resolved Hide resolved

frenck reviewed Jul 7, 2024

View reviewed changes

frenck marked this pull request as draft July 7, 2024 15:09

LouisChrist and others added 7 commits July 12, 2024 18:17

Integrate pypi libraray: pyblu

596cbf4

Raise PlatformNotReady if _sync_status is not available yet

28cc42b

Revert "Raise PlatformNotReady if _sync_status is not available yet"

cb444e6

This reverts commit a649a6b.

Replace 'async with timeout' with parameter in library

3ae6c13

Set timeout back to 10 seconds

4506ee7

ruff fixes

138e4ac

Update homeassistant/components/bluesound/media_player.py

d48ab36

Co-authored-by: Joost Lekkerkerker <joostlek@outlook.com>

LouisChrist force-pushed the bluesound_pypi_library branch from a6d9a2a to d48ab36 Compare July 12, 2024 16:18

LouisChrist marked this pull request as ready for review July 12, 2024 16:30

Merge branch 'dev' into bluesound_pypi_library

578c488

joostlek approved these changes Jul 19, 2024

View reviewed changes

joostlek merged commit f629364 into home-assistant:dev Jul 21, 2024
39 of 40 checks passed

github-actions bot locked and limited conversation to collaborators Jul 22, 2024

LouisChrist deleted the bluesound_pypi_library branch August 3, 2024 09:44

MartinHjelmare reviewed Aug 28, 2024

View reviewed changes

home-assistant unlocked this conversation Aug 28, 2024

github-actions bot locked and limited conversation to collaborators Aug 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pyblu library in bluesound #117257

Use pyblu library in bluesound #117257

LouisChrist commented May 11, 2024

home-assistant bot commented May 11, 2024

joostlek May 11, 2024

LouisChrist May 11, 2024

joostlek May 11, 2024

LouisChrist May 11, 2024

joostlek May 11, 2024

LouisChrist May 12, 2024

joostlek May 12, 2024

LouisChrist May 12, 2024 •

edited

Loading

joostlek May 12, 2024

LouisChrist May 29, 2024

joostlek May 29, 2024

joostlek May 11, 2024

LouisChrist May 11, 2024

joostlek May 11, 2024

LouisChrist May 12, 2024

joostlek May 29, 2024

bdraco May 29, 2024

LouisChrist May 29, 2024

joostlek commented Jul 5, 2024

frenck left a comment

joostlek commented Jul 19, 2024

LouisChrist commented Jul 19, 2024 •

edited

Loading

MartinHjelmare left a comment

MartinHjelmare Aug 28, 2024

MartinHjelmare Aug 28, 2024

Use pyblu library in bluesound #117257

Use pyblu library in bluesound #117257

Conversation

LouisChrist commented May 11, 2024

Breaking change

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented May 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LouisChrist May 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joostlek commented Jul 5, 2024

frenck left a comment

Choose a reason for hiding this comment

joostlek commented Jul 19, 2024

LouisChrist commented Jul 19, 2024 • edited Loading

MartinHjelmare left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LouisChrist May 12, 2024 •

edited

Loading

LouisChrist commented Jul 19, 2024 •

edited

Loading