Add unknown interface descriptors to Extra #127
Conversation
interface.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
| DescriptorTypeConfig DescriptorType = C.LIBUSB_DT_CONFIG | ||
| DescriptorTypeString DescriptorType = C.LIBUSB_DT_STRING | ||
| DescriptorTypeInterface DescriptorType = C.LIBUSB_DT_INTERFACE | ||
| DescriptorTypeCSInterface DescriptorType = 0x24 |
There was a problem hiding this comment.
what about CSEndpoint?
also add a comment to specify where the value was sourced from. (in this case I assume this is CDC spec v1.1/1.2?), since this descriptor type is not a part of the USB spec as such.
| @@ -35,6 +35,11 @@ func (i InterfaceDesc) String() string { | |||
| return fmt.Sprintf("Interface %d (%d alternate settings)", i.Number, len(i.AltSettings)) | |||
| } | |||
|
|
|||
| type RawDescriptor struct { | |||
There was a problem hiding this comment.
document the type. In particular whether the header of the descriptor (type+length) is a part of the data is an important information here.
interface.go
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
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?
| var extraDescriptors []*RawDescriptor | ||
| extra := C.GoBytes(unsafe.Pointer(alt.extra), alt.extra_length) | ||
| for i := 0; i != len(extra); i += int(extra[i]) { | ||
| block := extra[i : i+int(extra[i])] | ||
| bType := DescriptorType(block[1]) | ||
|
|
||
| extraDescriptors = append(extraDescriptors, | ||
| &RawDescriptor{ | ||
| Type: bType, | ||
| Data: block}, | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
extract to a helper function, since we're going to use that on interfaces as well as endpoints and configs.
| block := extra[i : i+int(extra[i])] | ||
| bType := DescriptorType(block[1]) | ||
|
|
||
| extraDescriptors = append(extraDescriptors, |
There was a problem hiding this comment.
join the line for append (i.e. the &RawDescriptor literal starts in the same line)
| extraDescriptors = append(extraDescriptors, | ||
| &RawDescriptor{ | ||
| Type: bType, | ||
| Data: block}, |
There was a problem hiding this comment.
closing curly brace on the next line, together with closing parenthesis
|
any updates? |
|
ping again. A new release will be coming out in ~2wks and it would be nice if this was included. |
Include the extra interface descriptors that libusb includes.