On Thu, Jun 16, 2011 at 05:34:49PM +0100, Toby Gray wrote:
> Hello,
>
> I've reworked the libusb 1.0 support with the comments from Chris. It's
> sitting in the master branch of my github at
> git://github.com/tobygray/barry.git
>
> Hopefully the smaller commits are easier to understand and make more
> sense without me needing to explain any more of them.
Thanks! That was some beautiful work and code. I've merged the whole
thing, along with a few changes on top.
Splitting up the commits like that made it much easier for me to process.
There are still a few outstanding issues that I'd welcome your feedback on.
1) Your SetAltInterface() TODO message
diff --git a/src/controller.cc b/src/controller.cc
index cb20dda..e53c034 100644
--- a/src/controller.cc
+++ b/src/controller.cc
@@ -93,7 +93,10 @@ void Controller::SetupUsb(const ProbeResult &device)
m_priv->m_iface = new Usb::Interface(m_priv->m_dev, device.m_interface);
if( device.m_needSetAltInterface ) {
- m_priv->m_dev.SetAltInterface(device.m_interface);
+ // TODO: This doesn't seem correct behaviour as
+ // an alt setting number for the interface should be
+ // used as the parameter
+ m_priv->m_iface->SetAltInterface(device.m_interface);
}
if( device.m_needClearHalt ) {
The needSetAltInterface block was added by you... do you still have your
test system setup that you could make sure changing this works?
2) The libusb 1.0 context
I see that libusb 1.0 uses a ctx (context) pointer for init. Is it safe to call
init multiple times? If not, we should flag it somewhere, since Barry's
Init() must be safe to call multiple times.
Also, that static context pointer bothers me. I'm tempted to create a
context for Barry as well.
The idea of an Init class is tempting, since we could put the Uninit()
calls in the destructor, but I've run into problems before with libraries
being shared in applications, and destroying a library is something that
should be done carefully and explicitly by the application. This is
probably why libusb 1.0 added a context to their library init. It makes
it much safer to have multiple libraries using libusb all at once,
without conflicts in their uninit timing.
3) The Probe results
Commit fa10f865f61e73fbb89b293d3b14643488f38c0b needs to be fixed, I think.
I welcome your thoughts on how.
I really want the ProbeResults data to outlive the Probe class itself.
ProbeResults should be Usbwrap specific, not Probe specific. Also,
it should be possible to copy around ProbeResult data at will, and as
long as the bus hasn't been rescanned, and as long as the Barry library
has not been uninitialized, the ProbeResult data should be valid.
How to accomplish this, in a nice clean portable manner, though, might
be a challenge.
Is there any way of moving those DeviceID std::strings out of the
struct and into somewhere else? Then the DeviceID could just be a
typedef that references libusb.
Otherwise, this looks really good. Thanks again for all your effort!
- 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
