Hi,
On 09/12/2013 11:12 PM, Nathan Hjelm wrote:
>
> On Sep 12, 2013, at 6:35 AM, Hans de Goede <hdego...@redhat.com> wrote:
>
>> Hi,
>>
>> On 09/11/2013 11:04 PM, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 09/11/2013 04:10 PM, Nathan Hjelm wrote:
>>>>
>>>> On Sep 11, 2013, at 7:15 AM, Nathan Hjelm <hje...@me.com> wrote:
>>>>
>>>>> One question. Would it make sense to add an additional transfer type for
>>>>> stream transfers? The stream_id field is not set to 0 by
>>>>> libusb_fill_bulk_transfer so I can not reliably tell if a transfer is a
>>>>> stream transfer in the backend. I need a reliable way to detect stream
>>>>> transfers so I can switch APIs.
>>>>
>>>> So, if I can add LIBUSB_TRANSFER_TYPE_STREAM I have a complete
>>>> implementation. It compiles but needs testing.
>>>
>>> If possible I would like to avoid making this a new transfer type.
>>> And I think we can avoid this, above you wrote:
>>> "The stream_id field is not set to 0 by libusb_fill_bulk_transfer"
>>>
>>> This is true, but it is set to 0 by libusb_alloc_transfer, so it will
>>> always be 0 for non stream transfer, unless an app re-uses a transfer
>>> changing it from a bulk-stream one into a regular one.
>>>
>>> Also note that we could simply change libusb_fill_bulk_transfer to
>>> zero out the stream-id.
>>
>> Sleeping on this a night, a withdraw my objection against having
>> a LIBUSB_TRANSFER_TYPE_STREAM (note I would like to see it called
>> LIBUSB_TRANSFER_TYPE_BULK_STREAM) if that makes life easier for other
>> platforms. We should make a decision on this before finalizing the API.
>
> It isn’t ideal but it does make the code more reliable. I have the the type
> and the darwin backend for streams implemented in my tree. See
> https://github.com/hjelmn/libusbx/tree/upstream-stream
Thanks for adding support for streams, looks good. I see that Linux so far
is unique in allowing setting up multiple endpoints in one go, but as your
code shows, this can simply be worked around for backends which don't
support this.
I've 2 small remarks
1) The alloc_streams backend op should return the number of streams allocated,
quoting the libusb doxygen comment I added to core.c:
* Allocate up to num_streams usb bulk streams on the specified endpoints. This
* function takes an array of endpoints rather then a single endpoint because
* some protocols require that endpoints are setup with similar stream ids.
* All endpoints passed in must belong to the same interface.
*
* Note this function may return less streams then requested.
* \returns number of streams allocated, or a LIBUSB_ERROR code on failure
2) The alloc_streams backend op should not fail when the user requests more
streams
then available, instead it should simply allocate the number of available
streams
available and return that (as mentioned in the doxygen docs)
So this is wrong:
+ (*(cInterface->interface))->SupportsStreams (cInterface->interface,
pipeRef, &supportsStreams);
+ if (num_streams > supportsStreams) {
+ rc = LIBUSB_ERROR_INVALID_PARAM;
+ break;
+ }
And should be:
+ (*(cInterface->interface))->SupportsStreams (cInterface->interface,
pipeRef, &supportsStreams);
+ if (num_streams > supportsStreams)
+ num_streams = supportsStreams
Note the check for how many streams are supported is done inside the kernel
with Linux, and it will continue with the maximum number of streams supported
if the user request more streams, so it is hard to implement any other logic
under Linux, while it seems easy to mimick the Linux behavior with darwin.
Maybe we should log a warning when returning less streams then requested?
If we want to log a warning it would be best to actually do that in the
core.c alloc_streams wrapper, so that it is consistently done for all backends.
Regards,
Hans
------------------------------------------------------------------------------
How ServiceNow helps IT people transform IT departments:
1. Consolidate legacy IT systems to a single system of record for IT
2. Standardize and globalize service processes across IT
3. Implement zero-touch automation to replace manual, redundant tasks
http://pubads.g.doubleclick.net/gampad/clk?id=51271111&iu=/4140/ostg.clktrk
_______________________________________________
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
