Re: [Libusbx-devel] [RFC 0/2] Add support for USB-3 bulk streams
Hi,
On 09/12/2013 11:12 PM, Nathan Hjelm wrote:
>
> On Sep 12, 2013, at 6:35 AM, Hans de Goede 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 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=5127&iu=/4140/ostg.clktrk
___
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [RFC 0/2] Add support for USB-3 bulk streams
On Sep 13, 2013, at 3:18 AM, Hans de Goede wrote:
> Hi,
>
> On 09/12/2013 11:12 PM, Nathan Hjelm wrote:
>>
>> On Sep 12, 2013, at 6:35 AM, Hans de Goede 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 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
Ok. Easy to fix.
> 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.
Hmm, what exactly are the semantics under Linux if the endpoints support a
different number of streams? To mimic I will either need to take the minimum
first and allocate that for each endpoint (returning an error if any endpoint
returns 0 supported streams) or just allocate the smaller of num_streams or the
maximum supported streams for each endpoint and return the mimimum.
> 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.
Sounds fine to me.
-Nathan
--
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=5127&iu=/4140/ostg.clktrk
___
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-d
Re: [Libusbx-devel] [RFC 0/2] Add support for USB-3 bulk streams
Hi,
On 09/13/2013 02:35 PM, Nathan Hjelm wrote:
>
> On Sep 13, 2013, at 3:18 AM, Hans de Goede wrote:
>> 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.
>
> Hmm, what exactly are the semantics under Linux if the endpoints support a
> different number of streams?
The xhci driver (which implements this) first iterates over all endpoints to
find the minimum support by
all of them. Then if the minimum <= 1 it bails with an error (no streams
supported on 1 or more eps), if
the minimum > 1, it allocates the minimum found on all eps.
> To mimic I will either need to take the minimum first and allocate that for
> each endpoint (returning an error if any endpoint returns 0 supported
> streams) or just allocate the smaller of num_streams or the maximum supported
> streams for each endpoint and return the mimimum.
Right, you need to "calculate" the minimum first.
>
>> 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.
>
> Sounds fine to me.
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=5127&iu=/4140/ostg.clktrk
___
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [RFC 0/2] Add support for USB-3 bulk streams
Nathan Hjelm wrote: > > Pete, I know the Microsoft compiler is a sub-par C compiler so I want > to make sure uint32_t is available before chaning the type of > num_streams. Does it support standard C99 integer types (stdint.h)? I'm struggling to avoid commenting on your political statement there. Visual C++ started including stdint.h as of Visual Studio 2010. -- Tim Roberts, t...@probo.com Providenza & Boekelheide, Inc. -- 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=5127&iu=/4140/ostg.clktrk ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [RFC 0/2] Add support for USB-3 bulk streams
On Sep 13, 2013, at 11:21 AM, Tim Roberts wrote: > Nathan Hjelm wrote: >> >> Pete, I know the Microsoft compiler is a sub-par C compiler so I want >> to make sure uint32_t is available before chaning the type of >> num_streams. Does it support standard C99 integer types (stdint.h)? > > I'm struggling to avoid commenting on your political statement there. > Visual C++ started including stdint.h as of Visual Studio 2010. Not a political statement, fact. Microsoft continues to refuse to implement C99 in VS hense it is a sub-par C compiler. Its good to hear that they finally stdint.h though. I assume that older version of VS do not have stdint.h? -Nathan -- 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=5127&iu=/4140/ostg.clktrk ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [RFC 0/2] Add support for USB-3 bulk streams
On Sep 13, 2013, at 6:45 AM, Hans de Goede wrote:
> Hi,
>
> On 09/13/2013 02:35 PM, Nathan Hjelm wrote:
>>
>> On Sep 13, 2013, at 3:18 AM, Hans de Goede wrote:
>
>
>
>>> 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.
>>
>> Hmm, what exactly are the semantics under Linux if the endpoints support a
>> different number of streams?
>
> The xhci driver (which implements this) first iterates over all endpoints to
> find the minimum support by
> all of them. Then if the minimum <= 1 it bails with an error (no streams
> supported on 1 or more eps), if
> the minimum > 1, it allocates the minimum found on all eps.
Makes sense. I updated the doxygen of alloc_streams to make those semantics
clear.
>> To mimic I will either need to take the minimum first and allocate that for
>> each endpoint (returning an error if any endpoint returns 0 supported
>> streams) or just allocate the smaller of num_streams or the maximum
>> supported streams for each endpoint and return the mimimum.
>
> Right, you need to "calculate" the minimum first.
I made the relevant changes and force-pushed an update to my tree. Let me know
if there are any other issues.
So far, I think the semantics are good. Will need some feedback from the
Windows and BSD developers on how well they will work for them.
Pete, I know the Microsoft compiler is a sub-par C compiler so I want to make
sure uint32_t is available before chaning the type of num_streams. Does it
support standard C99 integer types (stdint.h)?
-Nathan
--
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=5127&iu=/4140/ostg.clktrk
___
libusbx-devel mailing list
libusbx-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [RFC 0/2] Add support for USB-3 bulk streams
On Sep 13, 2013, at 11:36 AM, Pete Batard wrote: > and we provide our "own" stdint.h for earlier versions. See the msvc > directory. > We're also using uint#_t all over the place in both core and the Windows > backend => feel free to use anything you want from stdint.h/inttypes.h. MS > may have neglected to update their C compiler to modern standards, but we > sure do whatever we can to compensate for it. Great. I will make the change to the API and push it to my tree now. -Nathan -- 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=5127&iu=/4140/ostg.clktrk ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [RFC 0/2] Add support for USB-3 bulk streams
and we provide our "own" stdint.h for earlier versions. See the msvc directory. We're also using uint#_t all over the place in both core and the Windows backend => feel free to use anything you want from stdint.h/inttypes.h. MS may have neglected to update their C compiler to modern standards, but we sure do whatever we can to compensate for it. Regards, /Pete PS: Don't expect much from me until the middle of next week, especially in terms of reviewing anything Windows related. On 13 September 2013 18:21, Tim Roberts wrote: > Nathan Hjelm wrote: > > > > Pete, I know the Microsoft compiler is a sub-par C compiler so I want > > to make sure uint32_t is available before chaning the type of > > num_streams. Does it support standard C99 integer types (stdint.h)? > > I'm struggling to avoid commenting on your political statement there. > Visual C++ started including stdint.h as of Visual Studio 2010. > > -- > Tim Roberts, t...@probo.com > Providenza & Boekelheide, Inc. > > > > -- > 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=5127&iu=/4140/ostg.clktrk > ___ > libusbx-devel mailing list > libusbx-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/libusbx-devel > -- 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=5127&iu=/4140/ostg.clktrk___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
Re: [Libusbx-devel] [RFC 0/2] Add support for USB-3 bulk streams
One more thing about the API. It might be good to change: uint32_t API_EXPORTED libusb_transfer_get_stream_id (struct libusb_transfer *transfer); to: int API_EXPORTED libusb_transfer_get_stream_id (struct libusb_transfer *transfer, uint32_t *stream_id); This would allow the return of LIBUSB_ERROR_INVALID_PARAM if the transfer is not a bulk stream transfer. Anyway, the type change is pushed to my tree. -Nathan -- 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=5127&iu=/4140/ostg.clktrk ___ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
