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.

Sign up for GitHub

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

Jump to bottom

Add a new optional input parameter to discovery services #4389

Merged
merged 2 commits into from
Sep 29, 2024
Merged

Add a new optional input parameter to discovery services #4389

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2896560
Add a new optional input parameter to discovery services
lolodomo Sep 25, 2024
a80378e
Review comment: restore IllegalStateException
lolodomo Sep 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 67 additions & 5 deletions ...g.discovery/src/main/java/org/openhab/core/config/discovery/AbstractDiscoveryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@
* @author Kai Kreuzer - Refactored API
* @author Dennis Nobel - Added background discovery configuration through Configuration Admin
* @author Andre Fuechsel - Added removeOlderResults
* @author Laurent Garnier - Added discovery with an optional input parameter
*/
@NonNullByDefault
public abstract class AbstractDiscoveryService implements DiscoveryService {
Expand All @@ -64,6 +65,9 @@ public abstract class AbstractDiscoveryService implements DiscoveryService {

private boolean backgroundDiscoveryEnabled;

private @Nullable String scanInputLabel;
private @Nullable String scanInputDescription;

private final Map<ThingUID, DiscoveryResult> cachedResults = new HashMap<>();

private final Set<ThingTypeUID> supportedThingTypes;
Expand All @@ -84,20 +88,44 @@ public abstract class AbstractDiscoveryService implements DiscoveryService {
* service automatically stops its forced discovery process (>= 0).
* @param backgroundDiscoveryEnabledByDefault defines, whether the default for this discovery service is to
* enable background discovery or not.
* @param scanInputLabel the label of the optional input parameter to start the discovery or null if no input
* parameter supported
* @param scanInputDescription the description of the optional input parameter to start the discovery or null if no
* input parameter supported
* @throws IllegalArgumentException if {@code timeout < 0}
*/
protected AbstractDiscoveryService(@Nullable Set<ThingTypeUID> supportedThingTypes, int timeout,
boolean backgroundDiscoveryEnabledByDefault) throws IllegalArgumentException {
boolean backgroundDiscoveryEnabledByDefault, @Nullable String scanInputLabel,
@Nullable String scanInputDescription) throws IllegalArgumentException {
if (timeout < 0) {
throw new IllegalArgumentException("The timeout must be >= 0!");
}
this.supportedThingTypes = supportedThingTypes == null ? Set.of() : Set.copyOf(supportedThingTypes);
this.timeout = timeout;
this.backgroundDiscoveryEnabled = backgroundDiscoveryEnabledByDefault;
this.scanInputLabel = scanInputLabel;
this.scanInputDescription = scanInputDescription;
}

/**
* Creates a new instance of this class with the specified parameters and no input parameter supported to start the
* discovery.
*
* @param supportedThingTypes the list of Thing types which are supported (can be null)
* @param timeout the discovery timeout in seconds after which the discovery
* service automatically stops its forced discovery process (>= 0).
* @param backgroundDiscoveryEnabledByDefault defines, whether the default for this discovery service is to
* enable background discovery or not.
* @throws IllegalArgumentException if {@code timeout < 0}
*/
protected AbstractDiscoveryService(@Nullable Set<ThingTypeUID> supportedThingTypes, int timeout,
boolean backgroundDiscoveryEnabledByDefault) throws IllegalArgumentException {
this(supportedThingTypes, timeout, backgroundDiscoveryEnabledByDefault, null, null);
}

/**
* Creates a new instance of this class with the specified parameters and background discovery enabled.
* Creates a new instance of this class with the specified parameters and background discovery enabled
* and no input parameter supported to start the discovery.
*
* @param supportedThingTypes the list of Thing types which are supported (can be null)
* @param timeout the discovery timeout in seconds after which the discovery service
Expand All @@ -111,7 +139,8 @@ protected AbstractDiscoveryService(@Nullable Set<ThingTypeUID> supportedThingTyp
}

/**
* Creates a new instance of this class with the specified parameters and background discovery enabled.
* Creates a new instance of this class with the specified parameters and background discovery enabled
* and no input parameter supported to start the discovery.
*
* @param timeout the discovery timeout in seconds after which the discovery service
* automatically stops its forced discovery process (>= 0).
Expand All @@ -133,6 +162,21 @@ public Set<ThingTypeUID> getSupportedThingTypes() {
return supportedThingTypes;
}

@Override
public boolean isScanInputSupported() {
return scanInputLabel != null && scanInputDescription != null;
Copy link
Contributor

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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the difference?

Copy link
Contributor

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.

Copy link
Member

Choose a reason for hiding this comment

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

#4393 is merged

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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 ;)

}

@Override
public @Nullable String getScanInputLabel() {
return scanInputLabel;
}

@Override
public @Nullable String getScanInputDescription() {
return scanInputDescription;
}

/**
* Returns the amount of time in seconds after which the discovery service automatically
* stops its forced discovery process.
Expand Down Expand Up @@ -168,7 +212,16 @@ public void removeDiscoveryListener(@Nullable DiscoveryListener listener) {
}

@Override
public synchronized void startScan(@Nullable ScanListener listener) {
public void startScan(@Nullable ScanListener listener) {
startScanInternal(null, listener);
}

@Override
public void startScan(String input, @Nullable ScanListener listener) {
startScanInternal(input, listener);
}

private synchronized void startScanInternal(@Nullable String input, @Nullable ScanListener listener) {
synchronized (this) {
// we first stop any currently running scan and its scheduled stop
// call
Expand All @@ -194,7 +247,11 @@ public synchronized void startScan(@Nullable ScanListener listener) {
timestampOfLastScan = Instant.now();

try {
startScan();
if (isScanInputSupported() && input != null) {
startScan(input);
} else {
startScan();
}
} catch (Exception ex) {
scheduledStop = this.scheduledStop;
if (scheduledStop != null) {
Expand Down Expand Up @@ -232,6 +289,11 @@ public synchronized void abortScan() {
*/
protected abstract void startScan();

// An abstract method would have required a change in all existing bindings implementing a DiscoveryService
protected void startScan(String input) {
logger.warn("Discovery with input parameter not implemented by service '{}'!", this.getClass().getName());
}

/**
* This method cleans up after a scan, i.e. it removes listeners and other required operations.
*/
Expand Down
11 changes: 9 additions & 2 deletions ...src/main/java/org/openhab/core/config/discovery/AbstractThingHandlerDiscoveryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
* It handles the injection of the {@link ThingHandler}
*
* @author Jan N. Klug - Initial contribution
* @author Laurent Garnier - Added discovery with an optional input parameter
*/
@NonNullByDefault
public abstract class AbstractThingHandlerDiscoveryService<T extends ThingHandler> extends AbstractDiscoveryService
Expand All @@ -45,12 +46,18 @@ public abstract class AbstractThingHandlerDiscoveryService<T extends ThingHandle
protected @NonNullByDefault({}) T thingHandler = (@NonNull T) null;

protected AbstractThingHandlerDiscoveryService(Class<T> thingClazz, @Nullable Set<ThingTypeUID> supportedThingTypes,
int timeout, boolean backgroundDiscoveryEnabledByDefault) throws IllegalArgumentException {
super(supportedThingTypes, timeout, backgroundDiscoveryEnabledByDefault);
int timeout, boolean backgroundDiscoveryEnabledByDefault, @Nullable String scanInputLabel,
@Nullable String scanInputDescription) throws IllegalArgumentException {
super(supportedThingTypes, timeout, backgroundDiscoveryEnabledByDefault, scanInputLabel, scanInputDescription);
this.thingClazz = thingClazz;
this.backgroundDiscoveryEnabled = backgroundDiscoveryEnabledByDefault;
}

protected AbstractThingHandlerDiscoveryService(Class<T> thingClazz, @Nullable Set<ThingTypeUID> supportedThingTypes,
int timeout, boolean backgroundDiscoveryEnabledByDefault) throws IllegalArgumentException {
this(thingClazz, supportedThingTypes, timeout, backgroundDiscoveryEnabledByDefault, null, null);
}

protected AbstractThingHandlerDiscoveryService(Class<T> thingClazz, @Nullable Set<ThingTypeUID> supportedThingTypes,
int timeout) throws IllegalArgumentException {
this(thingClazz, supportedThingTypes, timeout, true);
Expand Down
40 changes: 40 additions & 0 deletions ...re.config.discovery/src/main/java/org/openhab/core/config/discovery/DiscoveryService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
* @author Michael Grammling - Initial contribution
* @author Kai Kreuzer - Refactored API
* @author Dennis Nobel - Added background discovery configuration through Configuration Admin
* @author Laurent Garnier - Added discovery with an optional input parameter
*
* @see DiscoveryListener
* @see DiscoveryServiceRegistry
Expand All @@ -60,6 +61,31 @@ public interface DiscoveryService {
*/
Collection<ThingTypeUID> getSupportedThingTypes();

/**
* Returns {@code true} if the discovery supports an optional input parameter to run, otherwise {@code false}.
*
* @return true if the discovery supports an optional input parameter to run, otherwise false
*/
boolean isScanInputSupported();

/**
* Returns the label of the supported input parameter to start the discovery.
*
* @return the label of the supported input parameter to start the discovery or null if input parameter not
* supported
*/
@Nullable
String getScanInputLabel();

/**
* Returns the description of the supported input parameter to start the discovery.
*
* @return the description of the supported input parameter to start the discovery or null if input parameter not
* supported
*/
@Nullable
String getScanInputDescription();

/**
* Returns the amount of time in seconds after which an active scan ends.
*
Expand Down Expand Up @@ -87,6 +113,20 @@ public interface DiscoveryService {
*/
void startScan(@Nullable ScanListener listener);

/**
* Triggers this service to start an active scan for new devices using an input parameter for that.<br>
* This method must not block any calls such as {@link #abortScan()} and
* must return fast.
* <p>
* If started, any registered {@link DiscoveryListener} must be notified about {@link DiscoveryResult}s.
* <p>
* If there is already a scan running, it is aborted and a new scan is triggered.
*
* @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 startScan(String input, @Nullable ScanListener listener);

/**
* Stops an active scan for devices.<br>
* This method must not block any calls such as {@link #startScan} and must
Expand Down
15 changes: 13 additions & 2 deletions ...g.discovery/src/main/java/org/openhab/core/config/discovery/DiscoveryServiceRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
package org.openhab.core.config.discovery;

import java.util.List;
import java.util.Set;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
Expand All @@ -29,6 +30,7 @@
*
* @author Michael Grammling - Initial contribution
* @author Ivaylo Ivanov - Added getMaxScanTimeout
* @author Laurent Garnier - Added discovery with an optional input parameter
*
* @see DiscoveryService
* @see DiscoveryListener
Expand All @@ -44,6 +46,7 @@ public interface DiscoveryServiceRegistry {
*
* @param thingTypeUID the Thing type UID pointing to collection of discovery
* services to be forced to start a discovery
* @param input an optional input parameter to be used during discovery scan, can be null.
* @param listener a callback to inform about errors or termination, can be null.
* If more than one discovery service is started, the {@link ScanListener#onFinished()} callback is
* called after all
Expand All @@ -54,7 +57,7 @@ public interface DiscoveryServiceRegistry {
* @return true if a t least one discovery service could be found and forced
* to start a discovery, otherwise false
*/
boolean startScan(ThingTypeUID thingTypeUID, @Nullable ScanListener listener);
boolean startScan(ThingTypeUID thingTypeUID, @Nullable String input, @Nullable ScanListener listener);

/**
* Forces the associated {@link DiscoveryService}s to start a discovery for
Expand All @@ -65,6 +68,7 @@ public interface DiscoveryServiceRegistry {
*
* @param bindingId the binding id pointing to one or more discovery services to
* be forced to start a discovery
* @param input an optional input parameter to be used during discovery scan, can be null.
* @param listener a callback to inform about errors or termination, can be null.
* If more than one discovery service is started, the {@link ScanListener#onFinished()} callback is
* called after all
Expand All @@ -75,7 +79,7 @@ public interface DiscoveryServiceRegistry {
* @return true if a t least one discovery service could be found and forced
* to start a discovery, otherwise false
*/
boolean startScan(String bindingId, @Nullable ScanListener listener);
boolean startScan(String bindingId, @Nullable String input, @Nullable ScanListener listener);

/**
* Aborts a started discovery on all {@link DiscoveryService}s for the given
Expand Down Expand Up @@ -163,6 +167,13 @@ public interface DiscoveryServiceRegistry {
*/
List<String> getSupportedBindings();

/**
* Returns the list of all {@link DiscoveryService}s, that discover thing types of the given binding id.
*
* @return list of discovery services, that discover thing types of the given binding id
*/
Set<DiscoveryService> getDiscoveryServices(String bindingId) throws IllegalStateException;

/**
* Returns the maximum discovery timeout from all discovery services registered for the specified thingTypeUID
*
Expand Down
30 changes: 20 additions & 10 deletions ...rc/main/java/org/openhab/core/config/discovery/internal/DiscoveryServiceRegistryImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
* @author Kai Kreuzer - Refactored API
* @author Andre Fuechsel - Added removeOlderResults
* @author Ivaylo Ivanov - Added getMaxScanTimeout
* @author Laurent Garnier - Added discovery with an optional input parameter
*
* @see DiscoveryServiceRegistry
* @see DiscoveryListener
Expand Down Expand Up @@ -189,27 +190,29 @@ public void addDiscoveryListener(DiscoveryListener listener) throws IllegalState
}

@Override
public boolean startScan(ThingTypeUID thingTypeUID, @Nullable ScanListener listener) throws IllegalStateException {
public boolean startScan(ThingTypeUID thingTypeUID, @Nullable String input, @Nullable ScanListener listener)
throws IllegalStateException {
Set<DiscoveryService> discoveryServicesForThingType = getDiscoveryServices(thingTypeUID);

if (discoveryServicesForThingType.isEmpty()) {
logger.warn("No discovery service for thing type '{}' found!", thingTypeUID);
return false;
}

return startScans(discoveryServicesForThingType, listener);
return startScans(discoveryServicesForThingType, input, listener);
}

@Override
public boolean startScan(String bindingId, final @Nullable ScanListener listener) throws IllegalStateException {
public boolean startScan(String bindingId, @Nullable String input, @Nullable ScanListener listener)
throws IllegalStateException {
final Set<DiscoveryService> discoveryServicesForBinding = getDiscoveryServices(bindingId);

if (discoveryServicesForBinding.isEmpty()) {
logger.warn("No discovery service for binding id '{}' found!", bindingId);
return false;
}

return startScans(discoveryServicesForBinding, listener);
return startScans(discoveryServicesForBinding, input, listener);
}

@Override
Expand Down Expand Up @@ -326,15 +329,16 @@ private boolean abortScans(Set<DiscoveryService> discoveryServices) {
return allServicesAborted;
}

private boolean startScans(Set<DiscoveryService> discoveryServices, @Nullable ScanListener listener) {
private boolean startScans(Set<DiscoveryService> discoveryServices, @Nullable String input,
@Nullable ScanListener listener) {
boolean atLeastOneDiscoveryServiceHasBeenStarted = false;

if (discoveryServices.size() > 1) {
logger.debug("Trying to start {} scans with an aggregating listener.", discoveryServices.size());
AggregatingScanListener aggregatingScanListener = new AggregatingScanListener(discoveryServices.size(),
listener);
for (DiscoveryService discoveryService : discoveryServices) {
if (startScan(discoveryService, aggregatingScanListener)) {
if (startScan(discoveryService, input, aggregatingScanListener)) {
atLeastOneDiscoveryServiceHasBeenStarted = true;
} else {
logger.debug(
Expand All @@ -343,21 +347,26 @@ private boolean startScans(Set<DiscoveryService> discoveryServices, @Nullable Sc
}
}
} else {
if (startScan(discoveryServices.iterator().next(), listener)) {
if (startScan(discoveryServices.iterator().next(), input, listener)) {
atLeastOneDiscoveryServiceHasBeenStarted = true;
}
}

return atLeastOneDiscoveryServiceHasBeenStarted;
}

private boolean startScan(DiscoveryService discoveryService, @Nullable ScanListener listener) {
private boolean startScan(DiscoveryService discoveryService, @Nullable String input,
@Nullable ScanListener listener) {
Collection<ThingTypeUID> supportedThingTypes = discoveryService.getSupportedThingTypes();
try {
logger.debug("Triggering scan for thing types '{}' on '{}'...", supportedThingTypes,
discoveryService.getClass().getSimpleName());

discoveryService.startScan(listener);
if (discoveryService.isScanInputSupported() && input != null) {
discoveryService.startScan(input, listener);
} else {
discoveryService.startScan(listener);
}
return true;
} catch (Exception ex) {
logger.error("Cannot trigger scan for thing types '{}' on '{}'!", supportedThingTypes,
Expand All @@ -380,7 +389,8 @@ private synchronized Set<DiscoveryService> getDiscoveryServices(ThingTypeUID thi
return discoveryServices;
}

private synchronized Set<DiscoveryService> getDiscoveryServices(String bindingId) throws IllegalStateException {
@Override
public synchronized Set<DiscoveryService> getDiscoveryServices(String bindingId) throws IllegalStateException {
Set<DiscoveryService> discoveryServices = new HashSet<>();

for (DiscoveryService discoveryService : this.discoveryServices) {
Expand Down
Loading