On 17/06/2011 23:52, Chris Frey wrote:
Thanks! That was some beautiful work and code. I've merged the whole
thing, along with a few changes on top.
Thanks for making the various style and error handling 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?
If I'm remembering correctly then this was needed for older handsets on
some systems, possibly FreeBSD and/or MacOSX, but I'm not 100% sure.
Sadly I don't have either FreeBSD or MacOSX close to hand to confirm
which one it was.
However I'm fairly sure that it's a problem we'll never see due to all
known Blackberry USB descriptors having a layout which makes the
different not matter. Just picking the closest BlackBerry I have to
hand, a Tour 9630, I've attached the output from lsusb -v.
Cutting the output down to only the important information:
Bus 002 Device 003: ID 0fca:8004 Research In Motion, Ltd.
Device Descriptor:
bNumConfigurations 1
Configuration Descriptor:
bNumInterfaces 2
bConfigurationValue 1
iConfiguration 0
Interface Descriptor:
bInterfaceNumber 0 <--- The line uses this number
bAlternateSetting 0 <--- But the libusb APIs expect this number
bInterfaceClass 255 Vendor Specific Class
iInterface 12 BlackBerry
Interface Descriptor:
bInterfaceNumber 1
bAlternateSetting 0
bInterfaceClass 8 Mass Storage
iInterface 14 RIM Mass Storage Device
The line I've added the TODO comment on assumes that bInterfaceNumber ==
bAlternateSetting for the BlackBerry interface. The only time I've come
across alternate settings being used in USB is to do with bus bandwidth
usage and having different alternate settings for a particular interface
which can do larger endpoint transfers. Which probably explains why
BlackBerry devices don't make use of the alternate setting.
So I think the most likely way that this could get broken is if RIM
release a device which has the vendor specific interface being something
other than interface number 1. I'll start a FreeBSD virtual machine
installing in the background and see if I can reproduce the need for
that if statement so that I can modify it to use the bAlternateSetting
value instead of bInterfaceNumber.
The corresponding code in Probe::ProbeDevice where it decides on if
SetAltInterface is needed will also need updating, but that's equally
straightforward.
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.
Won't the check on the value of initialized in Barry::Init() prevent the
USB library from being initialised more than once?
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.
Agreed. Another issue would be which Barry APIs would start needing to
have the Barry context passed into them. Probe could probably wrap the
Barry context up into the ProbeResult, so it'd hopefully be only a small
change to the API.
Perhaps a start would be to have a USB context but just have it as a
static inside common.cc, that way it's at least in the same place as
most (all?) of the other global statics in Barry. What do you think?
3) The Probe results
Commit fa10f865f61e73fbb89b293d3b14643488f38c0b needs to be fixed, I think.
I welcome your thoughts on how.
It's a tricky one. One option would be to use shared_ptr class to hold
the reference to the DeviceID so that it's all nicely reference counted
so would solve the ownership issues.
However I see a couple of problems with that. Firstly it gives the false
impression that you can keep ProbeResults around after reprobing, which
won't work with libusb 0.1 due to probing calling usb_scan_busses() to
be called and therefore invalidating previous DeviceIDs.
Secondly I've got a background task of trying to get Barry compiling and
running on Android tablet devices. The bit that I'm currently stuck on
with getting Barry to compile for Android is that there is no
<tr1/memory> support in Android, so the Router class fails to compile.
I guess one solution would be to implement a shared_ptr style class
within Barry, but that isn't an appealing option for obvious reasons!
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.
Moving the ProbeResults to Usbwrap (or to at least hold a class which is
from UsbWrap) sounds sensible, but I think it'd still require the need
for reference counting.
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.
It'd be possible, but I think it'd just be moving them to static buffers
within Usbwrap, which doesn't sound like a better situation.
What would you say to going the other way and making DeviceID a proper
class and then either using reference counting (which could be managed
by a custom copy constructor for ProbeResult) or just using a shared_ptr
to wrap the DeviceID object?
Regards,
Toby
Bus 002 Device 003: ID 0fca:8004 Research In Motion, Ltd.
Device Descriptor:
bLength 18
bDescriptorType 1
bcdUSB 2.00
bDeviceClass 0 (Defined at Interface level)
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize0 64
idVendor 0x0fca Research In Motion, Ltd.
idProduct 0x8004
bcdDevice 2.11
iManufacturer 1 Unknown String
iProduct 15 RIM Composite Device
iSerial 13 66DAC0456088FF7E71CD509F9F291B6048F86417
bNumConfigurations 1
Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength 97
bNumInterfaces 2
bConfigurationValue 1
iConfiguration 0
bmAttributes 0x80
(Bus Powered)
MaxPower 500mA
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 0
bAlternateSetting 0
bNumEndpoints 8
bInterfaceClass 255 Vendor Specific Class
bInterfaceSubClass 1
bInterfaceProtocol 255
iInterface 12 BlackBerry
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81 EP 1 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01 EP 1 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82 EP 2 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02 EP 2 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83 EP 3 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x03 EP 3 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x84 EP 4 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x04 EP 4 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Interface Descriptor:
bLength 9
bDescriptorType 4
bInterfaceNumber 1
bAlternateSetting 0
bNumEndpoints 2
bInterfaceClass 8 Mass Storage
bInterfaceSubClass 6 SCSI
bInterfaceProtocol 80 Bulk (Zip)
iInterface 14 RIM Mass Storage Device
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x85 EP 5 IN
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x05 EP 5 OUT
bmAttributes 2
Transfer Type Bulk
Synch Type None
Usage Type Data
wMaxPacketSize 0x0200 1x 512 bytes
bInterval 0
Device Qualifier (for other device speed):
bLength 10
bDescriptorType 6
bcdUSB 2.00
bDeviceClass 0 (Defined at Interface level)
bDeviceSubClass 0
bDeviceProtocol 0
bMaxPacketSize0 64
bNumConfigurations 1
Device Status: 0x0000
(Bus Powered)
------------------------------------------------------------------------------
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
