openhab-core icon indicating copy to clipboard operation
openhab-core copied to clipboard

UsbSerialDiscovery service based on Javax-Usb

Open andrewfg opened this issue 1 year ago • 29 comments

Split from #3922

This is a new implementation of the UsbSerialDiscovery interface based on Javax-Usb. It provides a UsbSerialDiscovery service for all operating systems. So in particular it extends coverage to Windows and Mac OS which are otherwise not covered by an implementation of UsbSerialDiscovery.

Signed-off-by: Andrew Fiddian-Green [email protected]

andrewfg avatar Dec 18 '23 13:12 andrewfg

@mherwege this was split from #3922

andrewfg avatar Dec 18 '23 13:12 andrewfg

building up a database is a lot of work and will require continuous efforts to adapt to new hardware

The word 'database' is confusing. So I changed its name to information provider. It is just that in some cases the javax.usb code delivers null values for product and manufacturer description texts. So information provider fills in the blanks for a dozen or so sticks where we happen to already have that information from other sources i.e. the community.

andrewfg avatar Dec 18 '23 13:12 andrewfg

So in particular it extends coverage to Windows and Mac OS which are otherwise not covered by an implementation of UsbSerialDiscovery.

Can it also be used on Linux instead of linuxsysfs or does it come with significant disadvantages?

So information provider fills in the blanks for a dozen or so sticks where we happen to already have that information from other sources i.e. the community.

Does this match the strings provided by linuxsysfs and ser2net? If not that may cause discovery issues.

wborn avatar Dec 18 '23 17:12 wborn

Can it also be used on Linux instead of linuxsysfs or does it come with significant disadvantages?

Yes it CAN be used on Linux (but it is not a MUST). Depending on user access rights -- javaxusb may return only the vendorId and productId yet the manufacturer description and product descriptions might be null. I don't know if linuxsysfs suffers from similar access rights issues too, but I get the impression that linuxsysfs might have a higher chance of returning both the ids, and the manufacturer and product descriptions strings too. ?? However TBH I have not really done a full side by side cross comparison of which delivers what data under which circumstances..

Does this match the strings provided by linuxsysfs and ser2net?

