-
Notifications
You must be signed in to change notification settings - Fork 133
Conversation
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
commented
Jun 9, 2023
I think this change requires more explanations. When is this needed? Why doesn't the current implementation work for your use case? Could your use case be solved without adding new methods?
ARizzo35
commented
Jun 9, 2023
I think this change requires more explanations. When is this needed? Why doesn't the current implementation work for your use case? Could your use case be solved without adding new methods?
I used this library to interact with a FTDI chip over USB, specifically using their MPSSE. In order to use the USB device in this mode, on one specific interface (the chip has multiple interfaces), the standard linux kernel USB serial driver for FTDI has to be unloaded first.
The libusb auto-detach implementation handles unloading the kernel driver for a specific interface properly so that it can be claimed. In this gousb library, extra functionality is added in the implementation of SetAutoDetach() which sets a flag to detach the kernel driver for all interfaces on the device when the config is changed, which is not normal behavior in libusb.
This added function only calls the libusb auto-detach binding, and does not set the autodetach flag in the device class.
An extra boolean parameter could be added to the current implementation of SetAutoDetach() to handle this as well, but that would be less backwards compatible since Golang does not support optional function parameters with default values.
zagrodzki
commented
Jun 12, 2023
if I understand correctly, you're saying that just setting the libusb option is sufficient and the kernel will automatically detach whatever interfaces need to be claimed in whatever config is currently loaded? Ok. I wonder if the current behavior is then something that is actually required, or was the code unnecessarily overzealous in calling the detach function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not add a new method. We can fit this into the current function though, using variadic options:
// DetachOption allow fine-tuning of the SetAutoDetach behavior.
type DetachOption interface {
update(d *Device)
}
// DetachAllInterfaces is a DetachOption that controls the behavior of Config(). If SetAutoDetach is called
// with DetachAllInterfaces(true), subsequent calls to Config() will proactively detach all interfaces of the
// specified device configuration.
// If SetAutoDetach is called with DetachAllInterfaces(bool), interfaces will not be explicitly detached and
// only the kernel auto-detach is enabled.
// The default behavior is to detach all interfaces if autodetach is enabled and
// not detach all interfaces if autodetach is disabled.
type DetachAllInterfaces bool
func (val DetachAllInterfaces) update(d *Device) {
d.autodetach = bool(val)
}
func (d *Device) SetAutoDetach(autodetach bool, opts ...DetachOption) error {
if d.handle == nil { ... }
d.autodetach = autodetach
for _, o := range opts {
o.update(d)
}
...
...setAutoDetach(...)
}
Also perhaps add a TODO to offer a better interface for the next major release - autodetach should only set autodetach, while controlling the interfaces explicitly should be done through another option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and then your use case is represented as:
d.SetAutoDetach(true, DetachAllInterfaces(false))
No description provided.