On Mon, Aug 23, 2010 at 02:30:13PM +0100, Toby Gray wrote: > Hi, > > I've been modifying barry to add support for USB channels in my git > branch at git://github.com/tobygray/barry-usbrelay.git
Hi Toby, First of all, thanks very much for this patch. It looks well written and can almost be applied as-is. I have some specific comments that should be fixed first, below. > The intention has been to provide an API with similar functionality to > the channel API in the BlackBerry desktop manager API. I implemented > this as a new mode named RawChannel and have included a tool which > connects uses STDIN and STDOUT to send and received data over the > RawChannel. > > This allows Barry to be used to interface with applications running on a > BlackBerry which have made use of the net.rim.device.api.system.USBPort > API > (http://www.blackberry.com/developers/docs/4.0.2api/net/rim/device/api/system/USBPort.html). Very nice! What would be even more super would be a documented example of how to use this, which could be added to the HTML docs under doc/www/. > 1 - BlackBerry OS v4.x connections are unreliable due to interactions > with usb_storage driver. > > This one took me a while to track down, but it manifests such that > about 50% of the time when you set up a channel/socket with the > BlackBerry that it would always fail after 30 seconds. Looking at the > USBmon traces it appears that usb_storage issues a command which it > doesn't get a response to. After 30 seconds with no response the > usb_storage driver resets the BlackBerry USB device, causing the channel > to drop. If I rmmod the usb_storage module then channel connections are > reliable and long lasting. BlackBerry OS v5.x devices work fine, > regardless of if usb_storage is loaded. I could also get OS v4.x devices > to work reliably by using usb_detach_kernel_driver_np. I have USBmon > traces available if anyone is interested in working out what mass > storage command is timing out. Thanks for finding this. Looks like this confirms that it is a firmware issue. Feel free to post the log snippet here, but even if we know what command is causing the problem, the fix would have to be on RIM's side, no? The patch, with comments: > diff --git a/src/controller.cc b/src/controller.cc > index a29bde8..d15dd1a 100644 > --- a/src/controller.cc > +++ b/src/controller.cc > @@ -156,6 +156,20 @@ Controller::~Controller() > // > uint16_t Controller::SelectMode(ModeType mode) > { > + return SelectMode(mode, NULL); > +} > +// > +// Tells device which mode is desired, and returns the suggested > +// socket ID to use for that mode. > +// > +// If explicitModeName is not NULL then it will be used as the mode name. > +// Otherwise the default mode name for the given mode will be used. > +// It should be a nul terminated string if it is provided. > +// > +// The RawChannel mode requires an explicitModeName to be specified. > +// > +uint16_t Controller::SelectMode(ModeType mode, const char *explicitModeName) > +{ > // select mode > Protocol::Packet packet; > packet.socket = 0; > @@ -166,36 +180,47 @@ uint16_t Controller::SelectMode(ModeType mode) > memset(packet.u.socket.u.mode.name, 0, > sizeof(packet.u.socket.u.mode.name)); > > char *modeName = (char *) packet.u.socket.u.mode.name; > - switch( mode ) > + > + if(explicitModeName) > { Please try to follow existing tabs and coding style. Should be: if( explicitModeName ) { ... } else { switch( mode ) { } } > > ... > + strcpy(modeName, explicitModeName); Make sure that explicitModeName is not longer than the modeName buffer. > +class BXEXPORT RawChannel : public Mode > +{ > +public: > +protected: > +private: Barry's coding style is private, protected, public. > diff --git a/tools/brawchannel.cc b/tools/brawchannel.cc > new file mode 100644 > index 0000000..abe73df > --- /dev/null > +++ b/tools/brawchannel.cc > +// Wrapper for pthread_mutex_t > +class Mutex > +{ > +public: > + friend class Condvar; > + Mutex(); > + ~Mutex(); > + void Lock(); > + void Unlock(); > +private: > + bool m_initialized; > + pthread_mutex_t m_mutex; > +}; Could src/scoped_lock.h be reused here? Just a thought. I see a lot of thread support classes... wondering what is the best way to handle this code. What do you think of moving it into the library? > +// RIAA wrapper for locking Mutex class > +class MutexLock RAII ;-) RIAA is that "evil" recording corp. > +// Class which extends the functionality of SocketRoutingQueue to add > +// error detection and setting of a continue boolean to false when an > +// error is detected. > +// This code is heavily based on the thread > +// creation code of SocketRoutingQueue, which sadly has too many > +// private variables to just sub-class. > +class ErrorHandlingSocketRoutingQueue Perhaps we should just fix SocketRoutingQueue! :-) What precisely is SocketRoutingQueue missing? Thanks again, - Chris ------------------------------------------------------------------------------ Sell apps to millions through the Intel(R) Atom(Tm) Developer Program Be part of this innovative community and reach millions of netbook users worldwide. Take advantage of special opportunities to increase revenue and speed time-to-market. Join now, and jumpstart your future. http://p.sf.net/sfu/intel-atom-d2d _______________________________________________ Barry-devel mailing list Barry-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/barry-devel
