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 Windows registry #3934

Merged
merged 20 commits into from
Jan 11, 2024

Conversation

andrewfg
Copy link
Contributor

This is a new implementation of the UsbSerialDiscovery interface for Windows, which discovers USB devices by reading from the Windows registry.

This may be an alternative, or an extension, to #3930

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

@andrewfg andrewfg requested a review from a team as a code owner December 19, 2023 16:26
@andrewfg
Copy link
Contributor Author

@mherwege 4yi

@andrewfg
Copy link
Contributor Author

andrewfg commented Dec 19, 2023

@mherwege it will probably fail the Karaf build (as usual) due to missing feature :( -- EDIT: fixed :)

@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
@andrewfg andrewfg changed the title [wip] UsbSerialDiscovery service based on Windows registry UsbSerialDiscovery service based on Windows registry Dec 24, 2023
@andrewfg
Copy link
Contributor Author

@mherwege I added the missing serial port and interface information

Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Andrew Fiddian-Green <[email protected]>
@andrewfg
Copy link
Contributor Author

As #3922 is now merged, this one is also ready for review.

@wborn wborn removed the work in progress A PR that is not yet ready to be merged label Dec 30, 2023
Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

This seems useful for Windows users. 👍

I only have a few minor comments:

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

andrewfg commented Jan 7, 2024

@wborn there is still the question of which UsbSerialDiscovery components should be loaded on which platforms .. I guess the following .. => WDYT? And the next question is how to program the dependencies and karaf stuff .. which is probably beyond my capabilities :(

  • ser2net - all platforms
  • sysfs - Linux
  • windowsregistry - Windows
  • javaxusb - all platforms except Linux and Windows

@wborn
Copy link
Member

wborn commented Jan 9, 2024

I guess the following

Yes that probably makes sense.

sysfs - Linux

It doesn't use any native dependency which is very convenient because Linux runs on so many platforms. So it will result in fewer issues compared to javaxusb.

javaxusb - all platforms except Linux and Windows

It uses native libraries and only supports Linux, Windows and macOS. So it probably only makes sense to use it on macOS.

And the next question is how to program the dependencies and karaf stuff

You may be able to use <conditional> for that, see also #3632 (comment).

@andrewfg
Copy link
Contributor Author

may be able to use for that

@wborn the following is simply a wild guess idea .. but I would like to have some feedback from more knowledgeable Karaf experts before even attempting to make such a commit.

.. in particular if the osgi.native.osname~=Windows is a proper syntax..

<feature name="openhab-transport-serial" description="Serial Transport" version="${project.version}">
    <feature>openhab-core-base</feature>

    <requirement>openhab.tp;filter:="(feature=commons-net)"</requirement>
    <feature dependency="true">openhab.tp-commons-net</feature>

    <requirement>openhab.tp;filter:="(&amp;(feature=serial)(impl=rxtx))"</requirement>
    <feature dependency="true">openhab.tp-serial-rxtx</feature>

    <feature dependency="true">openhab-core-io-transport-mdns</feature>

    <bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.serial/${project.version}</bundle>
    <bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.discovery.usbserial/${project.version}</bundle>
    
    <conditional>
        <condition>req:osgi.native;filter:="(osgi.native.osname=MacOS)"</condition>
        <bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.discovery.usbserial.javaxusb/${project.version}</bundle>
    </conditional>
    
    <conditional>
        <condition>req:osgi.native;filter:="(osgi.native.osname=Linux)"</condition>
        <bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.discovery.usbserial.linuxsysfs/${project.version}</bundle>
    </conditional>
    
    <bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.discovery.usbserial.ser2net/${project.version}</bundle>
    
    <conditional>
        <condition>req:osgi.native;filter:="(osgi.native.osname~=Windows)"</condition>
        <bundle>mvn:org.openhab.core.bundles/org.openhab.core.config.discovery.usbserial.windowsregistry/${project.version}</bundle>
    </conditional>
    
    <bundle>mvn:org.openhab.core.bundles/org.openhab.core.io.transport.serial/${project.version}</bundle>
    <bundle>mvn:org.openhab.core.bundles/org.openhab.core.io.transport.serial.rxtx/${project.version}</bundle>
    <bundle>mvn:org.openhab.core.bundles/org.openhab.core.io.transport.serial.rxtx.rfc2217/${project.version}</bundle>

</feature>

@wborn
Copy link
Member

wborn commented Jan 10, 2024

I haven't used this myself yet. Just give it a try and see what happens. 🙂

@andrewfg
Copy link
Contributor Author

Just give it a try and see what happens

Yeah. I just ran a full build of OH this afternoon (it takes ages on my machine; I probably need to buy a new one). The build succeeded without error. But I will test it tomorrow.

@andrewfg
Copy link
Contributor Author

@wborn below is a screenshot in Windows showing that this (Windows) discovery is installed, and the existing (Linux) discovery is not. To be ultra safe we would need to check the opposite case on a Linux machine. However I currently don't have a Linux test machine, so I wonder if you could eventually test it for me?

image

@wborn
Copy link
Member

wborn commented Jan 11, 2024

I wonder if you could eventually test it for me?

I just gave it a test on Ubuntu and the linuxsysfs bundle keeps getting installed while the windowsregistry bundle is not. 👍

Copy link
Member

@wborn wborn left a comment

Choose a reason for hiding this comment

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

Thanks for the new UsbSerialDiscovery!

@wborn wborn merged commit 9cb4b9e into openhab:main Jan 11, 2024
2 of 3 checks passed
@wborn wborn added this to the 4.2 milestone Jan 11, 2024
@andrewfg
Copy link
Contributor Author

gave it a test on Ubuntu and the linuxsysfs bundle keeps getting installed

I also -- finally -- managed to test it on a RasPi3 running Linux and it also shows the correct components loaded..

image

cipianpascu pushed a commit to cipianpascu/openhab-core that referenced this pull request Jan 17, 2024
Signed-off-by: Andrew Fiddian-Green <[email protected]>
Signed-off-by: Ciprian Pascu <[email protected]>
@andrewfg andrewfg deleted the discovery-windows-registry branch August 25, 2024 16:28
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.

3 participants