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

allow only libusb auto detach #117

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ARizzo35
Copy link

@ARizzo35 ARizzo35 commented Jun 9, 2023

No description provided.

@google-cla
Copy link

google-cla bot commented Jun 9, 2023

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

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
Copy link
Author

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

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?

// SetAutoDetachLibOnly is similar to SetAutoDetach, except it will not
// flag the Device instance to detach all interfaces when changing Configs.
// This will only call the libusb implementation of auto detach.
func (d *Device) SetAutoDetachLibOnly(autodetach bool) error {
Copy link
Collaborator

@zagrodzki zagrodzki Jun 12, 2023

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.

Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants