Hot Plug Support

[bruce]

Any interest in the participants from kylelemons/gousb#9 (@zagrodzki , @nkovacs, et al) reviving the work on the hotplug event support in this version?

(My own golang/c skills are woefully inadequate to help in anything much besides testing and moral support, but I would be very interested in this feature for a project I'm flailing around with.)

[nkovacs]

I'm sorry, I haven't had time for this, and my old fork works well enough for the project where I needed this. I'll probably take another stab at it when I have to update that project, but no promises.

[nkovacs]

I've started working on this: https://github.com/nkovacs/gousb/tree/hotplug

Because the libusb function has a bunch of optional parameters, I've decided to use a fluent api:

cb := ctx.Hotplug().Enumerate().ProductID(42).Register(func(event gousb.HotplugEvent) {
    ...
})

Another remaining issue is how to handle deregistering a callback inside the callback. This might be useful if you want to have a one-off event. Although you could call the returned object's Deregister function in the callback (cb.Deregister()), there's a race condition there, because the cb object might not exist when the callback is first called. Plus you would have to write your code like this:

var cb gousb.HotplugCallback

cb = ctx.Hotplug().Enumerate().ProductID(42).Register(func(event gousb.HotplugEvent) {
    cb.Deregister()
})

Which is unwieldy (and, of course, racy). Putting a method onto HotplugEvent could work, though the other methods must not be called after the callback function returns (I think that's how libusb works, you're probably not supposed to hang on to the parameters passed in to the callback function), and implementing it correctly is a bit hard.
The libusb callback can return true to deregister the event, and that's how I implemented it before, but that's also confusing, and I don't like forcing you to write a return in the callback when that's not a common use case (see kylelemons/gousb#9 (comment)). Also, it doesn't work correctly, because I have to clean up the map, or it'll leak memory (the previous version was also racy).

[zagrodzki]

Thanks. I've added some minor comments at the first pass, but to be honest, I already completely forgot how all this works.

What would be the purpose of the callback? apart from deregistering the handler. If there's no other use for it, then perhaps we could have something like this:

h := ctx.Hotplug(func(sameH *gousb.Hotplug, ev gousb.HotplugEvent) {
  ...
  sameH.Cancel()
}, ...<options>...)
...
h.Cancel()

Ideally, to get me back on track, would you mind preparing a few examples of how the new API would be used for different cases?

[nkovacs]

My use case is to listen forever for any device, both leave and arrive, so it would look like this:

ctx.Hotplug.Enumerate().Register(func() {
...
})
// ctx.Close will clean up everything

If you're only interested when one device leaves (you've opened it, but you want to know when it goes away):

id := uint16(desc.Bus) + 255 + uint16(desc.Address)
ctx.Hotplug.Left().Register(func(event gousb.HotplugEvent) {
    desc, _ := event.DeviceDesc()
    leftID := uint16(desc.Bus) + 255 + uint16(desc.Address)
    if id == leftID {
        // do some cleanup now that the device has left
        // deregister the hotplug callback somehow
    }
})

Though it would probably be better to register a single "left" callback instead of a per-device one.

If you only care about a specific type of device:

ctx.Hotplug.ProductID(0x1234).VendorID(0x1234).Enumerate().Register(func() {
...
})

Now that I thought about it, maybe productID and vendorID filtering is not necessary.
It doesn't allow you to register the same callback for multiple products, for example: PID: 18d1, VID: 2d00 is Android in accessory mode, but if you have ADB enabled, the VID changes to 2d01, so you would have to register the same function as two separate callbacks.
And you can just use an if inside the callback. The same goes for device class. So maybe the whole thing can be simplified to something like this:

RegisterHotplug(cb func(HotplugEvent), enumerate bool)
RegisterHotplugArrived(cb func(HotplugEvent), enumerate bool)
// enumerate doesn't make sense here
RegisterHotplugLeft(cb func(HotplugEvent))

Also, I found this in libusb's source code: https://github.com/libusb/libusb/blob/6402a10c145d1a38964fc43009d115fe7a86c8c7/libusb/hotplug.c#L62
This wasn't documented in the api doc.

[kylelemons]

Just as a drive-by comment, I tend to prefer the variadic options approach rather than the fluent/builder approach in Go.

You can kinda combine them a bit and let people find the balance that works for them too.

I would be kinda tempted to make it so that you enable hotplug and the Go library receives all events so that you can build a more useful and powerful API on top of it, like:

hp := ctx.NewHotplug().Match(hotplug.Enter())
for _, product := range products {
  hp.MatchAll(hotplug.Vendor(product.VendorID), hotplug.Product(product.ProductID))
}
hp.MatchAny(hotplug.Class(usb.HID), hotplug.Class(usb.MSC), hotplug.Class(usb.UMS))
hp.Callback(func(ev *hotplug.Event) { ... })
hp.CallbackOnce(func(ev *hotplug.Event) { ... })

[nkovacs]

What is hotplug in your example? A separate package?

Also, the MatchAny/MatchAll functions wouldn't work like that, libusb doesn't support matching like that. We'd have to reimplement matching in gousb.

[kylelemons]

Yes, hotplug was a separate package (to avoid usb.HotplugFoo for everything) and yeah, that was the point of the illustration. It might also very naturally solve the unregistration problem if you only have to worry about a single call back that is enabled and disabled via a top level function and that call back hands off to a dispatcher that understands a more natural set of filters and Go idioms.

[zagrodzki]

I'm not actually sure if we need that complex logic. Thinking some more about it, why do we need to do some special filter classes in the callback, if we could just let the user match any event directly, using Go instead of our invented matcher syntax. I generally agree with Kyle's sentiment that perhaps we should just pass all the hotplug events, I'd be up for that.

So I could imagine something as simple as this:

func (Context) RegisterHotplug(func(ev *HotplugEvent)) cancel func()

type HotplugEvent struct {
  Type HotplugEventType
  DeviceDesc *DeviceDesc
}

func (HotplugEvent) Open() (*Device, error)
func (HotplugEvent) Cancel()

HotplugEvent.Cancel does the same thing as the cancel closure returned by RegisterHotplug.

Everything related to matching can be done by the user based on the ev.DeviceDesc.

The cases mentioned by @nkovacs above would then be expressed as:

listen for all enumerated, arriving and leaving devices:

ctx.RegisterHotplug(func(ev *gousb.HotplugEvent) {
  ...
})

One particular device:

bus, addr := desc.Bus, desc.Address
ctx.RegisterHotplug(func(ev *gosusb.HotplugEvent)) {
  if ev.DeviceDesc.Bus != bus || ev.DeviceDesc.Addr != addr || ev.Type != gousb.HotplugLeft {
    return
  }
  // do some cleanup now that the device has left
  ev.Cancel()
})

