-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
Add a new optional input parameter to discovery services #4389
Conversation
* @param input an input parameter to be used during discovery scan | ||
* @param listener a listener that is notified about errors or termination of the scan | ||
*/ | ||
void startScanWithInput(String input, @Nullable ScanListener listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that I added a new method rather than adding a new parameter to the existing method startScan
because this method is called by few bindings.
But it is also possible to only update the existing method and prepare a fix for existing bindings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason not to overload the startScan method ?
void startScan(@Nullable ScanListener listener);
void startScan(String input, @Nullable ScanListener listener);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, method renamed.
There is still the method void startScanWithInput(String input)
, the main method that will be overriden in a binding. Before renaming it into startScan, I have to check if we have bindings declaring such method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ find */src -name "*.java" -exec grep -H "void startScan(" {} \; | grep -v "void startScan()"
org.openhab.binding.russound/src/main/java/org/openhab/binding/russound/internal/rio/system/RioSystemHandler.java: private void startScan(RioSystemConfig sysConfig) {
org.openhab.binding.webthing/src/main/java/org/openhab/binding/webthing/internal/discovery/WebthingDiscoveryService.java: private void startScan(boolean isBackground) {
Ok, there should be no risk I believe. I will rename also the last remaining startScanWithInput
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Every occurrence of startScanWithInput
is now renamed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To come back to my initial comment, in case you would prefer to not add a new method but rather add a parameter to the existing method, I checked how many bindings should be updated. Only five: easee, gardena, homematic, hue and lutron. So not a big deal. Let me know what you prefer.
$ find */src -name "*.java" -exec grep -H "startScan(" {} \; | grep -v "startScan()"
org.openhab.binding.easee/src/main/java/org/openhab/binding/easee/internal/handler/EaseeSiteHandler.java: discoveryService.startScan(null);
org.openhab.binding.gardena/src/main/java/org/openhab/binding/gardena/internal/handler/GardenaAccountHandler.java: discoveryService.startScan(null);
org.openhab.binding.homematic/src/main/java/org/openhab/binding/homematic/internal/handler/HomematicBridgeHandler.java: discoveryService.startScan(null);
org.openhab.binding.hue/src/main/java/org/openhab/binding/hue/internal/handler/Clip2BridgeHandler.java: discoveryService.startScan(null);
org.openhab.binding.lutron/src/main/java/org/openhab/binding/lutron/internal/handler/IPBridgeHandler.java: discoveryService.startScan(null);
org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/discovery/internal/ModbusDiscoveryService.java: scanStarted |= service.startScan(this);
org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/discovery/internal/ModbusEndpointDiscoveryService.java: public boolean startScan(ModbusDiscoveryService service) {
org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/discovery/internal/ModbusThingHandlerDiscoveryService.java: boolean startScan(ModbusDiscoveryService service);
org.openhab.binding.russound/src/main/java/org/openhab/binding/russound/internal/rio/system/RioSystemHandler.java: startScan(rioConfig);
org.openhab.binding.russound/src/main/java/org/openhab/binding/russound/internal/rio/system/RioSystemHandler.java: * {@link #startScan(RioSystemConfig)} when a device should be scanned and 'things' discovered from it
org.openhab.binding.russound/src/main/java/org/openhab/binding/russound/internal/rio/system/RioSystemHandler.java: private void startScan(RioSystemConfig sysConfig) {
org.openhab.binding.webthing/src/main/java/org/openhab/binding/webthing/internal/discovery/WebthingDiscoveryService.java: startScan(true);
org.openhab.binding.webthing/src/main/java/org/openhab/binding/webthing/internal/discovery/WebthingDiscoveryService.java: private void startScan(boolean isBackground) {
org.openhab.binding.webthing/src/main/java/org/openhab/binding/webthing/internal/discovery/WebthingDiscoveryService.java: startScan(false);
I tested it with success with a modified version of the astro binding. @digitaldan : what you have to do for matter binding in your discovery service is calling the constructor of
|
This is almost finished, I have in mind to add unit tests for the discovery REST API. I will add them later this week. |
For your information, I was able to build the full addons repo with these core changes (my build was done without running tests). |
Thanks @lolodomo , i am going to take a look at this tomorrow! |
f3a9d02
to
390b80a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
The new API endpoint provides everything that is required I think 👍
return JSONResponse.createResponse(Status.NOT_FOUND, null, | ||
"No discovery service found for binding " + bindingId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure if I should rather return Response.status(Status.NOT_FOUND).build()
I find the two approaches used in core framework without understanding which one is the most adapted.
Using JSONResponse.createResponse
allows returning a JSON with an error message in addition to the simple HTTP status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer your current approach because it provides the additional info, which is more user-friendly, but TBH I don’t know if anyone really „reads“ that response.
Related to openhab#4388 Signed-off-by: Laurent Garnier <lg.hc@free.fr>
390b80a
to
2896560
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, LGTM!
...y/src/main/java/org/openhab/core/config/discovery/internal/DiscoveryServiceRegistryImpl.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Laurent Garnier <lg.hc@free.fr>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@digitaldan shall I wait for your feedback or can I merge this?
Thanks @lolodomo looks great, @holgerfriedrich LGTM 👍 |
@florian-h05: you can now implement the UI part ;) |
@@ -133,6 +162,21 @@ public Set<ThingTypeUID> getSupportedThingTypes() { | |||
return supportedThingTypes; | |||
} | |||
|
|||
@Override | |||
public boolean isScanInputSupported() { | |||
return scanInputLabel != null && scanInputDescription != null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lolodomo
I think the goal of this implementation was to automatically return true if label and description are provided.
This however does not work at the moment, you rather need this:
return getScanInputLabel() != null && getScanInputDescription() != null;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using the methods instead of the classes private fields, isScanInputSupported
will return true if both the label and the description methods are overwritten in a child class. With the current implementation, this is not the case, thus requiring a child class that already provides label and description trough the methods to also overwrite the isScanInputSupported
method.
I will create a PR to fix that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#4393 is merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For your information, the idea is absolutely not to override getScanInputLabel and getScanInputDescription in a binding, but only to provide label and description in the constructor of the binding discovery service.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense ... sorry for the chaos.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By the way, your change is not a problem ;)
Refs openhab/openhab-core#4389. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
UI part is implemented: openhab/openhab-webui#2771 |
Refs openhab/openhab-core#4389. Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
…covery services Fixes an issue from openhab#4389. For more details see openhab#4389 (comment). Signed-off-by: Florian Hotze <florianh_dev@icloud.com>
Regression of openhab/openhab-core#4389 Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Regression of openhab/openhab-core#4389 Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Regression of openhab/openhab-core#4389 Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Regression of openhab/openhab-core#4389 Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
Related to #4388
Signed-off-by: Laurent Garnier lg.hc@free.fr