I imagine that if both linuxsysfs and javaxusb DO successfully return all strings (rather than null) then those strings would almost certainly be identical. (Note ser2net is not really in the same ball game as the other two, so it might be different -- but I can't really say).

In the cases where javaxusb returns null manufacturer / product strings, then the local information provider might be able to fill in those gaps from OH local data -- and in that case the fill-in strings almost certainly WILL differ from the OEM strings (if they would have been retrieved). Reason is that the local fill-in strings (written by me) explicitly have key-words (e.g. Z-Wave, Zigbee, etc.) in them in order to specifically improve the chances of proper discovery.

If not that may cause discovery issues.

As mentioned above, my feeling is that the local fill-in information may IMPROVE chances of discovery. Obviously in the case of inbox, the representation property must be stable across discovery service implementations. Which I think it is already. However perhaps you are thinking of some other reasons why this might cause issues? In that case please let me know so I can try to address them.

andrewfg avatar Dec 18 '23 18:12 andrewfg

Without wishing to further confuse the issue, I should perhaps mention that there is potentially yet another way of extracting USB device discovery data on the Windows platform -- namely via the Windows registry (see below). I don't even know if it is possible for Java code to read the windows registry, but if it is, then I imagine we could parse the registry analog to the way that linuxsysfs does it with the file system. Or something like that. Just a thought..

image

andrewfg avatar Dec 18 '23 18:12 andrewfg

Without wishing to further confuse the issue, I should perhaps mention that there is potentially yet another way of extracting USB device discovery data on the Windows platform -- namely via the Windows registry (see below). I don't even know if it is possible for Java code to read the windows registry, but if it is, then I imagine we could parse the registry analog to the way that linuxsysfs does it with the file system. Or something like that. Just a thought..

Maybe using this: https://www.rgagnon.com/javadetails/java-read-write-windows-registry-using-jna.html ?

mherwege avatar Dec 18 '23 20:12 mherwege

Maybe using this

@mherwege thanks for the suggestion; I will have a deeper look at that. EDIT: see #3934

andrewfg avatar Dec 18 '23 21:12 andrewfg

Reason is that the local fill-in strings (written by me) explicitly have key-words (e.g. Z-Wave, Zigbee, etc.) in them in order to specifically improve the chances of proper discovery.

But that means that other UsbSerialDiscovery implementations which do not use these specific keywords cannot be used for add-on suggestions? Why not use the vendor and product IDs in the addon.xml so suggestions works with every UsbSerialDiscovery implementation?

wborn avatar Dec 21 '23 13:12 wborn

other UsbSerialDiscovery implementations which do not use these specific keywords cannot be used for add-on suggestions

Why not use the vendor and product IDs in the addon.xml so suggestions works with every UsbSerialDiscovery implementation?

Weeell .. it is complicated :(

  • Some sticks are built around common OEM USB UART chips (such as Silicon Labs 0x10c4:0xea60) so a match based simply on vendorId and productId can lead to false positives from all sticks that use that chip -- particularly if that chip is very popular.
  • When a stick manufacturer uses a specific OEM chip (say the 0x10c4:0xea60) inside their product, they can add an additional application specific layer of information about their product. So for example the Sonoff Zigbee stick has the standard (very common) vendorId=0x10c4, productId=0xea60 (read from the OEM chip itself) plus additional application specific properties e.g. manufacturer="Sonoff", product="Sonoff Zigbee USB stick".
  • Therefore you get a better discrimination against false positives if the finder service can check on the latter two application specific properties rather than the generic OEM chip Id properties.
  • The problem is that the scanner (in particular this Javax-Usb scanner) can only read the application specific properties if a) on Linux, the application has read access to the sysfs folder), or b) on Windows, there is a driver installed. And if those cases are not fulfilled the application specific properties are null.

So the purpose of this local information provider is: if the application specific properties are null, and the chip id properties are known (they are always), and those properties are NOT for a very common generic UART chip, then try to fill in the gaps with a locally sourced version of the application specific properties, which is based on knowledge that we have gathered from our users.


Related threads

  • https://github.com/openhab/org.openhab.binding.zigbee/issues/818
  • https://github.com/openhab/org.openhab.binding.zwave/issues/1906

Related PRs

  • https://github.com/openhab/org.openhab.binding.zigbee/pull/819
  • https://github.com/openhab/org.openhab.binding.zwave/pull/1908
  • https://github.com/openhab/openhab-addons/pull/16089

andrewfg avatar Dec 21 '23 15:12 andrewfg

@wborn / @mherwege after some consideration I finally decided to adopt your suggestion to remove the local information provider, and instead rely on the discovery matchers to check the chipId.

andrewfg avatar Dec 25 '23 10:12 andrewfg

As https://github.com/openhab/openhab-core/pull/3922 is now merged, this one is also ready for review. I think there are a few points to note as follows..

  • it is quite heavy in terms of imported libraries
  • it can only reliably find the chipId of USB devices
  • the sysfs service provides more complete results on Linux
  • the windowsregistry service provides more complete results on Windows

So we need to discuss a) if it should only be loaded on systems with OS other than Windows or Linux, or b) if this PR should even be cancelled entirely?

andrewfg avatar Dec 27 '23 17:12 andrewfg

So we need to discuss a) if it should only be loaded on systems with OS other than Windows or Linux, or b) if this PR should even be cancelled entirely?

I share your concerns about the library and its size. We risk having to maintain it ourselves if we want to use the latest native libraries with bug fixes, and it is quite large. I guess the other major platform we are missing from the other serial discovery services is MacOS. I don’t have a Mac. Anyone with a Mac to comment?

