On 09/06/2011 21:43, Chris Frey wrote:
On Fri, Jun 03, 2011 at 03:00:51PM +0100, Toby Gray wrote:
Hi,
I've finally managed to get around to adding support for libusb 1.0.
This is to fix an issue with raw channels loosing data when running
using libusb 0.1 due to bulk read timeouts and transferred data being
dropped.
First of all, thank you *very* much for this work. Sorry for the delay
in getting to these.
That's ok, it took me months to get around to actually writing the code.
Ouch... gigantic patch. :-) I'll be splitting this one up, I suspect,
unless you would rather. :-)
I was working on a principle of it should compile and vaguely work after
each commit. If you're happy for me to split it up into smaller bits of
work which don't necessarily work or even compile without subsequent
patches then I'm happy to split it up as I rework it with your other
comments.
libraries.
66929841996ac1985f34
<https://github.com/tobygray/barry/commit/66929841996ac1985f349c9e716d187578edcc94>
- This changes the debian packages to always use libusb 1.0
I did not include this patch yet. It might be the right move, but I want to
do more testing before leaving 0.1 behind by default.
That's a very good point. I'll change it so that it forces configure to
use libusb 0.1.
cba4e6a668702764f06b
<https://github.com/tobygray/barry/commit/cba4e6a668702764f06bef388ec364a5c36c9dda>
- This forces the RPM packages to always use libusb 0.1, I don't know
enough about the different RPM based platforms to know if they all have
libusb 1.0 available, so this seemed the most sensible choice.
This appeared to remove the libusb-devel dependency completely.
That didn't seem right, so not including yet either.
That was deliberate. It's only removed for the libbarry-devel package as
you no longer need the usb headers when using libbarry as the public
barry API headers no longer #include <usb.h>. However I'm no expert on
how RPM packages work, so I've probably missed something.
d409f601e001556650ba
<https://github.com/tobygray/barry/commit/d409f601e001556650ba66159b108e4da8ee4295>
- Is a minor change to allow the test script to run in dash if that's set
as the system shell
Barry doesn't support dash. :-) Sorry. The right fix is to change
the shebang, I think. I'm not willing to give up bashisms in my shell
programming. None of Barry's scripts are speed-critical, so dash is
not a requirement from that perspective.
Good point. I'll just change the shebang then.
Of these changes I think the only possibly controversial changes are:
* How I've done the private implementation for the various USB wrapping
classes. It just uses a private struct rather than the more tradition
private implementation pattern of deferring all API calls to a private
class. The only reason I did this was because I couldn't see any good
reason for writing out all the methods wrappers to the private
implementation method (e.g. Device::DoStuff() { m_impl->DoStuff() } ).
However I'm happy to add all these methods if it's preferred.
I use the private struct myself. I don't mind.
* The change of breset and bcharge to using libbarry. There might be
reasons for these two tools using libusb directly which I'm not aware
of, so I made these changes as a separate commit.
They weren't a separate commit that I saw. :-) I think they were
included in the gigantic one.
They were changed in the gigantic one (4bf09c6b1034b5a80567
<https://github.com/tobygray/barry/commit/4bf09c6b1034b5a8056777723b67198c2dcd099e>)
to have big #ifdef blocks to select between the two USB libraries
(separate commits would certainly have been much easier). I made a
change locally to my machine (but forgot to push it to github) which
change bcharge and breset so that they made use of the USB wrap
functionality in libbarry instead of calling the USB library directory.
I've put these changes on github as (82a0e592349554c9d8eb
<https://github.com/tobygray/barry/commit/82a0e592349554c9d8ebdc39417b559b199cb4b7>)
and (b41e91677902084279c4
<https://github.com/tobygray/barry/commit/b41e91677902084279c4f62b1b1d67546c6b925f>).
bcharge is meant to be standalone. It started out as C as well, but
that didn't last. The idea was to enable USB charging with the
minimal dependencies.
I suspected as much. Assuming that you still want this to be the case,
do you think the library switching code is best done using #ifdefs how
it currently is or would it be better to do something else? The only
alternative I can think of is to create two separate source files for
each tool, so there would be bcharge_libusb.cc and bcharge_libusb_1_0.cc.
Since USB charging has been removed from the kernel as well, Barry is
the only software that supports this. If possible, I'd like to keep
bcharge somewhat separate, because of this.
Any other comments or suggestions are welcome.
I'm going through the changes in usbwrap.h, and I see you moved
SetAltInterface() to Interface instead of Device. Not even libusb
does this... it requires the device handle in SetAltInterface() and
so to my mind, this is part of the Device class.
My reasoning for this move was because it sets the alt setting for a
particular interface, not for the device as a whole. Libusb 0.1 only
provides the ability to do this for the currently interface (see
http://libusb.sourceforge.net/doc/function.usbsetaltinterface.html).
Libusb 1.0 lets you set the alternative setting on any claimed interface
(see
http://libusb.sourceforge.net/api-1.0/group__dev.html#ga3047fea29830a56524388fd423068b53).
However I'm not expert on the finer details of USB, so if the alt
setting of an interface is a device level property then I can change the
API back to how it was, but I think the libusb 1.0 wrapper will have to
keep track of the last Interface object to be created so that it can
emulate the current interface behaviour of libusb 0.1.
I see that the Match class is a goner, as well as the container-like
wrappers for device and endpoint discovery. I'm not sure how I feel
about this. Would you care to share your reasoning for breaking that
API so much?
My feeling was that it was cleaner to have a class which was explicitly
a list of all devices (Usb::DeviceList) and have a method on it to
retrieve a list of matching devices (Usb::DeviceList::MatchDevice).
However if you'd prefer to keep the Match class then I'm happy to rework
DeviceList so that it exposes the functionality in a similar way to
Match. Would you prefer a single Match class or a DeviceList which is
used by a Match class?
I didn't do the porting, so maybe it was a lot of work to try to maintain
that API. But I'd like to know why it's gone.
It's a fair question, needless API breaks are always a bad idea.
Regards,
Toby
------------------------------------------------------------------------------
EditLive Enterprise is the world's most technically advanced content
authoring tool. Experience the power of Track Changes, Inline Image
Editing and ensure content is compliant with Accessibility Checking.
http://p.sf.net/sfu/ephox-dev2dev
_______________________________________________
Barry-devel mailing list
Barry-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/barry-devel
