[Core] Introduced discovery for USB devices with serial port #5315

hsudbrock · 2018-03-26T10:23:56Z

This PR targets #5153, i.e., a discovery for things that provide functionality of USB dongles, and where the binding uses a serial port to communicate with the USB dongle. Examples for such dongles are Zigbee, Z-Wave, HomeMatic, or HomeMatic IP USB dongles.

As discussed in #5153, there are different options to discover USB dongles (e.g., using a native wrapper around usblib, or, for Linux systems, scanning the sysfs, accessing dbus, or using a native wrapper around udev), as well as different options to discover the corresponding serial ports (for Linux systems, scanning the sysfs, accessing dbus, or using a native wrapper around udev).

To allow for different implementations for discovering USB devices and serial ports, this PR contains, firstly, a generic discovery for USB devices that can be accessed via a serial port (cf. bundle org.eclipse.smarthome.config.discovery.usbserial). This discovery can be extended from two sides: On the one side, bindings can contribute implementations of the interface UsbSerialDiscoveryParticipant, which, given information about serial port and USB device provided by the discovery, generate a DiscoveryResult in case they support the USB device. On the other side, different implementations of the actual scanning for USB devices and serial ports can be provided by implementing the interface UsbSerialDiscovery.

Secondly, this PR contains an implementation of the actual discovery (i.e., an implementation of the interface UsbSerialDiscovery) that can be used on Linux systems (cf. bundle org.eclipse.smarthome.config.discovery.usbserial.linux.sysfs). This implementation regularly scans the 'sysfs', a Linux pseudo file system that provides information about the Linux system, including information about serial ports and about USB devices. (In #5153, I suggested to scan the sysfs for devices provided by the Linux usbserial driver; note that the actual implementation differs here by looking also for serial ports provided by USB devices that use another driver. This extends the set of discoverable USB dongles; an example here is the Aeotec Z-Stick for Z-Wave that uses the cdc-acm driver, and not the usbserial driver.)

I am not quite sure whether the bundle for the Linux discovery should actually go into the bundles subfolder, or whether this should rather go into the extensions subfolder of the Eclipse SmartHome project (or even somewhere else) - feedback here would be helpful for me. Of course, I would also be happy about any other feedback on this PR.

To illustrate briefly how a discovery participant for this USB-serial discovery could look like, the following code snippet illustrates this at the example of the Zigbee binding and two USB dongles supported as bridge by the Zigbee binding (a Telegesis USB dongle, and an Ember USB dongle) (this is not part of this PR; this would be part of a follow-up PR for the Zigbee binding):

@Component
public class ZigbeeDiscoveryParticipant implements UsbSerialDiscoveryParticipant {

    private static final String BINDING_ID = "zigbee";
    private static final ThingTypeUID THING_TYPE_TELEGESIS = new ThingTypeUID(BINDING_ID, "coordinator_telegesis");
    private static final ThingTypeUID THING_TYPE_EMBER = new ThingTypeUID(BINDING_ID, "coordinator_ember");

    @Override
    public @NonNull Set<@NonNull ThingTypeUID> getSupportedThingTypeUIDs() {
        return Sets.newHashSet(THING_TYPE_TELEGESIS, THING_TYPE_EMBER);
    }

    @Override
    public @Nullable DiscoveryResult createResult(@NonNull UsbSerialDeviceInformation deviceInformation) {
        if (isTelegesisDongle(deviceInformation)) {
            return DiscoveryResultBuilder
                    .create(new ThingUID(THING_TYPE_TELEGESIS, deviceInformation.getSerialNumber()))
                    .withLabel(deviceInformation.getProduct()).withRepresentationProperty("zigbee_port")
                    .withProperty("zigbee_port", deviceInformation.getSerialPort()).withProperty("zigbee_baud", 19200)
                    .build();
        } else if (isEmberDongle(deviceInformation)) {
            return DiscoveryResultBuilder.create(new ThingUID(THING_TYPE_EMBER, deviceInformation.getSerialNumber()))
                    .withLabel(deviceInformation.getProduct()).withRepresentationProperty("zigbee_port")
                    .withProperty("zigbee_port", deviceInformation.getSerialPort()).withProperty("zigbee_baud", 57600)
                    .build();
        } else {
            return null;
        }
    }

    @Override
    public @Nullable ThingUID getThingUID(@NonNull UsbSerialDeviceInformation deviceInformation) {
        if (isTelegesisDongle(deviceInformation)) {
            return new ThingUID(THING_TYPE_TELEGESIS, deviceInformation.getSerialNumber());
        } else if (isEmberDongle(deviceInformation)) {
            return new ThingUID(THING_TYPE_EMBER, deviceInformation.getSerialNumber());
        } else {
            return null;
        }
    }

    private boolean isTelegesisDongle(UsbSerialDeviceInformation deviceInformation) {
        return deviceInformation.getVendorId() == 0x10c4 && deviceInformation.getProductId() == 0x8293;
    }

    private boolean isEmberDongle(UsbSerialDeviceInformation deviceInformation) {
        return deviceInformation.getVendorId() == 0x10c4 && deviceInformation.getProductId() == 0x8b34;
    }

}

This commit contains an API for a generic discovery for USB devices that can be accessed via a serial port (typically provided by an operating system-specific driver). Examples of such things that could be discovered are bridges for wireless protocols (like, e.g., Zigbee, Z-Wave, HomeMatic, or HomeMatic IP) using an USB dongle, where the binding uses serial communication to communicate with the dongle. This discovery needs to be complemented from two sides: On the one side, bindings must contribute 'DiscoveryParticipants', which, given information about serial port and USB device provided by the discovery, generate a DiscoveryResult in case they support the USB device. On the other side, an implementation of the actual scanning for USB devices and serial ports must be provided. There are different options for providing such a discovery, as discussed on eclipse-archived#5153. This commit contains an implementation of the actual discovery that can be used on Linux systems, which regularly scans the so-called 'sysfs', a Linux pseudo file system that provides information about devices. Bug: eclipse-archived#5153 Signed-off-by: Henning Sudbrock <[email protected]>

amitjoy · 2018-03-26T10:47:08Z

.../org/eclipse/smarthome/config/discovery/usbserial/linux/sysfs/internal/UsbSerialScanner.java

+     *             identification of a single USB device might be swallowed, simply skipping that device while still
+     *             discovering other devices successfully.
+     */
+    Set<UsbSerialDeviceInformation> scan() throws Exception;


I believe it would be really helpful for consumers to handle a specific exception than a generic one. We could therefore introduce a custom checked exception.

@amitjoy The reason I chose to throw an Exception here (and not a custom exception) is that this interface is only used internally (it is in an internal package, and only used by classes in this internal package). Hence, there will be no other consumers of this interface.

Do you think that a specific exception should be thrown here nevertheless?

It should not depend if it is used internally only or externally.
Shouldn't an I/O exception fit your needs?

For the single implementation of UsbSerialScanner (the SysfsUsbSerialScanner) an IOException is indeed the suitable exception. Thanks for that suggestion, @maggu2810.

I will change the code accordingly.

In case there will be other implementations of UsbSerialScanner in the future that need other exception types, we could still think about a custom UsbSerialScanningException then...

hsudbrock · 2018-03-26T11:23:33Z

CC @cdjackson (as I believe that you might be interested in this PR - and of course might be able to provide very valuable feedback)

amitjoy · 2018-03-26T17:15:13Z

...eclipse/smarthome/config/discovery/usbserial/linux/sysfs/internal/SysfsUsbSerialScanner.java

+    @Modified
+    protected void modified(Map<String, Object> config) {
+        if (config.containsKey(SYSFS_TTY_DEVICES_DIRECTORY_ATTRIBUTE)) {
+            sysfsTtyDevicesDirectory = ObjectUtils.toString(config.get(SYSFS_TTY_DEVICES_DIRECTORY_ATTRIBUTE));


sysfsTtyDevicesDirectory is set twice if SYSFS_TTY_DEVICES_DIRECTORY_ATTRIBUTE exists in the configuration. You can also consider using

config.getOrDefault(SYSFS_TTY_DEVICES_DIRECTORY_ATTRIBUTE, SYSFS_TTY_DEVICES_DIRECTORY_DEFAULT);

As just discussed with @amitjoy: there should not be a problem with setting sysfsTtyDevicesDirectory twice; however, the configuration should not only be set in the modified method, but also on activation of the component. I will add this to the code. Thanks for the discussion, @amitjoy .

amitjoy · 2018-03-26T17:15:26Z

...eclipse/smarthome/config/discovery/usbserial/linux/sysfs/internal/SysfsUsbSerialScanner.java

+        }
+
+        if (config.containsKey(DEV_DIRECTORY_ATTRIBUTE)) {
+            devDirectory = ObjectUtils.toString(config.get(DEV_DIRECTORY_ATTRIBUTE));


Similarly here

Same as for comment above.

amitjoy · 2018-03-26T17:34:59Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

+        }
+    }
+
+    protected void unsetUsbSerialDiscovery(UsbSerialDiscovery usbSerialDiscovery) {


Dynamic service reference is a bit tricky in OSGi since no strict ordering of execution is followed. If you have two implementations of UsbSerialDiscovery (let's say A and B), binding callbacks can be called before the unbinding callbacks. That is, the order could be set(A) set(B) unset(A) and when unset is executed the this.usbSerialDiscovery will be null even though it is not expected since the field should only be null, if the injected instance (in this scenario, B) is deinjected.

It makes even situation worse, if multiple threads start invoking the callbacks together. Since OSGi as a container doesn't restrict any ordering for dynamic service references, the proper synchronization needs to be introduced by the consumer.

I personally use the following pattern (but not necessarily, you have to. You got the idea and you can use any suitable functionality that suffices your need. For example, AtomicReference is also a very good fit in this scenario.)

private volatile IA a; protected (synchronized) void setA(IA a) { .... } protected (synchronized) void unsetA(IA a) { if (this.a == a) { .... this.a = null; } }

Thanks @amitjoy for this hint. I like the pattern that you suggest, but I think that it needs to be augmented a little, because in the scenario that you describe the set(B) is an implicit unset(A), which has to be taken care of in the set method as well (i.e., the unregistration of A when setting B). I will change this and would be glad if you had a second look on it after my change.

…g/unsetting usbSerialDiscovery in the UsbSerialDiscoveryService Setting and unsetting of usbSerialDiscovery in the UsbSerialDiscoveryService is now synchronized, and should properly treat 'out of order' setting and unsetting of references by the OSGi container. Signed-off-by: Henning Sudbrock <[email protected]>

configuration in the modified method The configuration is now also set in the activate method; the superfluous modified method that simply delegates to the parent has been removed in the UsbSerialDiscoveryService.

@maggu2810

exception type in UsbSerialDiscovery As suggested by @maggu2810, an IOException is now used.

triller-telekom

Thank you for implementing this feature.

I left some comments inside the code. Please have a look at them.

triller-telekom · 2018-03-27T12:22:29Z

...home.config.discovery.usbserial.linux.sysfs.test/.settings/org.eclipse.jdt.groovy.core.prefs

@@ -0,0 +1,2 @@
+eclipse.preferences.version=1


I think you do not need any groovy settings, do you?

Should not be needed, as there are no groovy tests. I will remove the leftover references to groovy.

triller-telekom · 2018-03-27T12:24:04Z

bundles/config/org.eclipse.smarthome.config.discovery.usbserial.linux.sysfs.test/pom.xml

+    <artifactId>config</artifactId>
+    <version>0.10.0-SNAPSHOT</version>
+  </parent>
+


unnecessary blank line

Fixed (in other new pom files as well).

triller-telekom · 2018-03-27T12:28:30Z

...se/smarthome/config/discovery/usbserial/linux/sysfs/UsbSerialDeviceInformationGenerator.java

+    public UsbSerialDeviceInformation generate() {
+        int i = counter.getAndIncrement();
+        return new UsbSerialDeviceInformation(i, i, "serialNumber-" + i, "manufacturer-" + i, "product-" + i,
+                "ttyUSB" + i);


Are you sure that the creating of this AtomicInteger is always aligned with what the underlying operating system creates? I.e. is /dev/ttyUSB1 in the OS also a UsbSerialDeviceInformation object with property ttyUSB1?

This class here is only used for generating instances of UsbSerialDeviceInformation in tests, and the contents have no relation to actual USB devices in the OS (which is ok for the tests). I will add 'that can be used in tests' to the JavaDoc to make this a little more clear.

Does this answer your question?

Yes, thank you for the explanation and the added javadoc comment that you are only using this in tests.

Github abbreviated the path to the file so I originally did not see the test in the project name.

triller-telekom · 2018-03-27T12:31:30Z

...ipse/smarthome/config/discovery/usbserial/linux/sysfs/UsbSerialDeviceInformationMatcher.java

+            return other == null;
+        }
+
+        return Objects.equals(deviceInfo, other)


If both are the same reference in memory, you can omit the additional checks, because they are also the same, right?

Otherwise if you only want to compare the properties, you should not compare their references.

The equals method for UsbSerialDeviceInformation objects only considers serial port and model and vendor ID of the USB device, but not the fields manufacturer, product, and serial. As the checks shall test that all fields are equal, the matcher compares both the objects using equals, and the fields that are not included in the equality check.

The interesting question is why the equals method is implemented in this way. I did this, because UsbSerialDeviceInformation models a serial port provided by a USB device of some type, and I hence included the information about the serial port and about the USB device type. But one might as well include also the other fields into the equality check, and I will do this as it caused confusion here. Thanks for pointing this out.

triller-telekom · 2018-03-27T12:38:11Z

...pse/smarthome/config/discovery/usbserial/linux/sysfs/internal/DeltaUsbSerialScannerTest.java

+    }
+
+    /**
+     * If a first scan discovers devices usb1 and usb2, and a second scan discovers devices usb2 and usb2, then the


Typo? and a second scan discovers devices usb2 and usb3

Thanks, will fix.

triller-telekom · 2018-03-27T13:44:51Z

...clipse.smarthome.config.discovery.usbserial.test/.settings/org.eclipse.jdt.groovy.core.prefs

@@ -0,0 +1,3 @@
+eclipse.preferences.version=1


I guess this file is also a left over since there are no groovy files in this PR, right?

Correct, I'll remove the file.

triller-telekom · 2018-03-27T13:45:30Z

bundles/config/org.eclipse.smarthome.config.discovery.usbserial.test/pom.xml

+    <artifactId>config</artifactId>
+    <version>0.10.0-SNAPSHOT</version>
+  </parent>
+


Unnecessary blank line.

I'll remove it.

triller-telekom · 2018-03-27T14:10:12Z

...org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryServiceTest.java

+
+    private final AtomicInteger deviceInfoCounter = new AtomicInteger(0);
+
+    public UsbSerialDeviceInformation generateDeviceInfo() {


Why are you not using your UsbSerialDeviceInformationGenerator?

I do not use it, because I did not want to introduce a dependency between the test fragment for the bundle org.eclipse.smarthome.config.discovery.usbserial and the bundle org.eclipse.smarthome.config.discovery.usbserial.linux.sysfs just for this simple test data generator.

Do you think it is worthwile to introduce a dependency for this?

Since you need the code in both bundles anyway I think a dependency is better than duplicating the code.

But I would create the dependency in the other direction: Move the generator into the org.eclipse.smarthome.config.discovery.usbserial.test bundle and have this as a dependency in the org.eclipse.smarthome.config.discovery.usbserial.linux.sysfs.test bundle.

WDYT?

I, personally, would not introduce the dependency (as the piece of code is very short and quite simple). But I don't have such a strong opinion on that, so I will change as suggested.

triller-telekom · 2018-03-27T14:17:53Z

...ain/java/org/eclipse/smarthome/config/discovery/usbserial/UsbSerialDiscoveryParticipant.java

+ * {@link UsbSerialDiscoveryService} and can thus contribute {@link DiscoveryResult}s from
+ * scans for USB devices with an associated serial port.
+ *
+ * @author Henning sudbrock - initial contribution


Typo: sSudbrock :)

Will fix :)

triller-telekom · 2018-03-27T14:24:26Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

+    }
+
+    protected synchronized void unsetUsbSerialDiscovery(UsbSerialDiscovery usbSerialDiscovery) {
+        if (this.usbSerialDiscovery == usbSerialDiscovery) {


Does it make sense to have this method parameter and do this check here? Usually from an unset... method I do expect that it really unsets my variable. In your case the variable is only unset if it has the same value as the method parameter...

I introduced this due to @amitjoy 's review comment above (#5315 (comment)). The reason is that the usbSerialDiscovery is an optional dynamic dependency, and as indicated by @amitjoy , the OSGi container does not make any guarantees on the call order of the set and unset methods when there are more than one usbSerialDiscovery services available. I imagine that this could happen when changing to another implementation of UsbSerialDiscovery during bundle update, e.g., when replacing the current usbSerialDiscovery with an improved one. Then the OSGi container could first set the new service, and only unset the old one afterwards. Hence, as suggested by @amitjoy, the unset method does not unconditionally unset the current usbSerialDiscovery, but only unsets the usbSerialDiscovery passed as method parameter.

I just discussed briefly with @SJKA - it is probably possible to simple use a static dependency here - I will check if I can improve the code this way.

Using a static mandatory dependency works fine, I changed the code accordingly.

further discussion). * Small fixes (typos, superfluous whitespace) * Includes manufacturer, product, and serial in equality/hashcode for UsbSerialDeviceInformation * Removes now obsolete UsbSerialDeviceInformationMatcher * Removes superfluous eclipse groovy config files * Removes superfluous plugins from test launch configs

…dency in UsbSerialDiscoveryService - make it static mandatory Signed-off-by: Henning Sudbrock <[email protected]>

triller-telekom

Thank you for your changes.

Regarding the placement of the code, i.e. in the bundles or extensions directory. Your choice to put it into the bundles directory is alright.

I have left some minor comments in the code.

triller-telekom · 2018-03-29T08:03:18Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java


    public UsbSerialDiscoveryService() {
        super(5);
    }

+    @SuppressWarnings("null") // usbSerialDiscovery is not null, as it is set as static reference


You can mark such injected dependencies as @NonNullByDefault({}), see point 12 in https://www.eclipse.org/smarthome/documentation/development/guidelines.html#a-code-style

Afterwards you can remove the @SuppressWarnings("null") again.

Nice, I wasn't aware of that. Will change obviously (also in the PollingUsbSerialScanner) :)

triller-telekom · 2018-03-29T08:13:28Z

...se/smarthome/config/discovery/usbserial/linux/sysfs/UsbSerialDeviceInformationGenerator.java

+    public UsbSerialDeviceInformation generate() {
+        int i = counter.getAndIncrement();
+        return new UsbSerialDeviceInformation(i, i, "serialNumber-" + i, "manufacturer-" + i, "product-" + i,
+                "ttyUSB" + i);


Yes, thank you for the explanation and the added javadoc comment that you are only using this in tests.

Github abbreviated the path to the file so I originally did not see the test in the project name.

triller-telekom · 2018-03-29T08:24:24Z

...lipse/smarthome/config/discovery/usbserial/linux/sysfs/internal/PollingUsbSerialScanner.java

+
+    private final Logger logger = LoggerFactory.getLogger(PollingUsbSerialScanner.class);
+
+    private static final String THREAD_POOL_NAME = "usb-serial-discovery-linux-sysfs";


As far as I am aware, using the same name to get the thread pool means reusing and thus sharing the pool. So If you would also use discovery here your code would use the very same pool as the AbstractDiscoveryService.

You mention that your PollingUsbSerialScanner is decoupled from the UsbSerialDiscoveryService, but the UsbSerialDiscoveryService holds and instance of UsbSerialDiscovery, which at runtime might be a PollingUsbSerialScanner.

What I am trying to say is: This part of your code which wants to use a thread pool belongs to "discovery" tasks and thus I thought it would make sense to share the pool.

Maybe @SJKA or @kaikreuzer can comment on this, whether it makes sense to share the "discovery" thread pool for all discovery services?

triller-telekom · 2018-03-29T08:32:30Z

...eclipse/smarthome/config/discovery/usbserial/linux/sysfs/internal/SysfsUsbSerialScanner.java

+ *
+ * @author Henning Sudbrock - initial contribution
+ */
+@Component(configurationPid = "discovery.usbserial.linux.sysfs.usbserialscanner")


Valid point. Giving the actual discovery service UsbSerialDiscoveryService the PID discovery.XXX is what all of our discovery services do.

However, I don't know a guideline on how the PID for services should look like which are a part of the actual discovery service.

Maybe you are right and its PID should be a child PID of discovery.X

triller-telekom · 2018-03-29T08:45:08Z

...org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryServiceTest.java

+
+    private final AtomicInteger deviceInfoCounter = new AtomicInteger(0);
+
+    public UsbSerialDeviceInformation generateDeviceInfo() {


Since you need the code in both bundles anyway I think a dependency is better than duplicating the code.

But I would create the dependency in the other direction: Move the generator into the org.eclipse.smarthome.config.discovery.usbserial.test bundle and have this as a dependency in the org.eclipse.smarthome.config.discovery.usbserial.linux.sysfs.test bundle.

WDYT?

test utility classes in one test fragment The UsbSerialDeviceInformationGenerator is now exported by the usbserial test fragment, and used in that fragment and also in the usbserial.linux.sysfs test fragment. Signed-off-by: Henning Sudbrock <[email protected]>

annotations for static mandatory service references Signed-off-by: Henning Sudbrock <[email protected]>

Signed-off-by: Henning Sudbrock <[email protected]>

@Modified

modification work The UsbSerialDiscoveryService now has a modified method annotated as @Modified that delegates to the AbstractDiscoveryService modified method. Signed-off-by: Henning Sudbrock <[email protected]>

kaikreuzer · 2018-03-29T13:51:58Z

Might I suggest to rename org.eclipse.smarthome.config.discovery.usbserial.linux.sysfs to org.eclipse.smarthome.config.discovery.usbserial.linuxsysfs (or even org.eclipse.smarthome.config.discovery.usbserial.linux), just to not introduce any additional deeper hierarchies in the names?

hsudbrock · 2018-03-29T14:12:58Z

@kaikreuzer Removing one hierarchy level in the bundle names is fine for me. I'd rather change it to org.eclipse.smarthome.config.discovery.usbserial.linuxsysfs (and not org.eclipse.smarthome.config.discovery.usbserial.linux) as there is the possibility that one day someone might add an implementation that avoids polling by using one of the discovery approaches that relies on a native wrapper around libusb or udev, or that relies on dbus. (That's why I introduced the additional hierarchy level in the first place.)

…ad of linux.sysfs in the bundle names Signed-off-by: Henning Sudbrock <[email protected]>

triller-telekom

Thank you, LGTM now.

hsudbrock · 2018-04-03T16:03:45Z

As just discussed with @triller-telekom, in the current implementation DiscoveryParticipants are not informed about a USB dongle if the dongle was plugged in before the bundle containing the DiscoveryParticipant was installed. The reason is that DiscoveryParticipants are only informed when new USB dongles are detected.

Example: Assume the Zigbee binding provides a DiscoveryParticipant for Zigbee USB dongles. Assume that a Zigbee USB dongle is plugged in before the Zigbee binding is installed. Then the Zigbee DiscoveryParticipant will not be informed about the Zigbee USB dongle.

This can be improved simply by remembering detected USB dongles, and informing newly added `DiscoveryParticipant's about them. I will add this to this PR (is already implemented, but I also want to add a unit test for this, to which I will not get today).

DiscoveryParticipants after adding USB dongles Previously discovered USB dongles are now stored by the UsbSerialDiscoveryService, and newly added DiscoveryParticipants are informed about them. Signed-off-by: Henning Sudbrock <[email protected]>

triller-telekom

I have made ine comment regarding the clearing of your Set in the code.

triller-telekom · 2018-04-04T11:00:53Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

@@ -101,6 +109,7 @@ protected synchronized void unsetUsbSerialDiscovery(UsbSerialDiscovery usbSerial
        usbSerialDiscovery.stopBackgroundScanning();
        usbSerialDiscovery.unregisterDiscoveryListener(this);
        this.usbSerialDiscovery = null;
+        this.previouslyDiscovered.clear();


Are you sure that you want to clear the Set if you remove one participant? I think you have to leave the Set untouched if you remove a participant and only remove items from it in usbSerialDeviceRemoved, right?

@triller-telekom This method is called when unsetting the usbSerialDiscovery, not when removing a discoveryParticipant.

Github fooled me on this one... It folded away all lines between protected void removeUsbSerialDiscoveryParticipant(UsbSerialDiscoveryParticipant participant) { and usbSerialDiscovery.stopBackgroundScanning(); so I did not see that your added code ended up in unsetUsbSerialDiscovery. To me it looked as if you added it to removeUsbSerialDiscoveryParticipant.

So all is good now :)

triller-telekom · 2018-04-04T12:50:34Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

@@ -101,6 +109,7 @@ protected synchronized void unsetUsbSerialDiscovery(UsbSerialDiscovery usbSerial
        usbSerialDiscovery.stopBackgroundScanning();
        usbSerialDiscovery.unregisterDiscoveryListener(this);
        this.usbSerialDiscovery = null;
+        this.previouslyDiscovered.clear();


Github fooled me on this one... It folded away all lines between protected void removeUsbSerialDiscoveryParticipant(UsbSerialDiscoveryParticipant participant) { and usbSerialDiscovery.stopBackgroundScanning(); so I did not see that your added code ended up in unsetUsbSerialDiscovery. To me it looked as if you added it to removeUsbSerialDiscoveryParticipant.

So all is good now :)

sjsf

Great addition, really!

There were only a few minor, mostly formal things left for me to find. Mainly this is about guava (which we don't like on small gateways due to it's size/value-add ratio) and the consistent spelling of USB in logs.

In the realm of #5338, could you please also run the following maven plugin in order to format your newly added POMs:

mvn com.github.ekryd.sortpom:sortpom-maven-plugin:sort -Dsort.keepBlankLines=true

sjsf · 2018-04-04T12:48:46Z

.../eclipse/smarthome/config/discovery/usbserial/linuxsysfs/internal/DeltaUsbSerialScanner.java

+@NonNullByDefault
+public class DeltaUsbSerialScanner {
+
+    private Collection<UsbSerialDeviceInformation> lastScanResult = Sets.newHashSet();


Please don't add any new guava usages, as we're trying to get rid of it completely. Especially here, a simple

... = new HashSet<>();

would perfectly suffice.

Sounds sensible, changed.

sjsf · 2018-04-04T12:52:04Z

.../eclipse/smarthome/config/discovery/usbserial/linuxsysfs/internal/DeltaUsbSerialScanner.java

+
+        Set<UsbSerialDeviceInformation> added = getAddedDeviceInfos(deviceInfos);
+        Set<UsbSerialDeviceInformation> removed = getRemovedDeviceInfos(deviceInfos);
+        Set<UsbSerialDeviceInformation> unchanged = Sets.difference(deviceInfos, added);


... = new HashSet<>(deviceInfos).removeAll(added);

Unfortunately, removeAll does not return the set but a boolean indicating whether elements were removed :/ But of course the change is sensible, for removing Guava usage, and I will adapt the code.

sjsf · 2018-04-04T12:54:28Z

.../eclipse/smarthome/config/discovery/usbserial/linuxsysfs/internal/DeltaUsbSerialScanner.java

+
+    private ImmutableSet<UsbSerialDeviceInformation> getAddedDeviceInfos(
+            Collection<UsbSerialDeviceInformation> deviceInfos) {
+        return deviceInfos.stream().filter(deviceInfo -> !lastScanResult.contains(deviceInfo))


return Collections.unmodifiableSet(new HashSet<>(deviceInfos).removeAll(lastScanResult));

Good point, this (together with your previous and next comment) actually helps to simplify the code here significantly, as all added, removed, and unchanged are now just the result of a computation of a set difference.

sjsf · 2018-04-04T12:55:45Z

.../eclipse/smarthome/config/discovery/usbserial/linuxsysfs/internal/DeltaUsbSerialScanner.java

+
+    private ImmutableSet<UsbSerialDeviceInformation> getRemovedDeviceInfos(
+            Collection<UsbSerialDeviceInformation> deviceInfos) {
+        return lastScanResult.stream().filter(deviceInfo -> !deviceInfos.contains(deviceInfo))


return Collections.unmodifiableSet(new HashSet<>(lastScanResult).removeAll(deviceInfos));

see reply to comment above

sjsf · 2018-04-04T12:59:20Z

...clipse/smarthome/config/discovery/usbserial/linuxsysfs/internal/PollingUsbSerialScanner.java

+
+    @Activate
+    protected void activate(Map<String, Object> config) {
+        if (config.containsKey(PAUSE_BETWEEN_SCANS_IN_SECONDS_ATTRIBUTE)) {


You could delegate here to modified(...)

The reason I did not delegate to modified here is that modified actually does something more than activate, viz restarting the background scanning with the new value for the pause between scans.

What you are saying is of course correct, as this restart would simply not be done in activate, as the backgroundScanningJob is not yet running when activate is executed.

I would prefer to leave as is so that it is explicit that background scanning is not restarted on activation.

sjsf · 2018-04-04T13:34:07Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

+            usbSerialDiscovery.startBackgroundScanning();
+        }
+    }
+


This is missing a

@Override @Deactivate protected void deactivate() { super.deactivate(); }

I will add this (as discussed, to make explicit that the super method is called, although it would also be called without this overridden method).

sjsf · 2018-04-04T13:35:09Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

+        if (usbSerialDiscovery != null) {
+            usbSerialDiscovery.doSingleScan();
+        } else {
+            logger.info("Could not scan, as there is no usb serial discovery service configured.");


Will change.

sjsf · 2018-04-04T13:35:17Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

+            usbSerialDiscovery.startBackgroundScanning();
+        } else {
+            logger.info(
+                    "Could not start background discovery, as there is no usb serial discovery service configured.");


Will change.

sjsf · 2018-04-04T13:35:28Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

+        if (usbSerialDiscovery != null) {
+            usbSerialDiscovery.stopBackgroundScanning();
+        } else {
+            logger.info("Could not stop background discovery, as there is no usb serial discovery service configured.");


Will change.

sjsf · 2018-04-04T13:35:40Z

...ava/org/eclipse/smarthome/config/discovery/usbserial/internal/UsbSerialDiscoveryService.java

+
+    @Override
+    public void usbSerialDeviceDiscovered(UsbSerialDeviceInformation usbSerialDeviceInformation) {
+        logger.debug("Discovered new usb-serial device: {}", usbSerialDeviceInformation);


USB-serial

Will change.

hsudbrock · 2018-04-04T13:50:12Z

Thanks @SJKA for your feedback! I will adopt the PR accordingly.

* Unifies capitalization of USB and USB-Serial * Removes all usages of Guava * Formats the newly added poms using the sortpom maven plugin * Improves log messages Signed-off-by: Henning Sudbrock <[email protected]>

hsudbrock · 2018-04-04T15:56:00Z

@triller-telekom @SJKA : In a discussion about the PR I realized one more thing that I would like to add to this PR: Currently, the discovery provides information about the serial port and the USB dongle. This is sufficient for USB dongles that provide a single interface with a corresponding serial port. However, there are some USB dongles that provide more than one serial port (I am aware of only one, Nortek's Linear HUSBZB-1 that provides both Z-Wave and Zigbee connectivity). For this reason, I will also include the USB interface addressed by the serial port to the UsbSerialDeviceInformation, so that DiscoveryParticipants can use this information (e.g., for the Nortek dongle, to identify the serial port for the Z-Wave and the serial port for the Zigbee connectivity).

UsbserialDeviceInformation The UsbSerialDeviceInformation now also includes the interface number and the (optional) interface description. This will help with USB dongles with multiple interface (providing, e.g., both Zigbee and Z-Wave connectivity). Signed-off-by: Henning Sudbrock <[email protected]>

sjsf · 2018-04-05T13:01:57Z

Thanks!

…to p2 and karaf features The bundles o.e.sh.config.discovery.usbserial and o.e.sh.config.discovery.usbserial.linuxsysfs were recently added in eclipse-archived#5315, but they were not added to the p2 and karaf features. Signed-off-by: Henning Sudbrock <[email protected]>

…to p2 and karaf features (#5400) The bundles o.e.sh.config.discovery.usbserial and o.e.sh.config.discovery.usbserial.linuxsysfs were recently added in #5315, but they were not added to the p2 and karaf features. Signed-off-by: Henning Sudbrock <[email protected]>

…to p2 and karaf features (eclipse-archived#5400) The bundles o.e.sh.config.discovery.usbserial and o.e.sh.config.discovery.usbserial.linuxsysfs were recently added in eclipse-archived#5315, but they were not added to the p2 and karaf features. Signed-off-by: Henning Sudbrock <[email protected]>

) This commit contains an API for a generic discovery for USB devices that can be accessed via a serial port (typically provided by an operating system-specific driver). Examples of such things that could be discovered are bridges for wireless protocols (like, e.g., Zigbee, Z-Wave, HomeMatic, or HomeMatic IP) using an USB dongle, where the binding uses serial communication to communicate with the dongle. This discovery needs to be complemented from two sides: On the one side, bindings must contribute 'DiscoveryParticipants', which, given information about serial port and USB device provided by the discovery, generate a DiscoveryResult in case they support the USB device. On the other side, an implementation of the actual scanning for USB devices and serial ports must be provided. There are different options for providing such a discovery, as discussed on eclipse-archived#5153. This commit contains an implementation of the actual discovery that can be used on Linux systems, which regularly scans the so-called 'sysfs', a Linux pseudo file system that provides information about devices. fixes eclipse-archived#5153 Signed-off-by: Henning Sudbrock <[email protected]>

openhab-bot · 2018-12-11T10:06:54Z

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

https://community.openhab.org/t/ebus-2-0-new-binding-release-candidate-3/33547/529

amitjoy reviewed Mar 26, 2018

View reviewed changes

eclipse-archived deleted a comment from eclipsewebmaster Mar 26, 2018

amitjoy reviewed Mar 26, 2018

View reviewed changes

hsudbrock added 3 commits March 27, 2018 09:24

[5153] Addresses @amitjoy's review comment regarding setting the

6530263

configuration in the modified method The configuration is now also set in the activate method; the superfluous modified method that simply delegates to the parent has been removed in the UsbSerialDiscoveryService.

[5153] Addresses @amitjoy's review comment regarding a more specific

663eeeb

exception type in UsbSerialDiscovery As suggested by @maggu2810, an IOException is now used.

triller-telekom reviewed Mar 27, 2018

View reviewed changes

hsudbrock added 2 commits March 28, 2018 11:49

[5153] Address review comments regarding the usbSerialDiscovery depen…

2c3ff4c

…dency in UsbSerialDiscoveryService - make it static mandatory Signed-off-by: Henning Sudbrock <[email protected]>

triller-telekom reviewed Mar 29, 2018

View reviewed changes

hsudbrock added 4 commits March 29, 2018 11:57

[5153] Addresses @triller-telekom's comment regarding NonNullByDefault

2c22d2f

annotations for static mandatory service references Signed-off-by: Henning Sudbrock <[email protected]>

[5153] Fix minor issues detected by the static code analysis

6f6f739

Signed-off-by: Henning Sudbrock <[email protected]>

[5153] Fix failing test and make UsbSerialDiscoveryService config

af5dd60

modification work The UsbSerialDiscoveryService now has a modified method annotated as @Modified that delegates to the AbstractDiscoveryService modified method. Signed-off-by: Henning Sudbrock <[email protected]>

[5153] Addresses @kaikreuzer's review comment to use linuxsysfs inste…

177d8b9

…ad of linux.sysfs in the bundle names Signed-off-by: Henning Sudbrock <[email protected]>

triller-telekom approved these changes Apr 3, 2018

View reviewed changes

triller-telekom suggested changes Apr 4, 2018

View reviewed changes

triller-telekom approved these changes Apr 4, 2018

View reviewed changes

sjsf reviewed Apr 4, 2018

View reviewed changes

[5153] Addresses the review comments by @SJKA

9c422d3

* Unifies capitalization of USB and USB-Serial * Removes all usages of Guava * Formats the newly added poms using the sortpom maven plugin * Improves log messages Signed-off-by: Henning Sudbrock <[email protected]>

sjsf approved these changes Apr 5, 2018

View reviewed changes

sjsf merged commit 4dbc5e1 into eclipse-archived:master Apr 5, 2018

This was referenced Apr 10, 2018

Add the bundles for discovery of usb devices providing a serial port to p2 and karaf features #5400

Merged

Support discovery of USB dongles openhab/org.openhab.binding.zigbee#178

Closed

hsudbrock mentioned this pull request Apr 12, 2018

[178] Support Discovery of USB dongles (for some Telegesis and one Ember dongle) openhab/org.openhab.binding.zigbee#179

Merged

hsudbrock mentioned this pull request Apr 22, 2018

Include feature dependency to ESH USB-Serial discovery for ZigBee binding openhab/openhab-distro#685

Closed

kaikreuzer changed the title ~~[5153] Discovery for USB devices with serial port~~ [Core] Introduced discovery for USB devices with serial port May 20, 2018

kaikreuzer added the enhancement label May 20, 2018

htreu added this to the 0.10.0 milestone Oct 30, 2018

maggu2810 mentioned this pull request Jun 21, 2019

Reduce polling frequency of usb serial scanner openhab/openhab-core#889

Merged

mherwege mentioned this pull request Dec 14, 2023

Add-on suggestion finder for USB devices openhab/openhab-core#3922

Merged


		private final AtomicInteger deviceInfoCounter = new AtomicInteger(0);

		public UsbSerialDeviceInformation generateDeviceInfo() {


		private final Logger logger = LoggerFactory.getLogger(PollingUsbSerialScanner.class);

		private static final String THREAD_POOL_NAME = "usb-serial-discovery-linux-sysfs";

[Core] Introduced discovery for USB devices with serial port #5315

[Core] Introduced discovery for USB devices with serial port #5315

Conversation

hsudbrock commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsudbrock commented Mar 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsudbrock Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsudbrock Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer commented Mar 29, 2018

hsudbrock commented Mar 29, 2018

triller-telekom left a comment

Choose a reason for hiding this comment

hsudbrock commented Apr 3, 2018

triller-telekom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsudbrock Mar 28, 2018 •

edited

Loading

hsudbrock Mar 29, 2018 •

edited

Loading