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

Android support via fileDescriptor #130

Merged
merged 13 commits into from
Sep 8, 2024
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions fakelibusb_devices.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,10 @@ package gousb

// fake devices connected through the fakeLibusb stack.
type fakeDevice struct {
devDesc *DeviceDesc
strDesc map[int]string
alt uint8
devDesc *DeviceDesc
strDesc map[int]string
alt uint8
sysDevPtr uintptr
}

var fakeDevices = []fakeDevice{
Expand Down Expand Up @@ -64,6 +65,7 @@ var fakeDevices = []fakeDevice{
}},
}},
},
sysDevPtr: 78,
},
// Bus 001 Device 002: ID 8888:0002
// One config, two interfaces. interface #0 with no endpoints,
Expand Down Expand Up @@ -186,6 +188,7 @@ var fakeDevices = []fakeDevice{
8: "Slower streaming",
9: "Interface for https://github.com/google/gousb/issues/65",
},
sysDevPtr: 94,
},
// Bus 001 Device 003: ID 9999:0002
// One config, one interface, one setup,
Expand Down
34 changes: 28 additions & 6 deletions fakelibusb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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"

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd go with "both" 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.

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

// ts has a map of all allocated transfers, indexed by the pointer of
// underlying libusbTransfer.
ts map[*libusbTransfer]*fakeTransfer
Expand All @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest:

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

return nil, fmt.Errorf("The passed file descriptor %d does not point to a valid device", systemDeviceHandle)
Copy link
Collaborator

Choose a reason for hiding this comment

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

lowercase "the"

}
h := newDevHandlePointer()
f.mu.Lock()
defer f.mu.Unlock()
f.handles[h] = f.fakeSysDevices[systemDeviceHandle]
return h, nil
}

func (f *fakeLibusb) getDevice(handle *libusbDevHandle) *libusbDevice {
return f.handles[handle]
}