WDYT?

[nkovacs]

@zagrodzki What about enumerate?

I don't know if it's a use case we should care about, but with your suggestion, we cannot leave out already plugged in devices, since the callback doesn't receive this information.

That's why I suggested this:

RegisterHotplug(cb func(HotplugEvent), enumerate bool)
RegisterHotplugArrived(cb func(HotplugEvent), enumerate bool)
// enumerate doesn't make sense here
RegisterHotplugLeft(cb func(HotplugEvent))

[zagrodzki]

I think not supporting it is a better option. I think the enumerate flag is poorly designed. Looking at the code, the only thing it does is an equivalent of OpenDevices (libusb_get_device_list underneath) after the callback has been registered. I don't think it makes sense to reuse that part of the implementation.

So I think it might be enough to first use RegisterHotplug, then call OpenDevices in the code. If it later turns out the enumerate behavior is valuable, I'd be up for reimplementing it on top of libusb, with a dedicated gousb event type "enumerate", instead of ambiguous "arrived" for both new arrival and enumerated.

[nkovacs]

While I agree with you that it's bad that the two overlap, I disagree with removing it. For my use case, enumerate is very convenient because I don't care if the device was plugged in before my program started, and I think this is the more common use case.

It also avoids a race condition (device gets plugged in after RegisterHotplug but before OpenDevices).

[zagrodzki]

The enumerate documentation states explicitly that it doesn't avoid the race:

This means that when using LIBUSB_HOTPLUG_ENUMERATE, your callback may be called twice for the arrival of the same device, once from libusb_hotplug_register_callback() and once from libusb_handle_events(); and/or your callback may be called for the removal of a device for which an arrived call was never made.

http://libusb.sourceforge.net/api-1.0/group__hotplug.html#gae6c5f1add6cc754005549c7259dc35ea

I can agree on the convenience factor if you think that's useful. I'd be fine with adding an equivalent of enumerate controlled by Go code. I think then it would make sense to have three event types (arrive/leave/enumerate) to let the users decide if they want to use enumeration or not (with a simple if in the callback). I.e. have enumerate always enabled. We could then also properly handle the "stop callback" signal regardless of whether it arrives from enumeration or hotplug event, since we'd do that in Go code anyway.

[nkovacs]

Oh, that's unfortunate.

I'm not sure about making enumerate a separate event type. It's just an arrive event, your code is probably going to handle it the same way. And I'm struggling to come up with a use case where you would not want to use enumerate.

[zagrodzki]

I'm not sure. libusb defaults to "not enumerate", but I don't know if that's just because legacy API was not convenient enough and they needed to preserve compatibility, or because there are cases where you don't care about enumeration. Still, making a distinction between arrive and enumerate seems trivial for us and easy for the users to understand and use, while libusb's behavior makes it impossible to make a distinction. There's also the "cancel registration" which in libusb works differently depending on whether it's a callback or enumeration, which I find frustrating.

I'd rather preserve the capability to differentiate than to loose it irreversibly up front. So let's make separate event types for "arrived" and separate for "enumerated" and let's mention explicitly that the libusb restriction holds - you might get "arrived" and "enumerated" for the same device, and you may get "left" for a device that was never "arrived" or "enumerated".

[nkovacs]

If we implement the enumerate in Go code, we can handle cancellation correctly.
We could also deal with the race condition, but that feels like it's too much work for something that's easy enough to handle on the callback's side (i.e. check if you already have the device, ignore the duplicate "arrived"/"enumerated" and don't do anything for "left" if you don't have the device).

There's an old issue about refactoring libusb's hotplug API: libusb/libusb#29
It makes a good point about your callback being split into two with an if.

With the enumerate event, your callback will almost always look like this:

if (event.Type == gousb.HotplugEventDeviceArrived || event.Type == gousb.HotplugEventDeviceEnumerated) {
    // handle device arrived
} else {
    // handle device left
}

Maybe separate callbacks, one for left and one for arrived (with enumerate being a bool on the event object for the "arrived" event, so you can ignore it if you don't care, which I think is the common use case) would be better?

Another thing I noticed now that I looked at libusb's source: libusb_handle_events runs in a separate goroutine, and that will trigger the callback, but enumerate can trigger the callback on the goroutine calling register. Although libusb has mutexes for the callbacks, the callback functions themselves can be called from two separate goroutines, so you would have to use locking in your callbacks. That's probably another thing we should fix, but I'm not sure how.