Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix known_hosts for older chromecasts #762

Merged
merged 5 commits into from
Nov 21, 2023

Conversation

domenipavec
Copy link
Contributor

Older chromecasts (for example my Nvidia Shield TV) don't support secure /setup/eureka_info endpoint. That's why they don't get detected with known_hosts option in current implementation. There are several issues related to that (e.g. #611)

This PR does the following:

  • When secure=None in _get_status it first tries secure request, if that times out or returns an error, it falls back to insecure. This codepath is only used in get_device_info
  • In discovery HostBrowser propagates appropriate timeout to get_device_info, so there is enough time to try all specified known_hosts.

@emontnemery
Copy link
Collaborator

emontnemery commented Nov 21, 2023

@domenipavec can you confirm you have tested your changes both with your Nvidia Shield TV and with some device which does support secure /setup/eureka_info endpoint?
Also, can you please fix the lint errors so CI passes. I strongly recommend to run the linters locally since I have to approve CI since this is your first PR.

@domenipavec
Copy link
Contributor Author

@emontnemery Yes, this has been tested both with Nvidia Shield TV that uses insecure endpoint and with Google Home Mini and Chromecast with Google TV that use secure endpoint.

Lint errors should be fixed now.

pychromecast/dial.py Outdated Show resolved Hide resolved
Comment on lines 329 to 331
device_status = get_device_info(
host, timeout=self._timeout / len(known_hosts), context=self._context
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand the point of this change, _poll_hosts runs continuously from a worker thread, why does the timeout matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

discover_chromecasts waits for DISCOVER_TIMEOUT before returning chromecasts discovered. If that timeout is not divided between the hosts that need to be discovered, then no chromecasts are returned if the first takes a long time or times out.

Copy link
Collaborator

Choose a reason for hiding this comment

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

discover_chromecasts is really only useful for manual testing from console, and if that's what you're doing keeping track of chromecasts jumping to another IP-address seems moot.
Can you maybe explain your use case a bit further?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, please move this change to a separate PR, let's keep this PR about fixing known_hosts for older chromecasts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the timeout change.
I don't really have a use case for discover_chromecasts, I only used it for manual testing of the change because it's documented in readme.

pychromecast/dial.py Show resolved Hide resolved

cast_type = CAST_TYPE_CHROMECAST
display_supported = True
friendly_name = status.get("name", "Unknown Chromecast")
manufacturer = "Unknown manufacturer"
model_name = "Unknown model name"
multizone_supported = False
udn = None
udn = status.get("ssdp_udn", None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's up with this change?
What do you get back from your device when fetching /setup/eureka_info?params=device_info,name" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Response is different for insecure endpoint. This ensures that udn is correct in both cases.

Insecure endpoint:

{
  "bssid": "",
  "build_version": "374976",
  "cast_build_revision": "1.68.374976",
  "connected": true,
  "ethernet_connected": true,
  "has_update": false,
  "hotspot_bssid": "bssid",
  "ip_address": "ip",
  "locale": "en-US",
  "location": {
    "country_code": "US",
    "latitude": 255,
    "longitude": 255
  },
  "mac_address": "00:00:00:00:00:00",
  "name": "SHIELD",
  "opt_in": {
    "crash": false,
    "opencast": false,
    "stats": false
  },
  "public_key": "key",
  "release_track": "",
  "setup_state": 60,
  "setup_stats": {
    "historically_succeeded": true,
    "num_check_connectivity": 0,
    "num_connect_wifi": 0,
    "num_connected_wifi_not_saved": 0,
    "num_initial_eureka_info": 0,
    "num_obtain_ip": 0
  },
  "ssdp_udn": "udn",
  "ssid": "",
  "time_format": 1,
  "tos_accepted": true,
  "uptime": 5335.944937,
  "version": 12,
  "wpa_configured": false,
  "wpa_state": 0
}

Secure endpoint:

{
  "device_info": {
    "4k_blocked": 0,
    "capabilities": {
      "aogh_supported": true,
      "assistant_supported": true,
      "audio_hdr_supported": false,
      "audio_surround_mode_supported": false,
      "ble_supported": true,
      "bluetooth_audio_sink_supported": true,
      "bluetooth_audio_source_supported": true,
      "bluetooth_supported": true,
      "cloud_groups_supported": true,
      "cloudcast_supported": true,
      "content_filters_supported": true,
      "disable_google_dns_supported": true,
      "display_supported": false,
      "fdr_supported": false,
      "hdmi_prefer_50hz_supported": false,
      "hdmi_prefer_high_fps_supported": false,
      "hotspot_supported": true,
      "https_setup_supported": true,
      "input_management_supported": true,
      "keep_hotspot_until_connected_supported": true,
      "multi_user_supported": true,
      "multichannel_group_supported": true,
      "multizone_supported": true,
      "night_mode_supported": true,
      "night_mode_supported_v2": true,
      "opencast_supported": false,
      "preview_channel_supported": true,
      "reboot_supported": true,
      "remote_ducking_supported": true,
      "renaming_supported": true,
      "set_network_supported": true,
      "setup_supported": true,
      "sleep_mode_supported": true,
      "stats_supported": true,
      "system_sound_effects_supported": false,
      "ui_flipping_supported": true,
      "user_eq_supported": true,
      "wifi_auto_save_supported": true,
      "wifi_supported": true
    },
    "cloud_device_id": "id",
    "factory_country_code": "DE",
    "hotspot_bssid": "bssid",
    "local_authorization_token_hash": "hash",
    "mac_address": "mac",
    "manufacturer": "Google Inc.",
    "model_name": "Google Home Mini",
    "product_name": "mushroom",
    "public_key": "key",
    "ssdp_udn": "udn",
    "uma_client_id": "id",
    "uptime": 59169.998497,
    "weave_device_id": ""
  },
  "name": "Google Home"
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if you instead fetch /setup/eureka_info?options=detail on the Nvidia Shield device?

Copy link
Contributor Author

@domenipavec domenipavec Nov 21, 2023

Choose a reason for hiding this comment

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

I get the same output as with /setup/eureka_info?params=device_info,name

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I see.

Instead of doing udn = status.get("ssdp_udn", None) can you please move it to an else clause:

if "device_info" in status:
    ...
else:
    udn = status.get("ssdp_udn", None)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Collaborator

@emontnemery emontnemery left a comment

Choose a reason for hiding this comment

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

Thanks a lot @domenipavec 👍

@emontnemery emontnemery merged commit 358604f into home-assistant-libs:master Nov 21, 2023
1 check passed
@domenipavec
Copy link
Contributor Author

@emontnemery When can we expect a new release with this changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants