Android support via fileDescriptor #130

kubaraczkowski · 2024-07-19T20:08:28Z

See base #126
Since libusb supports the peculiar Android USB handling (usb device must be opened from Java, after acquiring permission from the user to do so) it's useful to enable gousb to do the same.

Checked using a basic Android app with a USB serial device in loopback.

Documentation is not yet done, if the code changes are OK, I'll write the docstrings as well

opts

…recommended libusb context creation method

google-cla · 2024-07-19T20:08:32Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

zagrodzki · 2024-07-22T13:36:07Z

fakelibusb_test.go

@@ -98,7 +100,9 @@ type fakeLibusb struct {
 	claims map[*libusbDevice]map[uint8]bool
 }

-func (f *fakeLibusb) init() (*libusbContext, error)                       { return newContextPointer(), nil }
+func (f *fakeLibusb) init() (*libusbContext, error) {


revert? this is just different formatting

zagrodzki · 2024-07-22T13:36:18Z

fakelibusb_test.go

@@ -87,6 +87,8 @@ type fakeLibusb struct {
 	mu sync.Mutex
 	// fakeDevices has a map of devices and their descriptors.
 	fakeDevices map[*libusbDevice]*fakeDevice
+	// fakeLibUsbDevices keeps the order of devices to be accessd by wrapSysDevice
+	fakeLibUsbDevices []*libusbDevice


fakeSysDevices

zagrodzki · 2024-07-22T13:38:18Z

fakelibusb_test.go

@@ -87,6 +87,8 @@ type fakeLibusb struct {
 	mu sync.Mutex
 	// fakeDevices has a map of devices and their descriptors.
 	fakeDevices map[*libusbDevice]*fakeDevice
+	// fakeLibUsbDevices keeps the order of devices to be accessd by wrapSysDevice
+	fakeLibUsbDevices []*libusbDevice


would it be better to instead have a map of uintptr->device? That would allow for more realistic pointer-like values, or at least wouldn't limit the code to use contiguous 0-indexed range of values.

Sure, good point. The FDs I randomly saw were e.g. never 0,1,2 as those are 'special'. Certainly a '0' could be messing up somewhere in a test.

Hm, correction about the 0 - it makes no difference since the FDs still need to be iterated from the list of fakeDevices, unless we'd add a 'fakeFD' entry in there?

zagrodzki · 2024-07-22T13:54:33Z

libusb.go

+type libusbImpl struct {
+	logLevel         int
+	disableDiscovery bool
+	useUsbDK         bool


useUSBDK, or maybe useUSBDevKit?

zagrodzki · 2024-07-22T13:57:27Z

libusb.go

-type libusbImpl struct{}
+type libusbImpl struct {
+	logLevel         int
+	disableDiscovery bool


doouble negatives (disable: false) are bad for readability. Use "discovery bool" here, and we'll initialize it with "true" by default.

We obviously don't want the discovery to default to false though. See my other comment in the user-facing code.

Yes, good point. I wanted to keep the structure as is in the libusb options, but what you propose makes sense since gousb is supposed to be more 'go-like' rather than being just a libusb wrapper.

zagrodzki · 2024-07-22T14:05:49Z

libusb.go

+		copy(libusb_options[n_options].value[:], b)
+		n_options += 1
+	}
+	if impl.useUsbDK {


ditto here - I don't think this is needed for Android support? Move to a separate PR

zagrodzki · 2024-07-22T14:06:54Z

usb.c

@@ -24,3 +24,4 @@ void gousb_set_debug(libusb_context *ctx, int lvl) {
    libusb_set_debug(ctx, lvl);
 #endif
 }
+


zagrodzki · 2024-07-22T14:07:18Z

usb_test.go

@@ -15,7 +15,9 @@

 package gousb

-import "testing"
+import (
+	"testing"


revert this block

zagrodzki · 2024-07-22T14:08:11Z

usb_test.go

+		{0x7777, 0x0003, false},
+	} {
+		// The fake keeps the order of fakeDevices so that it can be indexed by a fake "fileDescriptor"
+		dev, err := ctx.OpenDeviceWithFileDescriptor(uintptr(ix))


this conflates the ordering of the test cases with the file descriptor values, I think it's fragile. See my other comment about a map for sys devices.

zagrodzki · 2024-07-22T14:25:16Z

usb_test.go

+		if (dev != nil) != d.exists {
+			t.Errorf("OpenDeviceWithFileDescriptor(%d): device != nil is %v, want %v", ix, dev != nil, d.exists)
+		}
+		if err != nil && d.exists { // It's OK to get an error if device doesn't exist


the behavior of OpenDeviceWithFileDescriptor creates a small divergence:
the OpenDeviceWithVIDPID returns nil, nil if the device is not found. Here the function will return an error if the device is not found. I think this is fine though - the Open...VIDPID should have returned an error for not found too.

But this change makes it necessary to change the structure of the test, you cannot use the same structure - I think you don't need to distinguish "exists" vs "not exists", instead you need to distinguish errors vs non-errors. And I think it may make sense to add a separate test case for a "want error", to simplify the code.

Well, since the fakeDevice can have an uninitialised sysDevPtr (e.g. because the system doesn't support that?) there still is a case where a device exists, but is 'not useful'/'not good'. Perhaps the word 'exists' should rather be 'good' or 'useful' ?
I will split it in two test cases, this will actually make it avoid the wording problem and indeed make the cases simpler.

kubaraczkowski · 2024-07-22T18:32:12Z

Thx @zagrodzki for the comments, please have a look at the update.

zagrodzki

also documentation please, I think this is good overall.

zagrodzki · 2024-07-22T18:40:17Z

fakelibusb_test.go

@@ -108,6 +110,21 @@ func (f *fakeLibusb) getDevices(*libusbContext) ([]*libusbDevice, error) {
 	return ret, nil
 }

+func (f *fakeLibusb) wrapSysDevice(ctx *libusbContext, systemDeviceHandle uintptr) (*libusbDevHandle, error) {
+	if _, ok := f.fakeSysDevices[systemDeviceHandle]; !ok {
+		return nil, fmt.Errorf("The passed file descriptor %d does not point to a valid device", systemDeviceHandle)


lowercase "the"

zagrodzki · 2024-07-22T18:41:26Z

fakelibusb_test.go

@@ -108,6 +110,21 @@ func (f *fakeLibusb) getDevices(*libusbContext) ([]*libusbDevice, error) {
 	return ret, nil
 }

+func (f *fakeLibusb) wrapSysDevice(ctx *libusbContext, systemDeviceHandle uintptr) (*libusbDevHandle, error) {
+	if _, ok := f.fakeSysDevices[systemDeviceHandle]; !ok {


I'd suggest:

dev, ok := f.fakeSysDevices[...]
if !ok {
return nil, fmt.Errorf(...)
}
...
f.handles[h] = dev

zagrodzki · 2024-07-22T18:56:46Z

usb.go

+func (c *Context) OpenDeviceWithFileDescriptor(fd uintptr) (*Device, error) {
+	handle, err := c.libusb.wrapSysDevice(c.ctx, fd)
+	if err != nil {
+		return nil, fmt.Errorf("Error opening device from file descriptor: %d, %s", fd, err.Error())


remove the word "Error" from the message, it's superfluous in a context of an error message. And "opening device from the file descriptor" seems unnecessary too, because the caller know what they just called. Better just return the error from libusb wrapper as is.

zagrodzki · 2024-07-22T19:00:13Z

usb.go

+
+	desc, err := c.libusb.getDeviceDesc(dev)
+	if err != nil {
+		return nil, err


if you want to annotate an error somewhere, here it would be appropriate perhaps:
fmt.Errorf("device was opened, but get device descriptor failed: %v")

zagrodzki · 2024-07-22T19:00:54Z

usb_test.go

+			t.Errorf("OpenDeviceWithFileDescriptor(%d): device == nil for a valid device", d.sysDevPtr)
+		}
+		if err != nil {
+			t.Errorf("OpenDeviceWithFileDescriptor(%d): err != nil for a valid device: %v", d.sysDevPtr, err)


Fatalf, and move before the dev == nil check.

zagrodzki · 2024-07-22T19:03:45Z

usb_test.go

+	} {
+		dev, err := ctx.OpenDeviceWithFileDescriptor(d.sysDevPtr)
+		if dev == nil {
+			t.Errorf("OpenDeviceWithFileDescriptor(%d): device == nil for a valid device", d.sysDevPtr)


Fatalf.

Also add a check to see if the obtained device is the one you asked for (does VID/PID match).

these two above (line 112 and 115) still need to be Fatalf.

With Fatal(f), the test is immediately interrupted. With Error(f), the test continues, but will report a failure at the end with all recorded errors. The principle is "return as much useful test results from a single run as possible".

Hence:

if err is not nil, dev is undefined and no further testing can be done (Fatal)

if dev is nil, there's no device, testing properties of the device makes no sense (Fatal)

if dev is not nil and err is nil, then dev may have useful properties. Checking these properties likely doesn't affect whether further testing can be done (Error), unless the code after that check has a hard dependency on the data that the previous check validated.

For future reference: if there's enough distinct check flows that each check forms a chain of dependencies, it may make sense to split individual checks into a subtest (t.Run).

zagrodzki · 2024-07-22T19:05:25Z

usb_test.go

+		if err == nil {
+			t.Errorf("OpenDeviceWithFileDescriptor(%d): got nil error for invalid device", sysDevPtr)
+		}
+		if dev != nil {


drop this check - it's enough to check that error was non-nil. The return value is undefined, it may be non-nil, but it shouldn't be used either way.

zagrodzki · 2024-07-22T19:05:47Z

libusb.go

@@ -40,6 +40,7 @@ type libusbDevice C.libusb_device
 type libusbDevHandle C.libusb_device_handle
 type libusbTransfer C.struct_libusb_transfer
 type libusbEndpoint C.struct_libusb_endpoint_descriptor
+type libusbLogLevel C.enum_libusb_log_level


drop (for later PR)

zagrodzki · 2024-07-22T19:07:09Z

fakelibusb_test.go

@@ -87,6 +87,8 @@ type fakeLibusb struct {
 	mu sync.Mutex
 	// fakeDevices has a map of devices and their descriptors.
 	fakeDevices map[*libusbDevice]*fakeDevice
+	// fakeSysDevices keeps the order of devices to be accessd by wrapSysDevice
+	fakeSysDevices map[uintptr]*libusbDevice


actually, perhaps just "sysDevices" - this is already inside the fakeLibusb, so "fake" stutters a bit. "fakeDevices" is ok because the type is called "fakeDevice", although perhaps it should also be renamed "devices"...

You're the boss, but I'd argue that either both should be changed or neither - it's much more readable to see these two named in the same way. You immediately go "ah, right, these are just some mappings of the fakes"

I'd go with "both" then :)

I understand this as "change both" :)
Will update

zagrodzki · 2024-07-22T19:08:56Z

usb.go

@@ -168,6 +168,23 @@ func NewContext() *Context {
 	return newContextWithImpl(libusbImpl{})


would it make sense to replace this with "return ContextOptions{}.New()"? It's now becoming essentially a "context with defaults" anyway.

I agree - it also matches better the 'new' libusb initializer, since the int libusb_init(libusb_context ctx) is deprecated in favour of always calling it with options.

…based on comments

kubaraczkowski · 2024-07-24T20:58:41Z

Updated! Thx for detailed and clear feedback.

zagrodzki · 2024-07-26T22:41:46Z

usb.go

+	return ContextOptions{}.New()
+}
+
+// DeviceDiscovery enables a scan of the available USB devices connected to the system


Add a "Defaults to enabled". Also, full sentences, end with a period.

zagrodzki · 2024-07-26T22:41:59Z

usb.go

@@ -165,7 +165,27 @@ func newContextWithImpl(impl libusbIntf) *Context {

 // NewContext returns a new Context instance.


...with default ContextOptions

zagrodzki · 2024-07-26T22:43:05Z

usb.go

+	DisableDeviceDiscovery
+)
+
+// ContextOptions holds optional flags checked when creating a new Context


"optional flags" is too specific, since we might add more parameters to the context creation that do not easily translate into options in libusb init.

Perhaps:
ContextOptions holds parameters for Context initialization.

Good point. I always need to remind myself - gousb is not a libusb wrapper. Not only a wrapper at least.

Anything still missing here?

usb.go

zagrodzki · 2024-07-26T22:54:07Z

usb.go

@@ -210,6 +230,38 @@ func (c *Context) OpenDevices(opener func(desc *DeviceDesc) bool) ([]*Device, er
 	return ret, reterr
 }

+// OpenDeviceWithFileDescriptor takes a (Unix) file descriptor of an opened USB device
+// and wraps the library around it.
+// This is particularly useful when working on Android, where the USB device must be opened


perhaps "is opened" rather than "must be"? I suspect that Android NDK may offer a similar functionality, it's unlikely the "must be opened in Java SDK" is going to be true forever.

It actually sucks a bit, but unfortunately NDK is not able to open USB devices. Yet?
TBH I thought that the 'must be' would be a nudge for the users to realise that they will need the Java SDK to acquire permissions and open the device.
But you're correct, it's entirely possible that the NDK limitation will be lifted at some point and this docstring will become invalid.

I will change it to "can be opened"

usb.go

zagrodzki · 2024-07-26T22:55:15Z

usb.go

+	}
+
+	dev := c.libusb.getDevice(handle)
+


drop this empty line

I think it's already in place

I meant remove the empty line (i.e. remove line 262 between the dev assignment and desc assignment)

usb_test.go

libusb.go

kubaraczkowski · 2024-07-27T22:06:01Z

Adjusted! Thx for the suggestions.

usb_test.go

zagrodzki · 2024-08-16T16:22:31Z

any updates here?

kubaraczkowski · 2024-08-17T19:59:06Z

Just back from holidays, so it will move in a few days :)

kubaraczkowski · 2024-09-07T09:32:43Z

Sorry for the delay, restarting work after holidays took longer than expected :)
Something more to be done? Thx for the help!

zagrodzki

there are still some comments open

zagrodzki · 2024-09-07T10:19:25Z

usb_test.go

+	} {
+		dev, err := ctx.OpenDeviceWithFileDescriptor(d.sysDevPtr)
+		if dev == nil {
+			t.Errorf("OpenDeviceWithFileDescriptor(%d): device == nil for a valid device", d.sysDevPtr)


these two above (line 112 and 115) still need to be Fatalf.

With Fatal(f), the test is immediately interrupted. With Error(f), the test continues, but will report a failure at the end with all recorded errors. The principle is "return as much useful test results from a single run as possible".

Hence:

if err is not nil, dev is undefined and no further testing can be done (Fatal)

if dev is nil, there's no device, testing properties of the device makes no sense (Fatal)

if dev is not nil and err is nil, then dev may have useful properties. Checking these properties likely doesn't affect whether further testing can be done (Error), unless the code after that check has a hard dependency on the data that the previous check validated.

For future reference: if there's enough distinct check flows that each check forms a chain of dependencies, it may make sense to split individual checks into a subtest (t.Run).

usb.go

zagrodzki · 2024-09-07T10:20:31Z

usb.go

+	}
+
+	dev := c.libusb.getDevice(handle)
+


zagrodzki · 2024-09-07T10:23:34Z

usb.go

+// OpenDeviceWithFileDescriptor takes a (Unix) file descriptor of an opened USB device
+// and wraps the library around it.
+// This is particularly useful when working on Android, where the USB device must be opened
+// by the SDK (Java), giving access to the device through the file descriptor (https://developer.android.com/reference/android/hardware/usb/UsbDeviceConnection#getFileDescriptor()).


usb.go

kubaraczkowski · 2024-09-07T20:18:25Z

Sorry for missing a few points.

Thx for the explanation of Errorf and Fatalf.
I think all comments are now resolved, not 100% sure. I put 👍 on everything I've checked.

zagrodzki

only one minor nitpicky comment left

zagrodzki · 2024-09-08T15:26:12Z

usb.go

+	}
+
+	dev := c.libusb.getDevice(handle)
+


I meant remove the empty line (i.e. remove line 262 between the dev assignment and desc assignment)

zagrodzki · 2024-09-08T16:23:38Z

a linux build is failing due to too old libusb version (1.0.25 in the build env). I wonder if we should support the older versions, but I think it would get complicated. I'll just update the minor version at the release time. In the meantime, I need to update the build env, stay tuned.

zagrodzki · 2024-09-08T16:33:49Z

a linux build is failing due to too old libusb version (1.0.25 in the build env). I wonder if we should support the older versions, but I think it would get complicated. I'll just update the minor version at the release time. In the meantime, I need to update the build env, stay tuned.

Upgraded Linux build env to ubuntu 22.04, rebase and we should be good.

kubaraczkowski · 2024-09-08T19:24:41Z

Any hints on how you would approach keeping it work for ubuntu 20.04 while enabling the additional functionality on 22+ ?
I'm just curious how this could be done, if required.

It seems the feature came out in 2019 and should already be present in 20.04 (1.0.23+), not sure though. 20.04 is still being used, at least EOL is 2025.

zagrodzki · 2024-09-08T19:37:56Z

Any hints on how you would approach keeping it work for ubuntu 20.04 while enabling the additional functionality on 22+ ? I'm just curious how this could be done, if required.

I think I would go with compiler conditionals in the C code, adding a replacement definition of the libusb_init_context if the libusb doesn't provide it (#if (LIBUSB_API_VERSION < ...)). And it can inspect the options and if they are non-default, return a "not supported" error.

Alternatively, the libusb version could be inspected from the Go code, so perhaps the C implementation can be trivial (just to appease the compiler, always return a not supported error) and the Go code can conditionally call either old-style init (if old libusb and init called with default options), or fail (old libusb and non-default options), or libusb_init_context if supported.

Both of these would keep working for the existing users, until someone tries to use ContextOptions{} to change some defaults.

It seems the feature came out in 2019 and should already be present in 20.04 (1.0.23+), not sure though. 20.04 is still being used, at least EOL is 2025.

https://libusb.sourceforge.io/api-1.0/group__libusb__lib.html#gaabdf0bf6be87e12023b7399003c50470 says for libusb_init_context:

Since version 1.0.27, LIBUSB_API_VERSION >= 0x0100010A

1.0.27 came out Feb 1st this year.

zagrodzki · 2024-09-08T19:41:24Z

thank you for your patience during the process! I'll see if I can get any of the other pending pull requests in before I release a new version, I plan to do that in ~2wks.

kubaraczkowski · 2024-09-08T19:43:58Z

Hey, nice!

I was looking at only libusb_wrap_sys_device() which is the base of android support.
https://libusb.sourceforge.io/api-1.0/group__libusb__dev.html#ga98f783e115ceff4eaf88a60e6439563c

OK, I'm sure somebody using gousb on an older system will complain if needed!

Thanks for the great support!

juaoose and others added 6 commits May 13, 2024 17:32

Add OpenDeviceWithFileDescriptor

e3b240a

Add option to disable device discovery

3900892

opts

Add missing cgo call

25dfbc2

Update to use fd int instead of path

beb6261

improved based on suggestions from google#126 and updated to the now-…

b6f20af

…recommended libusb context creation method

added test for opening by file descriptor

f7bafd7

kubaraczkowski mentioned this pull request Jul 19, 2024

Add functions to achieve android support #126

Closed

zagrodzki requested changes Jul 22, 2024

View reviewed changes

kubaraczkowski-spectricity added 2 commits July 22, 2024 20:28

adjusted according to comments on PR#130

d7d6cb5

split off context options unnecessary for android support

3b9530c

zagrodzki requested changes Jul 22, 2024

View reviewed changes

added documentation regarding OpenDeviceWithFileDescriptor, adjusted …

c0bdd87

…based on comments

zagrodzki requested changes Jul 26, 2024

View reviewed changes

zagrodzki requested changes Jul 27, 2024

View reviewed changes

libusb.go Outdated Show resolved Hide resolved

libusb.go Outdated Show resolved Hide resolved

libusb.go Outdated Show resolved Hide resolved

fixes from PR

40a2d32

zagrodzki reviewed Jul 28, 2024

View reviewed changes

usb_test.go Outdated Show resolved Hide resolved

changed Fatalf to Errorf in an 'if'-chain

c1f9177

zagrodzki requested changes Sep 7, 2024

View reviewed changes

fixed according to PR comments

c4e0d59

zagrodzki requested changes Sep 8, 2024

View reviewed changes

remove one empty line

be696ef

zagrodzki approved these changes Sep 8, 2024

View reviewed changes

zagrodzki merged commit 606016a into google:master Sep 8, 2024
4 checks passed

		@@ -168,6 +168,23 @@ func NewContext() *Context {
		return newContextWithImpl(libusbImpl{})

		@@ -165,7 +165,27 @@ func newContextWithImpl(impl libusbIntf) *Context {

		// NewContext returns a new Context instance.

Android support via fileDescriptor #130

Android support via fileDescriptor #130

Conversation

kubaraczkowski commented Jul 19, 2024

google-cla bot commented Jul 19, 2024

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

kubaraczkowski commented Jul 22, 2024

zagrodzki 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

kubaraczkowski commented Jul 24, 2024

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

kubaraczkowski commented Jul 27, 2024

zagrodzki commented Aug 16, 2024

kubaraczkowski commented Aug 17, 2024

kubaraczkowski commented Sep 7, 2024

zagrodzki 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

kubaraczkowski commented Sep 7, 2024

zagrodzki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zagrodzki commented Sep 8, 2024

zagrodzki commented Sep 8, 2024

kubaraczkowski commented Sep 8, 2024

zagrodzki commented Sep 8, 2024

zagrodzki commented Sep 8, 2024

kubaraczkowski commented Sep 8, 2024