openmoko dfu-util improvements (patch & questions)

[Stefan Schmidt]

Hello.

On Sun, 2010-04-04 at 06:11, Sandro Giessl wrote:
> Am Sonntag, 4. April 2010 03:42:17 schrieb Harald Welte:
> > On Sat, Apr 03, 2010 at 11:36:37PM +0200, Stefan Schmidt wrote:
> > > > DFU_GETSTATUS ignores bwPollTimeout but does hardcoded usleep(5000)
> > > > when doing DFU download:
> > > > This is a show-stoper for us, since this delay is too short and results
> > > > in libusb 'Broken pipe' errors.
> > > > A patch implementing proper timeout handling is attached.
> > >
> > > Nice to see other people using it.
> > 
> > I sporadically get reports from other people using it for various
> > different devices, too.

Good to know. Next to your u-boot patches I know at least one other bootloader
implementing DFU. Barebox, ealier called u-boot-v2. I would think that they are
also using dfu-utils as host counterpart, but I don't know for sure.

> > > > My question is, do you see a chance in getting these things
> > > > upstream?  Since some distros are shipping slightly different SVN
> > > > revisions, do you plan a real dfu-util release any time soon?
> > >
> > > As Paul already pointed out the biggest problem is that we don't have
> > > a real maintainer for it. I think Harald is way to busy with other
> > > things and nobody stepped up to take care of fixing problems going
> > > forward to a real release.
> > 
> > This is correct.  I don't mind bumping the version number and announcing
> > something as an official release, if that is something people want me to
> > do.  But I don't have all the various devices that people (want to) use
> > dfu-util with, and thus it's hard for me to tell what works where.
> 
> It's what stated in the header of sam7dfu.c which motivates me to merge 
> changes directly into dfu-util, instead doing some quick hacks and creating 
> yet another dfu-programmer:
> "This is supposed to be a "real" DFU implementation, just as specified in the 
> USB DFU 1.0 Spec."

Yes, it was the main motivation.

> Since it's not feasible to test any device out in the wild, the focus should 
> be set on a closer spec conformity + occasional quirks and special handling 
> based on idVendor/idProduct/bcdDevice.

I agree.

> As long as it *is* an option to not only continue in OM-bug-fixing mode I'm 
> more than happy.

It is. Personally I feel that dfu-utils is really a separate project from the
rest of the OM platform. See more below.

> > There are multiple different options:
> > 
> > 1) separate it from openmoko and create its own repository and
> >    mailinglist, run it as a standalone project
> > 
> > 2) simply giving anyone who's stepping up to maintain it the committ
> >    access in the openmoko repository and continue to point to this
> >    mailing list for patch submission and discussion
> 
> If you really don't have the time I will be glad to look after it 
> occasionally.  You may call it maintainership, but I would still prefer to 
> have some of the OM developers looking over my shoulder, pointing out possible 
> flaws or OM-incompatibilities.

I can do this for you. Will test your patch tomorrow or friday and let you know
if it breaks anything for my diferent OM devices. Quite likely that we need the
quirk to keep it working with the u-boot versions shipped on the devices.

> It's up to you... I currently prefer 2) with the emphasis to be open for other 
> parties (e.g. also their legacy device specific quirks) interested in DFU, too.

The decision depends on how much patches we will get. If its only 3 or 4
patches I can just go ahead and commit them. If more parties join the show and
starting to contribute I would prefer to move it over to a separate project.
Giving it more visibility outside of OM, moving to a git repository and starting
more frequent releases.

Harald, could you host such a project? I know your infrastructure works well and
I'm already familiar with it. dfu-utils.org is also still available. ;)

Speaking about releases we should really do one. Sandro, how much oustanding
patches do you have to make it working fine with your hardware?

I'm trying to understand if we should better make a stable release now with a
known-good setup for OM devices and another one when your stuff is in and
settled or if we should wait for landing it into the first release because it
only brings small changes.

> > > The download part is quite stable I think, at least on the Openmoko
> > > phones and I already heard from people that it shoudl also detect
> > > various BT devices. The last time I tested upload was broken though.
> > 
> > As far as I remember, this was a problem inside u-boots DFU
> > implementation and not the host utility.

Ah, thanks for letting me know. So someone interested could fix the DFU upload
and the DFU timout setting in u-boot. Volunteers? :)

> > > Hmm, this is no longer only sam7 related. Seems like a historical
> > > fragment from the pre-OM days. (Some of the code was already used on
> > > the OpenPCD project).
> > 
> > yes, and somebody should probably test if it still works with openpcd at
> > some point before making a release...

I don't have one around. So that would be either you or I would need access to
the hardware.

> > > Aehm, I don't think 2 lines of code change plus three lines comments
> > > justify the copyright addition of two people...
> > 
> > For credits, we can start a CONTRIBUTERS file or something along those
> > lines.  But I agree with Stefan. It is even legally wrong: A trivial
> > change like this does not constitue a copyrightable work and thus there
> > should be no copyright statement of the respective authors
> 
> Sure, you are right. Of course there are some bigger changes that need to be 
> cleaned up and integrated some time, so I wasn't actually thinking about how 
> ridiculous this might look in conjunction with this single usleep diff. :)

heh, no worries. :)

I would say we start a CONTRIBUTORS file once we commit your patch. After more
code changes people would move into AUTHORS as well.

> Since I can't draw the line myself, commit/maintainer guidelines are 
> appreciated. I'm no expert in legal matters, but I believed that such trivial 
> changes are well sufficient for example to update the notice of copyrighted 
> years?

That is what I normally do if I already have a copyright on a file. A rule of
thumb, by no way legally backed up, I add my copyright if I have changed
between 40 and 50 percent of a file. Obviously also if I create it. :)

> > > Did you test if bwPollTimeout is really filled from u-boot on the
> > > Openmoko phones?
> > 
> > This is an important question, OM users are probably still our primary
> > user base for the program, so we need to check it still works.  I don't
> > have any OM phone on my holidays so I cannot test.
> > 
> > Looking at the usbdfu.c code in openmoko u-boot:
> >         /* FIXME: set dstat->bwPollTimeout */
> > does not seem promising.
> > 
> > There are two solutions to this:
> > 
> > 1) Check if bwPollTimeout == 0, and then revert to the old value of
> >    5000 for usleep
> > 
> > 2) add something like device-specific kludges/fixups/quirks, similar to
> >    how many linux kernel drivers handle this.  In this case, we would
> >    have a QUIRK_POLL_TIMEOUT_5MS and define this in case we're working
> >    on an openmoko device.
> > 
> > Both solutions are fine with me and I'm happy to apply them to the repo.
> 
> The second solution sounds good enough. I agree that OM users are the main 
> user base, but since there doesn't exist any open source reference 
> implementation for DFU, dfu-util might step in.

Agreed. I feel we will have no way around quirks, but it should still be
possible to reach for spec compliance while having the quirks to be usefull in
the real world.

> I don't have an OM phone for testing, therefore it would be neccessary to 
> document its quirks and/or someone verifying that nothing breaks in case of 
> openmoko/u-boot.

I can take care of this. Better spent your time on your other projects. :)

regards
Stefan Schmidt