func (f *fakeLibusb) exit(*libusbContext) error {
close(f.submitted)
if got := len(f.ts); got > 0 {
Expand Down Expand Up @@ -288,11 +305,12 @@ func (f *fakeLibusb) empty() bool {

func newFakeLibusb() *fakeLibusb {
fl := &fakeLibusb{
fakeDevices: make(map[*libusbDevice]*fakeDevice),
ts: make(map[*libusbTransfer]*fakeTransfer),
submitted: make(chan *fakeTransfer, 10),
handles: make(map[*libusbDevHandle]*libusbDevice),
claims: make(map[*libusbDevice]map[uint8]bool),
fakeDevices: make(map[*libusbDevice]*fakeDevice),
fakeSysDevices: make(map[uintptr]*libusbDevice),
ts: make(map[*libusbTransfer]*fakeTransfer),
submitted: make(chan *fakeTransfer, 10),
handles: make(map[*libusbDevHandle]*libusbDevice),
claims: make(map[*libusbDevice]map[uint8]bool),
}
for _, d := range fakeDevices {
// libusb does not export a way to allocate a new libusb_device struct
Expand All @@ -301,7 +319,11 @@ func newFakeLibusb() *fakeLibusb {
// The contents of these pointers is never accessed.
fd := new(fakeDevice)
*fd = d
fl.fakeDevices[newDevicePointer()] = fd
devPointer := newDevicePointer()
fl.fakeDevices[devPointer] = fd
if fd.sysDevPtr != 0 { // the sysDevPtr not being set in the fakeDevices list
fl.fakeSysDevices[fd.sysDevPtr] = devPointer
}
}
return fl
}
34 changes: 31 additions & 3 deletions libusb.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

drop (for later PR)


func (ep libusbEndpoint) endpointDesc(dev *DeviceDesc) EndpointDesc {
ei := EndpointDesc{
Expand Down Expand Up @@ -143,6 +144,7 @@ type libusbIntf interface {
dereference(*libusbDevice)
getDeviceDesc(*libusbDevice) (*DeviceDesc, error)
open(*libusbDevice) (*libusbDevHandle, error)
wrapSysDevice(*libusbContext, uintptr) (*libusbDevHandle, error)

close(*libusbDevHandle)
reset(*libusbDevHandle) error
Expand All @@ -152,6 +154,7 @@ type libusbIntf interface {
getStringDesc(*libusbDevHandle, int) (string, error)
setAutoDetach(*libusbDevHandle, int) error
detachKernelDriver(*libusbDevHandle, uint8) error
getDevice(*libusbDevHandle) *libusbDevice

// interface
claim(*libusbDevHandle, uint8) error
Expand All @@ -169,13 +172,24 @@ type libusbIntf interface {
}

// libusbImpl is an implementation of libusbIntf using real CGo-wrapped libusb.
type libusbImpl struct{}
type libusbImpl struct {
discovery DeviceDiscovery
}

func (libusbImpl) init() (*libusbContext, error) {
func (impl libusbImpl) init() (*libusbContext, error) {
var ctx *C.libusb_context
if err := fromErrNo(C.libusb_init(&ctx)); err != nil {

var libusb_options [4]C.struct_libusb_init_option // fixed to 4 - there are maximum 4 options
zagrodzki marked this conversation as resolved.
Show resolved Hide resolved
n_options := 0
zagrodzki marked this conversation as resolved.
Show resolved Hide resolved
if impl.discovery == DisableDeviceDiscovery {
libusb_options[n_options].option = C.LIBUSB_OPTION_NO_DEVICE_DISCOVERY
n_options += 1
zagrodzki marked this conversation as resolved.
Show resolved Hide resolved
}

if err := fromErrNo(C.libusb_init_context(&ctx, &(libusb_options[0]), C.int(n_options))); err != nil {
return nil, err
}

return (*libusbContext)(ctx), nil
}

Expand Down Expand Up @@ -217,6 +231,20 @@ func (libusbImpl) getDevices(ctx *libusbContext) ([]*libusbDevice, error) {
return ret, nil
}

func (libusbImpl) wrapSysDevice(ctx *libusbContext, fd uintptr) (*libusbDevHandle, error) {
var handle *C.libusb_device_handle
if ret := C.libusb_wrap_sys_device((*C.libusb_context)(ctx), C.intptr_t(fd), &handle); ret < 0 {
return nil, fromErrNo(C.int(ret))
}

return (*libusbDevHandle)(handle), nil
}

func (libusbImpl) getDevice(d *libusbDevHandle) *libusbDevice {
device := C.libusb_get_device((*C.libusb_device_handle)(d))
return (*libusbDevice)(device)
}

func (libusbImpl) exit(c *libusbContext) error {
C.libusb_exit((*C.libusb_context)(c))
return nil
Expand Down
38 changes: 38 additions & 0 deletions usb.go
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,23 @@ func NewContext() *Context {
return newContextWithImpl(libusbImpl{})
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}

type DeviceDiscovery int

const (
EnableDeviceDiscovery = iota
DisableDeviceDiscovery
)

type ContextOptions struct {
DeviceDiscovery DeviceDiscovery
zagrodzki marked this conversation as resolved.
Show resolved Hide resolved
}

func (o ContextOptions) New() *Context {
return newContextWithImpl(libusbImpl{
discovery: o.DeviceDiscovery,
zagrodzki marked this conversation as resolved.
Show resolved Hide resolved
})
}

// OpenDevices calls opener with each enumerated device.
// If the opener returns true, the device is opened and a Device is returned if the operation succeeds.
// Every Device returned (whether an error is also returned or not) must be closed.
Expand Down Expand Up @@ -210,6 +227,27 @@ func (c *Context) OpenDevices(opener func(desc *DeviceDesc) bool) ([]*Device, er
return ret, reterr
}

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())
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}

dev := c.libusb.getDevice(handle)

Copy link
Collaborator

Choose a reason for hiding this comment

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

drop this empty line

Copy link
Collaborator

Choose a reason for hiding this comment

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

still open

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's already in place

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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")

}

o := &Device{handle: handle, ctx: c, Desc: desc}
c.mu.Lock()
c.devices[o] = true
c.mu.Unlock()

return o, nil
}

// OpenDeviceWithVIDPID opens Device from specific VendorId and ProductId.
// If none is found, it returns nil and nil error. If there are multiple devices
// with the same VID/PID, it will return one of them, picked arbitrarily.
Expand Down
42 changes: 42 additions & 0 deletions usb_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,45 @@ func TestOpenDeviceWithVIDPID(t *testing.T) {
}
}
}

func TestOpenDeviceWithFileDescriptor(t *testing.T) {
ctx := newContextWithImpl(newFakeLibusb())
defer ctx.Close()

// file descriptor is an index to the FakeDevices array
for _, d := range []struct {
vid, pid ID
sysDevPtr uintptr
}{
{0x9999, 0x0001, 78},
{0x8888, 0x0002, 94},
} {
dev, err := ctx.OpenDeviceWithFileDescriptor(d.sysDevPtr)
if dev == nil {
t.Errorf("OpenDeviceWithFileDescriptor(%d): device == nil for a valid device", d.sysDevPtr)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Fatalf.

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

}
}

}

func TestOpenDeviceWithFileDescriptorOnMissingDevice(t *testing.T) {
ctx := newContextWithImpl(newFakeLibusb())
defer ctx.Close()

for _, sysDevPtr := range []uintptr{
7, // set, but does not exist in the fakeDevices array
0, // unset
} {
dev, err := ctx.OpenDeviceWithFileDescriptor(sysDevPtr)
if err == nil {
t.Errorf("OpenDeviceWithFileDescriptor(%d): got nil error for invalid device", sysDevPtr)
}
if dev != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

t.Errorf("OpenDeviceWithFileDescriptor(%d): got non-nil device for invalid device", sysDevPtr)
}
}

}