Hi,
On 05/18/2013 01:31 AM, Pete Batard wrote:
> On 2013.05.17 14:43, Hans de Goede wrote:
<snip>
>> If we were to design this from scratch I would completely agree. But
>> we're not and we have
>> to maintain ABI and API compatibility, so I've to strongly NACK this
>> part of your patch:
>>
>> @@ -531,6 +585,9 @@ struct libusb_endpoint_descriptor {
>> /** For audio devices only: the address if the synch endpoint */
>> uint8_t bSynchAddress;
>>
>> + /** SuperSpeed Endpoint Companion descriptor */
>> + struct libusb_ss_endpoint_companion_descriptor
>> *ss_endpoint_companion;
>> +
>> /** Extra descriptors. If libusbx encounters unknown endpoint
>> descripto
>> * it will store them here, should you wish to parse them. */
>> const unsigned char *extra;
>>
>> Put this on your things to cleanup for libusb-2.x list, because we are
>> simply NOT going
>> to make this change in libusb[x]-1.x.y
>
> Well, if 1.x.y is out of the equation, then I will strongly vote for not
> adding BOS and EP companions in 1.x at all.
I disagree, just because we cannot have the perfect API, is not a very
valid reason to not add support for something, as long as we can do
so with a good enough API. To quote Torvalds:
"perfect is the enemy of good".
> While I recognize that introducing the extra desc in 1.0 may not be for
> the best (or more exactly, that the world is not quite yet ready for
> something like that just yet) you have to consider the logical extension
> of what you're advocating: Since we unwrap our choice selection of BOS
> descriptors, then we're also going to have to break our API again next
> time the USB committee introduces a new one there. Say there's a new BOS
> for 10 Gbps speed tomorrow, or that suddenly wireless USB becomes real
> popular (fat chance... I know!), and we want to add these new
> descriptors, what do we do then? Do we go 2.0->3.0 since we've gone
> 1.0->2.0 when we introduced ep_comp in our descriptors? Heck what if we
> already had 2.0 out, but without ep_comp. Should we go 3.0 right there,
> and then 4.0 when we want to add additional BOSes?
You make a very valid point here, and since we certainly don't want to
be changing the ABI every time we add support for a new BOS descriptor
type, we should design our API in such a way that we don't need too,
here is a proposal for that:
struct libusb_bos_dev_capability_descriptor {
uint8_t bLength;
uint8_t bDescriptorType;
uint8_t bDevCapabilityType;
uint8_t dev_capability_data[0];
};
struct libusb_bos_descriptor {
uint8_t bLength;
uint8_t bDescriptorType;
uint16_t wTotalLength;
uint8_t bNumDeviceCaps;
struct libusb_bos_dev_capability_descriptor *dev_capability[0];
};
This will allow libusb_get_bos_descriptor to simply always return all
descriptors.
For known types like the USB 2.0 Extension descriptor we will then
offer functions like:
int libusb_get_usb_2_0_extension_descriptor(struct
libusb_bos_dev_capability_descriptor *bos_dev_cap, struct
libusb_usb_2_0_extension_descriptor *usb_2_0_extension);
Or, if they have nested descriptors:
int libusb_get_foo_descriptor(struct libusb_bos_dev_capability_descriptor
*bos_dev_cap, struct libusb_foo_descriptor **foo);
void libusb_free_foo_descriptor(struct libusb_foo_descriptor *foo);
Note the difference in how we handle static-sized versus nested types
is following the existing pattern of how we do this with device descriptors
vs config descriptors.
This also fixes the potential issue you mentioned in this comment:
/* The following assumes that only one descriptor of the following
* types will ever be provided by the device for each endpoint... */
Note there is a lesson to be learned here for endpoint descriptors too,
when we fix the API to support the superspeed endpoint companion
descriptor, we probably want to do it in such a way that more new
endpoint descriptor types can be supported without breaking the ABI.
<snip discussion about versioning and ABI stability, I will respond to
that in a separate reply to keep the BOS / ss ep desc and that
discussion separate>
>> Instead I suggest we add a:
>>
>> int LIBUSB_CALL libusb_get_ss_endpoint_companion_descriptor(struct
>> libusb_endpoint_descriptor *endpoint_descriptor, struct
>> libusb_ss_endpoint_companion_descriptor **ss_endpoint_companion)
>
> Strongly against (even as this is more or less what I had in my patch
> for 1 year ago). Supporting and maintaining APIs that we really don't
> want in the first place, instead of encouraging people to use a better
> approach helps nobody.
The question really boils down to:
1) Not having ss ep desc support in our current ABI version
2) Doing an ABI breaking release to add it in an API wise better manner
and then quickly having todo an other ABI breaking release to fix
all our other API issues
3) Having less then ideal, but still very usable ss ep desc support
in a manner compatible with our ABI
1) Is not really desirable given how long patches for this have been
floating around
2) This will mean that rather then maintaining one less then ideal API
call separately from what it will look in the next ABI breaking release,
we end up maintaining 3 (*) complete different ABI branches, and
maintaining an entire branch is a lot more work then maintaining one
simple function in 2 different forms.
3) So by elimination of 1 and 2, 3 gets my vote
This mostly boils down to the whole ABI / versioning discussion
which I would like to keep separate from the BOS / ss-ep-comp
discussion, so lets discuss this further in the sub-thread
I'm starting for that please.
>> I'm not enthusiastic about the notion of a
>> libusb_open_with_vid_pid_instance(),
>
> Neither am I. I am however enthusiastic with providing our users with
> something that will suit them, regardless of whether this is seen as the
> "right" way to approach selection of devices with the same IDs.
>
>> it
>> feels too much like adding more duct-tape to a broken concept (opening by
>> vid-pid is inherently broken as vid-pid are not necessarily unique, as
>> this example clearly shows).
>
> You have to consider this from a user's perspective. If we're lucky
> enough, the end user will have some clue about a VID:PID uniquely
> identifying the same devices, since that's their whole point. And their
> first thought will be that, if they have two devices with same VID:PID,
> then the app (through libusbx) should provide a straightforward way to
> pick one, with a process of interactive elimination likely to work a lot
> better than trying to figure the topology.
>
>> I'm not saying I would nack a patch adding this, but I don't think it is
>> a good idea.
>
> The above being said, I'm still open to alternative ideas, and I'm only
> considering options.
I'm afraid I don't have any alternative ideas.
> Then again, forcing every developer to develop their own methods to
> separate devices, or using topology when I don't remember seeing one USB
> hub that had actual port numbering, ever (and even then, I'd be weary of
> the OS picking whatever order it pleases), doesn't strike me as too
> great, when we can introduce a simple and hopefully intuitive API based
> on user interaction/trial and error (which, while not that elegant has
> the merit of being KISS).
I understand, as said I won't nack a libusb_open_with_vid_pid_instance,
and after reading the above I better understand your motivations for it,
and withdraw my objections, with the side note that it still does not
feel right, but you're right that it would be real handy to have in
some cases.
Regards,
Hans
------------------------------------------------------------------------------
AlienVault Unified Security Management (USM) platform delivers complete
security visibility with the essential security capabilities. Easily and
efficiently configure, manage, and operate all of your security controls
from a single console and one unified framework. Download a free trial.
http://p.sf.net/sfu/alienvault_d2d
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
