Add `samsungtv.turn_off` trigger #91903

felipecrs · 2023-04-23T15:42:17Z

Breaking change

Not a breaking change.

Proposed change

This adds the samsungtv.turn_off trigger, so that users can specify their own automations to turn off the Samsung TV, which is useful when the default does not work.

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 Can't power off Samsung Home Theater #87472
This PR is related to issue: N/A
Link to documentation pull request: TBD

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 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.

To help with the load of incoming pull requests:

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

home-assistant · 2023-04-23T15:42:25Z

Hey there @chemelli74, @epenet, 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!

Code owner commands

Code owners of samsungtv 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 samsungtv Removes the current integration label and assignees on the pull request, add the integration domain after the command.

felipecrs · 2023-04-23T15:43:27Z

This is fully working on my local, testing against my Samsung TV device.

I will wait until I receive a positive feedback before start working on the documentation PR.

homeassistant/components/samsungtv/device_trigger.py

MichaelMraka · 2023-05-13T07:39:41Z

homeassistant/components/samsungtv/triggers/turn_off.py

@@ -0,0 +1,108 @@
+"""Samsung TV device turn off trigger."""


This file is 99.7% similar to turn_on.py. You should reuse the code not duplicate it.

I think I'm too dumb to do this. I got little to no Python experience. I really tried though. Would you mind doing it yourself? I can give you the needed permissions.

…felipecrs/91903

emontnemery

Instead of adding a new module turn_off.py do something like this:

rename turn_on.py to turn_on_off.py
rename async_get_turn_on_trigger to async_get_turn_on_off_trigger and make it return a list with both the triggers
update async_attach_trigger to check config[CONF_TYPE] and set the description accordingly

emontnemery · 2023-05-24T13:06:03Z

homeassistant/components/samsungtv/device_trigger.py

+    triggers = [
+        async_get_turn_on_trigger(device_id),
+        async_get_turn_off_trigger(device_id),
+    ]


Suggested change

triggers = [

async_get_turn_on_trigger(device_id),

async_get_turn_off_trigger(device_id),

]

triggers = async_get_turn_on_off_triggers(device_id),

Thanks for the suggestion but it's a lot more complicated than that. For example, I have to refactor attach_trigger, media_player.py and triggers.py. I just spent 2 hours trying to do it, and it ended up not working.

I'm pushing my draft changes if you want to review them.

home-assistant · 2023-05-24T13:07:18Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

…felipecrs/91903

MichaelMraka

core/homeassistant/components/samsungtv/trigger.py

Line 49 in dff73a2

return await platform.async_attach_trigger(hass, config, action, trigger_info)

platform_type does not have default value now so you have to define it:
+ return await platform.async_attach_trigger(hass, config, action, trigger_info, platform_type=config[CONF_PLATFORM])

This fixes all but 3 errors in turn_off tests. I don't know fix these.

…sungtv-turnoff

felipecrs · 2023-05-26T22:46:32Z

@MichaelMraka thanks for the hint. The fix ended up being a little different though, but around the same place.

I'll investigate the rest of the issues but I believe it may have something to do with this:

https://github.com/home-assistant/core/pull/91903/files#diff-a0781812482f5a259a2d8830825092609c3331101aa8ba76df95cfb5b6c4be71R17-R21

MartinHjelmare · 2023-05-28T06:55:39Z

I'd start by investigating more about the issue first before going ahead with this PR. Why doesn't turn off work for the device?

felipecrs · 2023-05-28T17:00:12Z

I'd start by investigating more about the issue first before going ahead with this PR. Why doesn't turn off work for the device?

Me too. And I did that:

Can't power off Samsung Home Theater #87472

But there is no solution. And considering that https://github.com/Ape/samsungctl is unmaintained since 2019, I believe there is virtually no chance of having a fix for it in the library.

MartinHjelmare · 2023-05-28T19:49:29Z

We don't know what's wrong yet so we don't know where to fix it. I suggest continuing troubleshooting until there's some lead on what's the problem. Codeowners may have suggestions for troubleshooting steps.

felipecrs · 2023-05-28T19:54:11Z

Ok. My understanding was that my device is simply unable to receive such command and thus this PR would make sense.

But it's true that's my understanding only, which may be wrong.

Any help is appreciated.

MartinHjelmare · 2023-05-28T20:06:05Z

It may be that in the end we will conclude that the device doesn't support turn off but I don't think that's confirmed yet.

felipecrs · 2023-06-06T17:37:11Z

@chemelli74 @epenet maybe you can recommend any troubleshooting step, or advise if you believe this PR is the best we can do about it.

