On Sep 13, 2013, at 3:18 AM, Hans de Goede <hdego...@redhat.com> wrote:
> 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 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=51271111&iu=/4140/ostg.clktrk _______________________________________________ libusbx-devel mailing list libusbx-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/libusbx-devel
