Add Upnp volume control/status to SamsungTV #68663

epenet · 2022-03-25T08:01:24Z

#68717 needs to be merged first

Proposed change

Add Upnp volume control/status to SamsungTV
Adds SUPPORT_VOLUME_SET to _attr_supported_features

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)
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
The code has been formatted using Black (black --fast 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.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

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

probot-home-assistant · 2022-03-25T08:01:30Z

Hey there @chemelli74, mind taking a look at this pull request as it has been labeled with an integration (samsungtv) you are listed as a code owner for? Thanks!
_{^{(message by CodeOwnersMention)}}

epenet · 2022-03-25T08:21:59Z

cc @bdraco, @sermayoral

I still need to add tests, and parse the source list.
However, since GetSourceList isn't supported on my TV I cannot complete the source list just yet.

epenet · 2022-03-25T10:07:16Z

cc @bdraco, @sermayoral

I still need to add tests, and parse the source list. However, since GetSourceList isn't supported on my TV I cannot complete the source list just yet.

I have removed the Upnp app_list request - I can add in a follow-up PR
I think this is good to go

sermayoral · 2022-03-25T12:46:58Z

@epenet. What do you need?

epenet · 2022-03-25T12:48:35Z

@epenet. What do you need?

For this PR => just confirmation that these three work correctly for you:

get volume level
set volume level
get mute status

epenet · 2022-03-25T12:58:07Z

@epenet. What do you need?

For Upnp app-list, maybe you can ping me on discord (https://discord.com/invite/home-assistant - epenet#1188) to discuss further

sermayoral · 2022-03-25T13:24:33Z

For Upnp app-list, maybe you can ping me on discord (https://discord.com/invite/home-assistant - epenet#1188) to discuss further

Not A Number:

Here is the full log: https://pastebin.com/rV1HyFx9

epenet · 2022-03-25T15:21:53Z

@sermayoral I have set a default value for the TVs that do not respond on url http://{self._host}:9197/dmr

I am not sure how to handle TVs that have Upnp on a different URL.

homeassistant/components/samsungtv/media_player.py

bdraco · 2022-03-25T20:17:10Z

We need to add "async-upnp-client==0.27.0" to manifest.json

bdraco · 2022-03-25T20:32:31Z

  "ssdp": [
    {
      "st": "urn:samsung.com:device:RemoteControlReceiver:1"
    },
    {
      "manufacturer": "Samsung",
      "st": "urn:schemas-upnp-org:service:RenderingControl:1"
    },
    {
      "manufacturer": "Samsung Electronics",
      "st": "urn:schemas-upnp-org:service:RenderingControl:1"
    }    
  ],

ssdp needs to change as well

bdraco · 2022-03-26T10:15:37Z

Testing looks good

I'll split this after some sleep

A reauth flow is no longer required to detect the need for encryption as it can now be detected during setup Fixes being able to setup legacy TVs since the encrypted check previously would prevent them from proceeding Improves the failure case where the user cannot hit the button to allow Home Assistant connect to the TV in time by allowing them to retry

bdraco · 2022-03-26T20:26:16Z

Changes split into #68717

bdraco · 2022-03-27T20:38:22Z

Fixed the conflicts

homeassistant/components/samsungtv/media_player.py

bdraco · 2022-03-27T21:36:36Z

In testing found a case where reauth was requested when it wasn't needed. If you turn off the tv during an update

2022-03-27 11:28:29 INFO (MainThread) [homeassistant.components.samsungtv] Failed to get remote for 192.168.106.51, re-authentication required: ConnectionClosedError(None, Close(code=1011, reason='keepalive ping timeout'), None)

I'll open another PR for that: #68762

MartinHjelmare · 2022-03-28T01:36:24Z

homeassistant/components/samsungtv/media_player.py

+            return
+
+        get_volume, get_mute = await asyncio.gather(
+            service.action("GetVolume").async_call(InstanceID=0, Channel="Master"),


I don't think this is in line with our design requirements. Protocol details should stay in a 3rd party library. The library should handle the UPnP setup and command details.

I figured this was ok since we have the similar pattern in the upnp and dlna* integrations

Not sure it's possible, and I'm out of time to dig in more for today, but maybe we create a new profile for Samsung Tv

async_upnp_client.profiles.samsungtv?

I would consider them exceptions as they are general integrations similar to mqtt.

The Samsung TV integration is not that.

@epenet async_upnp_client.profiles.samsungtv or a new library? It doesn't seem to fit in any of the existing ones.

Not sure what is better

I haven't looked into the details, and I won't be back home for a while so but I wonder if we could actually get subscriptions working as well so we could get push updates.

Add Upnp to SamsungTV

2a8e540

probot-home-assistant bot added integration: samsungtv new-feature labels Mar 25, 2022

project-bot bot added this to Needs review in Dev Mar 25, 2022

homeassistant added the cla-signed label Mar 25, 2022

probot-home-assistant bot assigned chemelli74 Mar 25, 2022

probot-home-assistant bot added the by-code-owner label Mar 25, 2022

project-bot bot moved this from Needs review to By Code Owner in Dev Mar 25, 2022

epenet added 5 commits March 25, 2022 08:42

Default UpnpFactory to fail

be18243

Add logs for set_volume_level

a42b341

Add tests

7d4069a

Add TODO

c4557a0

Remove upnp app_list request

0412c51

epenet marked this pull request as ready for review March 25, 2022 10:07

epenet requested a review from chemelli74 as a code owner March 25, 2022 10:07

epenet changed the title ~~Add Upnp services to SamsungTV~~ Add Upnp volume control/status to SamsungTV Mar 25, 2022

epenet requested review from bdraco and removed request for chemelli74 March 25, 2022 10:07

bdraco reviewed Mar 25, 2022

View reviewed changes

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

adjust

ffa7100

bdraco added 4 commits March 26, 2022 10:15

Merge branch 'dev' into samsungtv-upnp

bd93c3e

Update tests/components/samsungtv/__init__.py

2391ae9

Update tests/components/samsungtv/__init__.py

e4681ae

bdraco mentioned this pull request Mar 26, 2022

Add support for setting up encrypted samsung tvs from config flow #68717

Merged

22 tasks

bdraco added 7 commits March 26, 2022 10:27

isort

cbdd7e6

rename ssdp key since we will need one for main_tv_agent as well

295b284

Merge branch 'samsungtv_encrypted_flow' into samsungtv-upnp

5c81b1e

rename for merge fix

2e76a31

Merge remote-tracking branch 'upstream/dev' into samsungtv-upnp

d71f0bc

one more conflict

9283508

guards

e758cba

bdraco approved these changes Mar 27, 2022

View reviewed changes

Dev automation moved this from By Code Owner to Reviewer approved Mar 27, 2022

epenet commented Mar 27, 2022

View reviewed changes

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

bdraco added 3 commits March 27, 2022 11:12

do not set SUPPORT_VOLUME_SET if there is no ssdp location

dd1d500

do not set SUPPORT_VOLUME_SET if there is no ssdp location

5ba3e9a

gather cleanup

a5177b2

Merge branch 'dev' into samsungtv-upnp

fde2473

bdraco mentioned this pull request Mar 27, 2022

Add support for finding the samsungtv MainTvAgent service location #68763

Merged

22 tasks

bdraco merged commit c024033 into home-assistant:dev Mar 27, 2022

Dev automation moved this from Reviewer approved to Done Mar 27, 2022

epenet deleted the samsungtv-upnp branch March 27, 2022 22:28

MartinHjelmare reviewed Mar 28, 2022

View reviewed changes

epenet mentioned this pull request Mar 28, 2022

Use DmrDevice to communicate with SamsungTV #68777

Merged

22 tasks

github-actions bot locked and limited conversation to collaborators Mar 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Upnp volume control/status to SamsungTV #68663

Add Upnp volume control/status to SamsungTV #68663

epenet commented Mar 25, 2022 •

edited by bdraco

probot-home-assistant bot commented Mar 25, 2022

epenet commented Mar 25, 2022 •

edited

epenet commented Mar 25, 2022

sermayoral commented Mar 25, 2022

epenet commented Mar 25, 2022

epenet commented Mar 25, 2022 •

edited

sermayoral commented Mar 25, 2022

epenet commented Mar 25, 2022

bdraco commented Mar 25, 2022

bdraco commented Mar 25, 2022

bdraco commented Mar 26, 2022

bdraco commented Mar 26, 2022

bdraco commented Mar 27, 2022

bdraco commented Mar 27, 2022 •

edited

MartinHjelmare Mar 28, 2022

bdraco Mar 28, 2022

bdraco Mar 28, 2022 •

edited

MartinHjelmare Mar 28, 2022

bdraco Mar 28, 2022

bdraco Mar 28, 2022

Add Upnp volume control/status to SamsungTV #68663

Add Upnp volume control/status to SamsungTV #68663

Conversation

epenet commented Mar 25, 2022 • edited by bdraco

Proposed change

Type of change

Additional information

Checklist

probot-home-assistant bot commented Mar 25, 2022

epenet commented Mar 25, 2022 • edited

epenet commented Mar 25, 2022

sermayoral commented Mar 25, 2022

epenet commented Mar 25, 2022

epenet commented Mar 25, 2022 • edited

sermayoral commented Mar 25, 2022

epenet commented Mar 25, 2022

bdraco commented Mar 25, 2022

bdraco commented Mar 25, 2022

bdraco commented Mar 26, 2022

bdraco commented Mar 26, 2022

bdraco commented Mar 27, 2022

bdraco commented Mar 27, 2022 • edited

MartinHjelmare Mar 28, 2022

Choose a reason for hiding this comment

bdraco Mar 28, 2022

Choose a reason for hiding this comment

bdraco Mar 28, 2022 • edited

Choose a reason for hiding this comment

MartinHjelmare Mar 28, 2022

Choose a reason for hiding this comment

bdraco Mar 28, 2022

Choose a reason for hiding this comment

bdraco Mar 28, 2022

Choose a reason for hiding this comment

epenet commented Mar 25, 2022 •

edited by bdraco

epenet commented Mar 25, 2022 •

edited

epenet commented Mar 25, 2022 •

edited

bdraco commented Mar 27, 2022 •

edited

bdraco Mar 28, 2022 •

edited