On Mon, Jun 13, 2011 at 01:15:05PM +0100, Toby Gray wrote: > >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.
That would be great. For large fundamental changes like this, I don't mind if each patch isn't fully functioning, as long as the end result works and passes all tests. Of course, normally, it's best to have it work *and* have it small and logical, so I appreciate that. But for integration, it's easier to be able to see each change and why it was made, described in the commit. > 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. bcharge and breset are standalone right now, so it's safe to leave those for later. Let's get the main library work merged first. > 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. A bit of a maintenance headache to have two, but I think that's best as well. Just leave bcharge.cc as is, and add bcharge_libusb_1_0.cc, and selectively compile them. Hopefully the 0.1 bcharge can then soon be replaced entirely by the 1.0 version. > >>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). Thanks! Yes, your way is correct. > >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 was looking more at the changes required in the rest of the code, such as probe.cc. As much as I like the STL, I don't like the cumbersome vector loops, and I liked how Match avoided that bit of unpleasantness. Also the other discover classes were vectors and maps in themselves, and so didn't require so much "std::vector<...>" verbiage. At the least, I'd like the Match API back, if possible, but I don't mind how you implement it behind the scenes. Just so that the changes required in probe.cc and other code are more minimal. Thanks! - Chris ------------------------------------------------------------------------------ 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