mherwege avatar Dec 27 '23 22:12 mherwege

We risk having to maintain it ourselves if we want to use the latest native libraries with bug fixes, and it is quite large.

If it will only be used on macOS we can remove the Linux and Windows binaries which will reduce the bundle size from ~1 MB to ~0.2 MB.

I don’t have a Mac. Anyone with a Mac to comment?

I think @kaikreuzer and @J-N-K use Macs so maybe they can tell how well this discovery works on macOS?

wborn avatar Dec 30 '23 21:12 wborn

we can remove the Linux and Windows binaries

And indeed the features for the sysfs and windowsregistry discovery classes can also perhaps be made platform conditional. Or ??

andrewfg avatar Dec 31 '23 14:12 andrewfg

we can remove the Linux and Windows binaries

Wouldn’t that mean se have to compile and maintain a copy ourselves?

mherwege avatar Dec 31 '23 16:12 mherwege

Wouldn’t that mean se have to compile and maintain a copy ourselves?

The native libraries are dependencies of org.usb4java:usb4java which can be excluded:

    <dependency>
      <groupId>org.usb4java</groupId>
      <artifactId>libusb4java</artifactId>
      <version>${libusb4java.version}</version>
      <classifier>linux-x86</classifier>
    </dependency>
    <dependency>
      <groupId>org.usb4java</groupId>
      <artifactId>libusb4java</artifactId>
      <version>${libusb4java.version}</version>
      <classifier>linux-x86-64</classifier>
    </dependency>
    <dependency>
      <groupId>org.usb4java</groupId>
      <artifactId>libusb4java</artifactId>
      <version>${libusb4java.version}</version>
      <classifier>win32-x86</classifier>
    </dependency>
    <dependency>
      <groupId>org.usb4java</groupId>
      <artifactId>libusb4java</artifactId>
      <version>${libusb4java.version}</version>
      <classifier>win32-x86-64</classifier>
    </dependency>
    <dependency>
      <groupId>org.usb4java</groupId>
      <artifactId>libusb4java</artifactId>
      <version>${libusb4java.version}</version>
      <classifier>darwin-x86-64</classifier>
    </dependency>
    <dependency>
      <groupId>org.usb4java</groupId>
      <artifactId>libusb4java</artifactId>
      <version>${libusb4java.version}</version>
      <classifier>linux-arm</classifier>
    </dependency>
    <dependency>
      <groupId>org.usb4java</groupId>
      <artifactId>libusb4java</artifactId>
      <version>${libusb4java.version}</version>
      <classifier>linux-aarch64</classifier>
    </dependency>

wborn avatar Jan 02 '24 10:01 wborn

@andrewfg As openhab/openhab-distro#1626 is merged, I tried this on Windows. I rebased to current main.

My zwave stick is found,

[DEBUG] [al.internal.UsbSerialDiscoveryService] - Discovered new USB-Serial device: UsbSerialDeviceInformation [vendorId=0x0658, productId=0x0200, serialNumber=null, manufacturer=null, product=null, interfaceNumber=0x00, interfaceDescription=null, serialPort=, remote=false]

zwave addon is included in addons.xml.

Should it be suggested, or did I miss something to make it work?

holgerfriedrich avatar Jan 05 '24 22:01 holgerfriedrich

@holgerfriedrich

Should it be suggested did I miss something to make it work?

I think so. But why do you ask? It sounds like you have some doubt. Are you saying that it is 'found' but not 'suggested'? If so can you please trace log the suggestion finder?

andrewfg avatar Jan 06 '24 11:01 andrewfg

@andrewfg

My Zwave stick is discovered by UsbSerialDiscoveryService. Good.

We seem to have a valid description of Zwave add-on in addons.xml. Fine.

I was just wondering if Zwave add-on should be suggested - for me it is not the case.

