Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UsbSerialDiscovery service based on Javax-Usb #3930

Open
wants to merge 25 commits into
base: main
Choose a base branch
from

Conversation

andrewfg
Copy link
Contributor

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 andrewfg requested a review from a team as a code owner December 18, 2023 13:25
@andrewfg andrewfg changed the title New UsbSerialDiscovery service based on Javax-Usb [wip] New UsbSerialDiscovery service based on Javax-Usb Dec 18, 2023
@andrewfg
Copy link
Contributor Author

@mherwege this was split from #3922

@andrewfg
Copy link
Contributor Author

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 andrewfg changed the title [wip] New UsbSerialDiscovery service based on Javax-Usb [wip] UsbSerialDiscovery service based on Javax-Usb Dec 18, 2023
@wborn
Copy link
Member

wborn commented Dec 18, 2023

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.

@andrewfg
Copy link
Contributor Author

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
Copy link
Contributor Author

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

@mherwege
Copy link
Contributor

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 ?

@andrewfg
Copy link
Contributor Author

andrewfg commented Dec 18, 2023

Maybe using this

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

@wborn wborn added enhancement An enhancement or new feature of the Core work in progress A PR that is not yet ready to be merged labels Dec 19, 2023
@wborn
Copy link
Member

wborn commented Dec 21, 2023

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?

@andrewfg
Copy link
Contributor Author

andrewfg commented Dec 21, 2023

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

Related PRs

Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@wborn wborn removed the work in progress A PR that is not yet ready to be merged label Dec 30, 2023
@wborn
Copy link
Member

wborn commented Dec 30, 2023

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?

@andrewfg
Copy link
Contributor Author

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 ??

@mherwege
Copy link
Contributor

we can remove the Linux and Windows binaries

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

@wborn
Copy link
Member

wborn commented Jan 2, 2024

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>

@holgerfriedrich
Copy link
Member

@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?

@andrewfg
Copy link
Contributor Author

andrewfg commented Jan 6, 2024

@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?

@holgerfriedrich
Copy link
Member

holgerfriedrich commented Jan 6, 2024

@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().

@andrewfg
Copy link
Contributor Author

andrewfg commented Jan 6, 2024

if I read #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?

@holgerfriedrich
Copy link
Member

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

@wborn
Copy link
Member

wborn commented Jan 6, 2024

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

You may have run into #3983

Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg andrewfg requested a review from wborn January 7, 2024 11:59
@andrewfg
Copy link
Contributor Author

andrewfg commented Jan 7, 2024

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
Copy link
Contributor Author

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..

Signed-off-by: Andrew Fiddian-Green <[email protected]>
<overWriteReleases>true</overWriteReleases>
<overWriteSnapshots>true</overWriteSnapshots>
<excludeTransitive>false</excludeTransitive>
<type>jar</type>
Copy link
Member

Choose a reason for hiding this comment

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

You can exclude the Linux and Windows libraries by adding this:

Suggested change
<type>jar</type>
<excludes>**/linux-*,**/linux-*/*.so,**/win32-*,**/win32-*/*.dll</excludes>
<type>jar</type>

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed it only contains darwin-x86-64 libraries so it will not work on Apple silicon based Macs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will not work on Apple silicon based Macs

Nor on Arm based ones .. although TBH I am not sure if any such exist(ed)..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

.. and I even wonder if MacOS does not (still) have an inner core based on Linux .. I know that it used to

Copy link
Member

Choose a reason for hiding this comment

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

It's based on Unix because Linux didn't even exist yet back then. 🦖

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To summarise: this seems to be a Mac specific issue; neither you nor I have a Mac to test on; so we can't really judge if this PR adds value, or if the existing Discovery component added value prior to this. So the question is what shall we do with this one?

Copy link
Member

Choose a reason for hiding this comment

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

Let's wait for the feedback of a maintainer using macOS. IIRC there is also x86 emulation on the Macs using Apple silicon. I'm not familiar with how the emulation works. It could be that it only works well when all code runs on the x86 emulator and not just a single library.

@andrewfg
Copy link
Contributor Author

@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?

Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@kaikreuzer
Copy link
Member

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...

@andrewfg
Copy link
Contributor Author

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.

@wborn
Copy link
Member

wborn commented Jan 28, 2024

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. 😉

@kaikreuzer
Copy link
Member

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

<parent>
<groupId>org.openhab.core.bundles</groupId>
<artifactId>org.openhab.core.reactor.bundles</artifactId>
<version>4.2.0-SNAPSHOT</version>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<version>4.2.0-SNAPSHOT</version>
<version>4.3.0-SNAPSHOT</version>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants