Skip to content

Fix kodi media_player unavailable at start#41714

Merged
bdraco merged 4 commits intohome-assistant:devfrom
mvn23:kodi-unavailable-fix
Nov 17, 2020
Merged

Fix kodi media_player unavailable at start#41714
bdraco merged 4 commits intohome-assistant:devfrom
mvn23:kodi-unavailable-fix

Conversation

@mvn23
Copy link
Copy Markdown
Contributor

@mvn23 mvn23 commented Oct 12, 2020

Proposed change

After the recent overhaul of the kodi integration, the media player entity is unavailable when Home Assistant starts until the related kodi instance comes online. This prevents the power button from showing up on the media player card.
This PR changes the behavior so the media player entities are off at startup, allowing for power-on actions through the UI.
The changes are largely based on changes made by alexanv1@571780f with some additional fixes as mentioned in the discussion of the issue at #40450

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

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][dev-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:

  • (N/A) Documentation added/updated for [www.home-assistant.io][docs-repository]

If the code communicates with devices, web services, or third-party tools:

  • The [manifest file][manifest-docs] 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][quality-scale]:

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

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @alexanv1,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @alexanv1,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Copy link
Copy Markdown
Contributor

@OnFreund OnFreund left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mvn23
Left some minor comments.

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @alexanv1,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

1 similar comment
@homeassistant
Copy link
Copy Markdown
Contributor

Hi @alexanv1,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

@mvn23
Copy link
Copy Markdown
Contributor Author

mvn23 commented Oct 13, 2020

@alexanv1 are you planning to sign the CLA? If not, then do you mind if I remove you from the history in this PR to make sure we can proceed here?

@rschuiling
Copy link
Copy Markdown

Can this be merged? It seems a lot of people are waiting for it (including me :-).

Comment on lines +376 to +377
def start_watchdog(event=None):
"""Start websocket watchdog."""
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def start_watchdog(event=None):
"""Start websocket watchdog."""
async def start_websocket(event=None):
"""Start websocket."""
await self._async_connect_websocket_if_disconnected()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this may get the initial state 10 seconds (WEBSOCKET_WATCHDOG_INTERVAL) faster if kodi is available and responding, it will block HA startup longer than necessary if the kodi instance does not respond.
In my opinion, it's better to wait a bit and let HA start up before making the first connection attempt.

Copy link
Copy Markdown
Member

@bdraco bdraco Nov 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since its not happing until after the start event and its async, it shouldn't block anything since I/O is being awaited. We don't call async_block_till_done after the STARTED event.

What am I missing here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mistake, it should indeed be possible to connect right away. I will make the change soon.

# If Home Assistant is already in a running state, start the watchdog
# immediately, else trigger it after Home Assistant has finished starting.
if self.hass.state == CoreState.running:
start_watchdog()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
start_watchdog()
await start_websocket()

Copy link
Copy Markdown
Member

@bdraco bdraco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to start the websocket right away after the started event is fired without having to wait.

Suggested change above (untested)

@bdraco bdraco merged commit 212fb57 into home-assistant:dev Nov 17, 2020
@bdraco
Copy link
Copy Markdown
Member

bdraco commented Nov 17, 2020

Thanks

@mvn23 mvn23 deleted the kodi-unavailable-fix branch November 17, 2020 17:37
KJonline pushed a commit to Pyhass/core that referenced this pull request Nov 17, 2020
* 'dev' of https://github.com/home-assistant/core: (77 commits)
  Fix kodi media_player unavailable at start (home-assistant#41714)
  Add an option to template delay_on/off in template binary sensor (home-assistant#43259)
  Bump hatasmota to 0.0.31 (home-assistant#43319)
  Update cloud integration to 0.38.0 (home-assistant#43314)
  Add progress translation key to hassfest (home-assistant#43311)
  Bump codecov/codecov-action from v1.0.14 to v1.0.15 (home-assistant#43304)
  Improvement to allow parsing of station ID in vasttrafik integration. Addresses home-assistant#34851 (home-assistant#43136)
  Abort vizio discovery flow without unique ID (home-assistant#43303)
  Update directv to 0.4.0 (home-assistant#43302)
  Add notification binary_sensor to Plugwise integration (home-assistant#41473)
  [ci skip] Translation update
  Bump bimmer_connected to 0.7.13 (home-assistant#43294)
  Bump aioguardian to 1.0.4 (home-assistant#43299)
  Refactor how entities are created for homekit_controller services (home-assistant#43242)
  Updated frontend to 20201111.1 (home-assistant#43298)
  Update pytradfri to 7.0.4 (home-assistant#43297)
  Remove pts adjustments in stream (home-assistant#42399)
  Fix Enigma2 available entity property (home-assistant#43292)
  Make MQTT climate return PRESET_NONE when no preset is set (home-assistant#43257)
  Bump env_canada to 0.2.4, fix config validation (home-assistant#43251)
  ...
@Gerben321
Copy link
Copy Markdown

Is this in 0.118? Still getting this error.

@mvn23
Copy link
Copy Markdown
Contributor Author

mvn23 commented Nov 19, 2020

Nope, only in dev at the moment. Should be in 0.119.0 though.

@Gerben321
Copy link
Copy Markdown

Allright thanks for the update. Great job!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

8 participants