I don't think anyone else will be able to help us otherwise.

felipecrs · 2023-06-08T03:29:29Z

I was able to work around this issue by building an automation which achieves the same result:

alias: Turn off action for home theater
description: ""
trigger:
  - platform: state
    entity_id:
      - media_player.home_theater_do_escritorio
    from: "on"
    to: "off"
condition:
  - condition: template
    value_template: >-
      {{ trigger.to_state.context.user_id is not none or
      trigger.to_state.context.parent_id is not none }}
    alias: Confirm is triggered by turn_off service
action:
  - service: remote.send_command
    data:
      device: home_theater_do_escritorio
      command: power
    target:
      entity_id: remote.rm4_pro_remote
mode: single

Thus I'm closing this PR.

Add samsungtv.turn_off trigger

deec335

felipecrs requested review from chemelli74 and epenet as code owners April 23, 2023 15:42

home-assistant bot added cla-signed has-tests integration: samsungtv new-feature labels Apr 23, 2023

home-assistant bot assigned chemelli74 and epenet Apr 23, 2023

home-assistant bot added the Quality Scale: No score label Apr 23, 2023

felipecrs changed the title ~~Add samsungtv.turn_off trigger~~ Add samsungtv.turn_off trigger Apr 23, 2023

MichaelMraka suggested changes May 13, 2023

View reviewed changes

felipecrs added 2 commits May 18, 2023 23:58

Merge branch 'dev' of https://github.com/home-assistant/core into pr/…

1ea37c4

…felipecrs/91903

Fix some comments

b216d09

emontnemery requested changes May 24, 2023

View reviewed changes

home-assistant bot marked this pull request as draft May 24, 2023 13:07

felipecrs added 4 commits May 24, 2023 14:53

Merge branch 'dev' of https://github.com/home-assistant/core into pr/…

ab7b205

…felipecrs/91903

Fix Python extensions in devcontainer

b50a0b6

Fix pylint

af105a7

Try to convert in a single file

dff73a2

MichaelMraka reviewed May 26, 2023

View reviewed changes

felipecrs added 2 commits May 26, 2023 22:37

Fix missing platform_type

2e533ad

Merge branch 'dev' of https://github.com/home-assistant/core into sam…

a21ed83

…sungtv-turnoff

felipecrs closed this Jun 8, 2023

github-actions bot locked and limited conversation to collaborators Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `samsungtv.turn_off` trigger #91903

Add `samsungtv.turn_off` trigger #91903

felipecrs commented Apr 23, 2023

home-assistant bot commented Apr 23, 2023

felipecrs commented Apr 23, 2023

MichaelMraka May 13, 2023

felipecrs May 19, 2023

emontnemery left a comment •

edited

emontnemery May 24, 2023

felipecrs May 24, 2023

home-assistant bot commented May 24, 2023

MichaelMraka left a comment

felipecrs commented May 26, 2023 •

edited

MartinHjelmare commented May 28, 2023

felipecrs commented May 28, 2023 •

edited

MartinHjelmare commented May 28, 2023

felipecrs commented May 28, 2023

MartinHjelmare commented May 28, 2023

felipecrs commented Jun 6, 2023

felipecrs commented Jun 8, 2023

Add samsungtv.turn_off trigger #91903

Add samsungtv.turn_off trigger #91903

Conversation

felipecrs commented Apr 23, 2023

Breaking change

Proposed change

Type of change

Additional information

Checklist

home-assistant bot commented Apr 23, 2023

felipecrs commented Apr 23, 2023

MichaelMraka May 13, 2023

Choose a reason for hiding this comment

felipecrs May 19, 2023

Choose a reason for hiding this comment

emontnemery left a comment • edited

Choose a reason for hiding this comment

emontnemery May 24, 2023

Choose a reason for hiding this comment

felipecrs May 24, 2023

Choose a reason for hiding this comment

home-assistant bot commented May 24, 2023

MichaelMraka left a comment

Choose a reason for hiding this comment

felipecrs commented May 26, 2023 • edited

MartinHjelmare commented May 28, 2023

felipecrs commented May 28, 2023 • edited

MartinHjelmare commented May 28, 2023

felipecrs commented May 28, 2023

MartinHjelmare commented May 28, 2023

felipecrs commented Jun 6, 2023

felipecrs commented Jun 8, 2023

Add `samsungtv.turn_off` trigger #91903

Add `samsungtv.turn_off` trigger #91903

emontnemery left a comment •

edited

felipecrs commented May 26, 2023 •

edited

felipecrs commented May 28, 2023 •

edited