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

Start discovery with a parameter (like a pairing code in case of Matter) #4388

Closed
lolodomo opened this issue Sep 21, 2024 · 16 comments
Closed
Labels
enhancement An enhancement or new feature of the Core

Comments

@lolodomo
Copy link
Contributor

The Matter binding being developed by @digitaldan requires to fill a pairing code when scanning for a new device.
My feeling is that we need a new optional input field in Main UI to fill this parameter just before clicking the Scan button.

image

Main UI will then pass this parameter, if filled by user, to the REST API.

@digitaldan : please first confirm that it is THE solution.

If we go in that direction, I can do the work in core framework (enhancing REST API with a new parameter and handling this parameter in the discovery stuff), keeping a backward compatibility for all existing binding discovery (parameter will be ignored). But then we would need for your help @florian-h05 to update this screen. Are you volunteer for that?

@digitaldan : finally you will just have to override in your binding a new method void startScan(String scanParameter) in your discovery class.

WDYT ?

@lolodomo lolodomo added the enhancement An enhancement or new feature of the Core label Sep 21, 2024
@florian-h05
Copy link
Contributor

I'm in for the UI part. I guess Dan could do this as well but I think he is busy enough with the Matter binding itself.

By enhancing the REST API, does this include to provide the UI with the information whether a pairing code is required? Even better would be to also provide information about the pairing code format, e.g. a description and a RegEx for validation through the REST API. The binding could provide this through config descriptions to core.
This would make it as flexible and user friendly as possible.

@openhab-bot
Copy link
Collaborator

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/incorporating-matter/127907/140

@lolodomo
Copy link
Contributor Author

lolodomo commented Sep 21, 2024

By enhancing the REST API, does this include to provide the UI with the information whether a pairing code is required? Even better would be to also provide information about the pairing code format, e.g. a description and a RegEx for validation through the REST API. The binding could provide this through config descriptions to core.
This would make it as flexible and user friendly as possible.

That is a good idea but that makes the stuff more complex than I imagined.

This could be a new discovery GET API for a binding returning these information: GET discovery/bindingId

@florian-h05
Copy link
Contributor

Yeah it’s relatively complex on the core side, but I at least want the information whether to display such an input … I don’t want to display it always, because this would confuse the user and clutter the UI.

@digitaldan
Copy link
Contributor

I have not thought too much about it, but as far as the internal framework, i guess it would be nice if a binding advertised that it supported a discovery code through org.openhab.core.config.discovery.DiscoveryService, much like we have "isBackgroundDiscoveryEnabled" maybe "isAdditionalCodeEnabled" ? Then "startScan" could pass this code in as a optional parameter.

In the UI i could see when picking the binding type and scanning this being an option. It would also be great in the UI to allow the user to enter this code OR take a picture of of a QR code for the text (very common now).

@lolodomo
Copy link
Contributor Author

i guess it would be nice if a binding advertised that it supported a discovery code through org.openhab.core.config.discovery.DiscoveryService, much like we have "isBackgroundDiscoveryEnabled" maybe "isAdditionalCodeEnabled" ? Then "startScan" could pass this code in as a optional parameter.

Yes, good idea.

@florian-h05
Copy link
Contributor

florian-h05 commented Sep 24, 2024

Good idea Dan.

I think we should extend the DiscoveryService interface with a requiresPairingCode method with a default implementation that returns false (to avoid having to adjust too much binding code) and add an override for the startScan method to allow passing the pairing code.

Next the DiscoveryResource should be extended:

  • Add a new GET /bindings/{bindingId} endpoint that returns discovery information for the binding such as whether background discovery is enabled, whether a pairing code is required etc.
  • Extend the POST /bindings/{bindingId}/scan endpoint to add a path parameter pairingCode that can be passed to startScan

Finally, it would be really nice if a binding could provide a description for the pairing code so the UI could tell the user the required format.

@florian-h05
Copy link
Contributor

Please let me know what you think, especially any ideas wrt to the implementation of the description are highly welcomed!
I think I can take a look at implementing the core side as well as the UI side next week when I am back from vacation!

@lolodomo
Copy link
Contributor Author

I have almost finished the implementation in core framework. I will push something before the weekend.

@digitaldan
Copy link
Contributor

interface with a requiresPairingCode method with a default

I was actually staying away from "required" so that the discovery service of a binding could support both scanning with and without a code, but i agree this should be default false and not necessary for most bindings to handle.

@digitaldan
Copy link
Contributor

digitaldan commented Sep 24, 2024

Just FYI this will be very nice for the matter binding. I'm working on prototyping an IOS extension that will use the built in matter/thread framework in IOS 16 to detect new matter devices via bluetooth, then allow the user to add them to the Wifi or Thread network, then commission them to the Matter fabric. I'll end up using this new discovery feature for the Matter Fabric part, so once we get them onto the Wifi or Thread network, i can call the discovery API with the pairing code from the IOS client, and openHAB will take care of the rest !

@florian-h05
Copy link
Contributor

I was actually staying away from "required" so that the discovery service of a binding could support both scanning with and without a code

I wonder whether there is a binding (or better a „system“) that supports both cases …

Your iOS extension sounds super nice!

@cdjackson
Copy link
Contributor

I wonder whether there is a binding (or better a „system“) that supports both cases …

If I understand the question correctly, then the answer is yes. Zigbee can either use a device specific pairing code / key, or it can use a generic key. Most devices use the generic key, but the device specific code is becoming more popular. Both will be needed though for Zigbee.

@florian-h05
Copy link
Contributor

Thanks for the info, I think you understand the question right.
How is the generic key provided to the binding?

@lolodomo
Copy link
Contributor Author

My current implementation makes the input required or not. I will change it to make it optional. It is just a test to change and a method to rename.
I will push my current code this evening for review as I see a lot of interest and impatience ;)

lolodomo added a commit to lolodomo/openhab-core that referenced this issue Sep 25, 2024
lolodomo added a commit to lolodomo/openhab-core that referenced this issue Sep 26, 2024
lolodomo added a commit to lolodomo/openhab-core that referenced this issue Sep 26, 2024
lolodomo added a commit to lolodomo/openhab-core that referenced this issue Sep 28, 2024
@lolodomo
Copy link
Contributor Author

lolodomo commented Oct 1, 2024

Feature is now implemented

@lolodomo lolodomo closed this as completed Oct 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

No branches or pull requests

5 participants