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

Remove Thread credential sync from start of Matter commissioning flow #4150

Merged

Conversation

jpelgrom
Copy link
Member

Summary

After discussion with @agners, remove Thread credential syncing from the start of the Matter commissioning flow. There are 2 major reasons for this:

  • it significantly slows down the flow while only being useful some of the time (initially, and on Thread network/BR changes)
  • the results have limited usefulness because the API limits how much Home Assistant can influence the device data

The alternative for the user is to use the manual 'import credentials' button in the frontend (matching iOS), or access the sync option in the app settings > troubleshooting menu.

This should result in a more reliable user experience when using Matter and prevents unstable/unfinished Thread work on the core and app side 'polluting a real device'. Adding a comment in the code in case/when we revisit this in the future.

Screenshots

n/a / the Thread permission dialog will no longer show up when doing Matter commissioning

Link to pull request in Documentation repository

n/a, this behavior wasn't documented, general Thread companion app import/export documentation to be done independently

Any other notes

 - After discussion with Thread dev, remove Thread credential syncing from the start of the Matter commissioning flow. There are 2 major reasons for this:
   * it significantly slows down the flow while only being useful some of the time
   * the results have limited usefulness because the API limits how much Home Assistant can influence the device data
  The alternative for the user is to use the manual 'import credentials' button in the frontend (matching iOS), or access the sync option in the app settings > troubleshooting menu.
Copy link
Member

@agners agners left a comment

Choose a reason for hiding this comment

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

As it is now, this only really would be helpful for the very first time, if the user has indeed a Home Assistant controlled OTBR, and if he was indeed triggering the flow from within the Home Assistant Android App and has no Google Thread border router yet. For all other use-cases this currently just adds lot of waiting time.

@jpelgrom
Copy link
Member Author

I'd argue that there is still some value if you do not have OTBR but do have Google border routers, as this would get their dataset into Home Assistant (where you can't really use it but maybe for futureproofing?).

@agners
Copy link
Member

agners commented Jan 22, 2024

I'd argue that there is still some value if you do not have OTBR but do have Google border routers, as this would get their dataset into Home Assistant (where you can't really use it but maybe for futureproofing?).

I think both direction can be useful, it is just that very few use cases are actually benefit from it. Even when there is no Thread network on a phone, importing only seems to work once (at least in my test case, see #4146).

When a user has no TBR and uses a HA OTBR, then he probably wants to import, but even that is not a 100% given, especially since the limitations on Android side means he will be stuck with that setup (and we decided for him 😅 ).

I think in the current situation, and since the sync is rather slow, it is just better to only do this manually. We can revisit an automatic mechanism later.

@JBassett JBassett merged commit d9e65f8 into home-assistant:master Jan 24, 2024
4 checks passed
@jpelgrom jpelgrom deleted the disable-thread-sync-on-matter branch January 24, 2024 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants