Skip to content

Change unique_id for webostv#34979

Merged
ludeeus merged 2 commits intohome-assistant:devfrom
jjlawren:webostv_fix_startup
May 11, 2020
Merged

Change unique_id for webostv#34979
ludeeus merged 2 commits intohome-assistant:devfrom
jjlawren:webostv_fix_startup

Conversation

@jjlawren
Copy link
Copy Markdown
Contributor

@jjlawren jjlawren commented Apr 30, 2020

Breaking change

A new unique_id is used for webostv media players for users running a 0.109.X release. Duplicate entities will be created with new entity IDs and old entities will need to be manually removed.

Proposed change

Summary from here:

  1. A unique_id property was added to the media_player entity which allows it to use the entity registry. (Improve LG webosTV #34147)
  2. Setting the unique_id would fail on startup if the TV was off, since the information wasn't available.
  3. A change was made to delay setup of the media_player entity when the TV was unreachable. (Add retry at startup #34656). This means the media_player entity isn't available until the TV turns on and we can connect.
  4. The media_player.turn_on service can't be called if an entity doesn't exist so this functionality is currently unavailable.

Since we can't know the MAC address if the device is unavailable, we have either the IP or pairing code available before we connect. This will have the side-effect of creating a new entity if the device is ever reset to factory defaults or unpaired.

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

Example entry for configuration.yaml:

wake_on_lan:

webostv:
    host: 192.168.1.153
    name: LG TV
    turn_on_action:
      service: wake_on_lan.send_magic_packet
      data:
        mac: "a0:6f:aa:9c:57:e2"
    customize:
      sources:
        - 'Amazon Prime Video'
        - hdmi2
        - photovideo
        - livetv

Additional information

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:

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

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

@jjlawren jjlawren requested a review from ludeeus April 30, 2020 19:24
@MartinHjelmare
Copy link
Copy Markdown
Member

MartinHjelmare commented Apr 30, 2020

This should preferably be solved as in the braviatv integration where the media_player entity is added regardless if the TV is on or not. The unique_id is taken from the config entry. The TV is only required to be on during the config flow where the needed info is fetched after connecting to the TV.

@jjlawren
Copy link
Copy Markdown
Contributor Author

Agree that moving to a config flow/entry would be ideal, but that will take some time. As of now this is addressing a regression in the integration.

Looking through the library I saw two values that appeared to be unique enough: MAC address and paring ID. This one is readily available today while exposing MAC would require a change in the library.

Another alternative is to remove the unique_id completely until someone picks up the task to convert to use a config entry.

@jjlawren
Copy link
Copy Markdown
Contributor Author

If the pairing code isn't a good enough unique_id then another approach should be used.

If it is good enough then I feel this is a good stopgap solution to keep entity registry support and restore functionality until a config entry overhaul can be done by someone.

@jjlawren
Copy link
Copy Markdown
Contributor Author

For some context, the pairing ID for my TV has never changed since it was added ~2 years ago.

@joogps
Copy link
Copy Markdown
Contributor

joogps commented Apr 30, 2020

Another alternative is to remove the unique_id completely until someone picks up the task to convert to use a config entry.

I was looking forward to doing that after I finish the works on Panasonic Viera. It'd be very straight forward I guess, practically the same kind of flow.

@joogps
Copy link
Copy Markdown
Contributor

joogps commented Apr 30, 2020

@jjlawren maybe this service could retrieve data containing a stable unique id:

@jjlawren
Copy link
Copy Markdown
Contributor Author

@jjlawren maybe this service could retrieve data containing a stable unique id:

I checked the available data provided by the TV. The only usable unique identifiers would be the pairing code or the MAC address.

@joogps
Copy link
Copy Markdown
Contributor

joogps commented Apr 30, 2020

Using a MAC address is ok I think (at least that's what the docs say)

@joogps
Copy link
Copy Markdown
Contributor

joogps commented Apr 30, 2020

But then, the config flow implies for lots of other settings. We'd need to implement the source editing list, for example (unless it was something YAML-exclusive)

@ludeeus
Copy link
Copy Markdown
Member

ludeeus commented Apr 30, 2020

Until this integration uses config flow, I think client_key is a good unique ID.

@timmo001
Copy link
Copy Markdown
Member

timmo001 commented Apr 30, 2020

I agree, so long as the client_key stays the same

@ludeeus
Copy link
Copy Markdown
Member

ludeeus commented Apr 30, 2020

that is what's being used to reauthenticate, so if that changes you would need to set it up again anyway.

@MartinHjelmare
Copy link
Copy Markdown
Member

It is ok to change unique id if we are sure that we won't change again later. IP address is not good as they could be swapped between two TVs.

Is the pairing id and client_key the same thing?

@jjlawren
Copy link
Copy Markdown
Contributor Author

Yes, those are the same thing.

@probot-home-assistant
Copy link
Copy Markdown

Hey there @bendavid, mind taking a look at this pull request as its been labeled with a integration (webostv) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@bendavid
Copy link
Copy Markdown
Contributor

I'm not familiar with the necessity/benefits of having a unique_id defined, but this seems like a reasonable solution in order to have one.

The client_key will only change if the tv is factory reset, and in that case it needs to be re-paired anyways.

@joogps
Copy link
Copy Markdown
Contributor

joogps commented Apr 30, 2020

@bendavid Entities with unique IDs can have their names and entity IDs changed through the UI, and are tied to their respective config entries

@MartinHjelmare
Copy link
Copy Markdown
Member

Are we sure that we will never be able to get the serial number from the TV?

@jjlawren
Copy link
Copy Markdown
Contributor Author

jjlawren commented May 1, 2020

I haven't found anything yet. Closest I've come across is a private call (i.e., not available) for a different API on the open source version of the OS: https://www.webosose.org/docs/reference/ls2-api/com-webos-service-systemservice/#deviceinfo-query.

Honestly, for our purposes of requiring a stable unique identifier per device, the client_key seems quite sufficient.

@ludeeus ludeeus merged commit 1e00fc2 into home-assistant:dev May 11, 2020
@lock lock Bot locked and limited conversation to collaborators May 21, 2020
@jjlawren jjlawren deleted the webostv_fix_startup branch June 16, 2020 03:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

LG webostv if TV is OFF when HA start, loss possibility to turn ON

6 participants