I have not looked into the code deeply, but if I read #3922 correctly, it should automatically use the new JavaxUsbSerialDiscovery as all UsbSerialDiscovery services are injected via addUsbSerialDiscovery().

holgerfriedrich avatar Jan 06 '24 12:01 holgerfriedrich

if I read https://github.com/openhab/openhab-core/pull/3922 correctly, it should automatically use the new JavaxUsbSerialDiscovery as all UsbSerialDiscovery services are injected via addUsbSerialDiscovery().

Indeed. It will use ALL UsbSerialDiscovery components i.e. the existing Linux 'sysfs' and 'Ser2Net' components plus also my new 'javaxusb` and 'windowsregistry' components.

wondering if Zwave add-on should be suggested - for me it is not the case.

Hmm. One thought is that the chipId regex in addon.xml does not properly check upper- vs. lower- case hex characters e.g. 1234:abcd != 1234:ABCD => could you please try to manually change the regex in your /runtime/etc/addons.xml file by adding an ignore case prefix as shown below? But to be honest I think your stick 0658:0200 should NOT have that problem anyway..

// from this
<regex>0658:0200|10C4:8A2A|1A86:55D4</regex>

// to this
<regex>(?i)0658:0200|10C4:8A2A|1A86:55D4</regex>

NOTE: I have the same Z-Wave stick as you 0658:0200 and in my tests, it did suggest the addon.

So can you please submit trace logs for the finder?

andrewfg avatar Jan 06 '24 12:01 andrewfg

There was not much in the trace log. Compiled and installed again.... and 🎉 it works! image

holgerfriedrich avatar Jan 06 '24 13:01 holgerfriedrich

Compiled and installed again.... and 🎉 it works!

You may have run into https://github.com/openhab/openhab-core/issues/3983

wborn avatar Jan 06 '24 14:01 wborn

The native libraries are dependencies of org.usb4java:usb4java which can be excluded:

@wborn I am not sure how you want to handle this?

andrewfg avatar Jan 07 '24 12:01 andrewfg

The native libraries are dependencies of org.usb4java:usb4java which can be excluded

@wborn these are transitive dependencies and I would not know how to (directly) exclude them. This is a different case from from the feature <conditional> bundle install case that we are discussing elsewhere..

andrewfg avatar Jan 11 '24 12:01 andrewfg

@openhab/core-maintainers does any of you have a Mac where you can a) test this PR and/or b) at least have an opinion on it?

andrewfg avatar Jan 20 '24 17:01 andrewfg

Well, I am on macOS, but if I understand it right, you are looking for someone with Apple Silicon, right? I am still on x86...

kaikreuzer avatar Jan 27 '24 20:01 kaikreuzer

Well, I am on macOS, but if I understand it right, you are looking for someone with Apple Silicon, right? I am still on x86...

@kai the issue is we have various UsbSerialDiscovery implementation components; we have one that can find usb sticks on Linux, one that can find usb sticks on Windows, and one that can find ip remote usb sticks. However we do not have one that can find usb sticks on any other platform. The implementation in this PR could in theory find usb sticks on any other platform. In fact it can find sticks on Linux and Windows as well, but since it is quite heavy we prefer to use the other existing implementations on those platforms, and only use this PR on the platforms that the other components don't support. However I cannot test if this PR does actually work on such other platforms, so we don't really know if this PR provides any value at all on such platforms. Your use case MacOS on x86 is at least one candidate for this, and MacOS on Apple Silicon would be another.

andrewfg avatar Jan 28 '24 10:01 andrewfg

you are looking for someone with Apple Silicon, right? I am still on x86

It only has darwin-x86-64 libraries, so if it does not work for you it certainly won't work on Apple silicon. :wink:

wborn avatar Jan 28 '24 10:01 wborn

I am by now on Apple Silicon, so I guess I do not even have to test this anymore...

kaikreuzer avatar May 26 '24 14:05 kaikreuzer