Skip to content

Add unknown interface descriptors to Extra #127

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.

Sign up for GitHub

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

Open
juaoose wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Add unknown interface descriptors to Extra #127

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb19350
Add unknown interface descriptors to Extra
juaoose May 15, 2024
965a497
Update after comments
juaoose May 17, 2024
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
2 changes: 2 additions & 0 deletions interface.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ type InterfaceSetting struct {
// Endpoints enumerates the endpoints available on this interface with
// this alternate setting.
Endpoints map[EndpointAddress]EndpointDesc
// Unknown interface descriptors

@zagrodzki zagrodzki May 16, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this is sufficient.

First, the "extras" can be encountered in other types of descriptors, not just interface descriptors: config and endpoint descriptors also have the extra field. Why limit to interface only?

Second, I don't think this trivial format of []byte is a good one for this library and the most useful one for the user. The descriptors stored in libusb's extras are still valid descriptors, just of unknown/unsupported type. If you want to make them available to the users, I tink a []RawDescriptor would be a minimum:

 type RawDescriptor {
  Type DescriptorType
  Data []byte
}

Third, I expect the documentation to explain more clearly to the user what this data is and how it can be used. Just saying "unknown descriptors" and "extra" is meaningless, unless you know exactly what libusb does underneath. Explaining which types of descriptors will be stored here and how they can be processed would be better. Perhaps the field should be called "UnsupportedDescriptors"?

@juaoose juaoose May 16, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If we were to use the RawDescriptor, should we include the original []byte in Data? Since descriptors change a bit by parent type (interface has a subtype where endpoint doesnt), plus that way you can potentially just pass Data to a specific encoding.BinaryUnmarshaler (I updated the implementation a bit to reflect some of the comments)

@zagrodzki zagrodzki May 17, 2024

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

do you have an example of a struct that would implement the unmarshaler? I think you could pass the data either way, if the marshaler expects the payload of the descriptor rather than the header. And presumably the type that does the unmarshaling is already a type for a specific descriptor type.

Ultimately it could be either way, but it affects how the code that depends on RawDescriptor needs to behave. If you have an example of a subsequent change that handles specific descriptor types, we could take a look together.

Also if you intend to write implementations around specific standard (but not supported by libusb) descriptor types, we could do that in gousb directly rather than a separate package?

Extra []byte

iInterface int // index of a string descriptor describing this interface.
}
Expand Down
2 changes: 2 additions & 0 deletions libusb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,12 +301,14 @@ func (libusbImpl) getDeviceDesc(d *libusbDevice) (*DeviceDesc, error) {
}
descs := make([]InterfaceSetting, 0, len(alts))
for _, alt := range alts {

i := InterfaceSetting{
Number: int(alt.bInterfaceNumber),
Alternate: int(alt.bAlternateSetting),
Class: Class(alt.bInterfaceClass),
SubClass: Class(alt.bInterfaceSubClass),
Protocol: Protocol(alt.bInterfaceProtocol),
Extra: C.GoBytes(unsafe.Pointer(alt.extra), alt.extra_length),
Comment thread
zagrodzki marked this conversation as resolved.
Outdated
iInterface: int(alt.iInterface),
}

Expand Down