Abort apple_tv config flows when device is a Mac

[bdraco]

Proposed change

Trying to setup a Mac currently fails with invalid authentication

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 Authenticatiton problem with Macbook #94815
• 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
• 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]

Hey there @postlund, mind taking a look at this pull request as it has been labeled with an integration (apple_tv) you are listed as a code owner for? Thanks!

Code owner commands

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

[bdraco]

This works as expected as my MBA and MBP no longer show up.

Will add tests for this if this is how we want to handle this

[balloob]

I feel like this should be fixed upstream, unless upstream is able to handle device with authentication.

Adding approval to this PR in case upstream is not fixed in time for the beta, as we should not show these to the users.

[bdraco]

I'm hoping its fixed upstream in time for beta but this will be a good stop gap if not. I'll move it forward on Tuesday if we don't have an upstream bump by than

[postlund]

I have fixed so that Macs are marked as unsupported in pyatv, will be part of next release (I'll try to get it out today or tomorrow). There's one thing to note though, that likely needs fixing. In case a device (be it a Macbook or something) is discovered, with no services "pairable", then the device will be discovered but immediately present an error dialog when trying to add it. Ideally the device should not end up as discovered if pyatv cannot interact with it in any way. I added a helper method (pyatv.helpers.is_device_supported) that can be used to check this, but it needs to be integrated into the config flow and raise some kind of exception I guess (AbortFlow?). Is that possible?

[bdraco]

AbortFlow

Raising AbortFlow will do it

homeassistant/components/apple_tv/config_flow.py:            raise AbortFlow("already_in_progress")
homeassistant/components/apple_tv/config_flow.py:            raise AbortFlow("device_not_supported")

[bdraco]

will need to be adjusted after #95388

[bdraco]

My MBP isn't showing up anymore after #95388 so I'm not sure this PR is needed anymore ?

[frenck]

Doesn't show up here anymore either after the bump 👍

[postlund]

That is interesting. I cannot really explain that behavior. I would believe that it sho up, but fail when adding it. Strange...

[frenck]

Re-opening this one, to discuss it as well. After upgrading the nightly, it seems to have returned. So, my report above was most likely a fluke on my dev env.

[postlund]

When adding it, do you get the prompt about having to enter several PIN codes and then it just fails immediately when pressuring submit?

[postlund]

When adding it, do you get the prompt about having to enter several PIN codes and then it just fails immediately when pressuring submit?

[bdraco]

For my air it aborts with invalid auth immediately

For my pro it tries to get me to enter a PIN code but nothing appears to enter it.

[bdraco]

We need to abort sooner vs this solution since it will keep scanning otherwise.

I'll rework this to look at the model / am

[bdraco]

still need to fix the mocking to have a valid unique id

[bdraco]

going to test this on production instead of dev this time to make sure they go away

[bdraco]

All good now.

We can revert this change later once everything is sorted out in the lib

[postlund]

I have a better suggestion, will let you know after dinner.

[frenck]

I have a better suggestion, will let you know after dinner.

Drafting.

[balloob]

Besides filtering out macbooks, I also want to filter out Sonos. The Sonos native API works locally and will always be preferred over the Airplay API. We should abort the discoveries.

Looking at DeviceInfo, I notice that we don't track manufacturer, which makes filtering out all Sonos devices more complicated to achieve.

[bdraco]

I currently group my home pods with my sonos with airplay. While I don't do currently do it through hass ( I was thinking about doing that ), I think there would be a use case to group different brands of speakers using airplay.

I'm not sure thats a valid use care or something we would want to support ?

[postlund]

I agree with @balloob in most cases, but I also see that use case @bdraco as valid too. Can't really tell what is best here.

My suggestion for this is something along the lines of:

from pyatv.helpers import is_device_supported

# in async_find_device after device_scan

if not is_device_supported(self.atv):
    return self.async_abort(reason="device_not_supported")

This can be added permanently as that hands the responsibility over to pyatv for determining supported devices.

[bdraco]

I agree with @balloob in most cases, but I also see that use case @bdraco as valid too. Can't really tell what is best here.

My suggestion for this is something along the lines of:

from pyatv.helpers import is_device_supported

# in async_find_device after device_scan

if not is_device_supported(self.atv):
    return self.async_abort(reason="device_not_supported")

This can be added permanently as that hands the responsibility over to pyatv for determining supported devices.

Doesn't that mean we have to scan it before we can tell so every time zeroconf discovers it again its going to start a new scan in the background for a device that will never be supported?

[postlund]

Doesn't that mean we have to scan it before we can tell so every time zeroconf discovers it again its going to start a new scan in the background for a device that will never be supported?

That is a good point, yes. That would be the case. Maybe filtering is a better choice in this case.

[bdraco]

I think we can do both. It's best to abort as soon as possible though

[robloh]

This is a little off topic, but since discovery is discussed, is it time to rename this integration to better cover what it supports? Or at least add a virtual brand (I think it's called?) for HomePod. When I search for integrations on the website or within HASS Itself HomePod is not a thing, only Apple TV is. Apple calls the category "TV & Home" btw.

[bdraco]

I propose we move this forward as-is and add the is_device_supported in a followup PR (in addition)

[frenck]

I've opened up PR #95843 to revert the filters that cause all of this right now. I want to get it out of the scope for the 2023.7 release at this point.

Some more information can be found in #95843; one that is merged, we can remove this PR from the milestone.

../Frenck

[frenck]

Ok, merged #95843, remove this PR from the milestone.

Please let me be clear on not wanting to stall or dismiss any of the work done on this all the past month, but we do have to guard the user experience; if we would have released it in the state it was currently in, it would have caused a lot of confusion with the end-users why their macs, Sonos devices, Sony bravia TVs and what not would have been found by the "Apple TV" integration.

Let's make sure we won't be causing that confusion before we fully unlock the zeroconf filters.

../Frenck

[postlund]

I agree with you @frenck. It would just confuse users and create a lot of new unnecessary issues. So let's try to deal with all special cases (ignoring sonos and whatnot), and release it in a better state next release.

[michalmo]

Just want to chime in with some extra context that might be useful here, since @balloob mentioned filtering out Sonos.

I have the same use case of grouping Sonos and Apple devices through AirPlay as @bdraco.

I created PR #94704 which adds support for grouping devices. HomePods and Apple TVs can act as group controllers, but other AirPlay devices can be added to the group to sync playback to what is playing on the controller.

In my case I use this to automate grouping my Sonos speakers in the living room to a HomePod whenever the HomePod starts playing, to get full room sound. I have the Sonos speakers integrated through both the Sonos integration, and the Apple TV integration (with that PR installed locally), and it works very well.

If Sonos were to be excluded, then the alternative would be to make Sonos (and other AirPlay enabled speaker integrations) aware of AirPlay grouping (with extra complexity from conflicts with Sonos integrations own grouping).

[postlund]

Do note that the filtering is only for discovery, you can add devices excluded by filters manually via IP (or name) instead. You just have to know that of course.

[postlund]

Maybe it would be beneficial to have a filter in the integrations page that shows all integrations that have been excluded by filters (like with ignored integrations for instance)? Would naturally be an "advanced" setting.

[github-actions]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.