[dfu-util] New dfu-suffix manipulation tool

Stefan Schmidt stefan at datenfreihafen.org
Wed Mar 7 13:03:17 CET 2012


Hello.

On Thu, 2012-03-01 at 22:17, Tormod Volden wrote:
> 
> Let me first comment the 0.6 plan:
> 
> > Once this is in and no problems show up I would want to go for a 0.6
> > release. If you, or anyone else, has some fixes around let me know.
> 
> I have just discovered that the Maple project ships broken firmware
> (declares itself as 1.1a = dfuse but is really 1.0 - probably some
> copy and paste originally, with incomplete fix-ups later). So dfu-util
> 0.5 does not work with those:
> http://forums.leaflabs.com/topic.php?id=1369

Oh, pity. :(

> I will work with Maple to get their firmware compliant, but I would
> also like to add quirks to dfu-util so that their already shipped
> firmware can work. The extent of the quirk will depend a bit on how we
> fix the new Maple firmware. So please hold off the 0.6 release until
> we have a confirmed fix. This should give us time to get the suffix
> tool right meanwhile :)

Sure, I can wait with the 0.6 release for the fix.

> > For the next release cycle I would like to see how we can integrate
> > the windows support Satz Klauer wrote.
> 
> One of the reason the Maple breakage has gone unnoticed, is that Maple
> ships an ancient binary of dfu-util together with their IDE. They also
> have a Windows dfu-util.exe. So I assume dfu-util builds on Windows
> already? But I guess it can be improved. Do you have a pointer to
> Satz' patches?

He sent me his changes as a tarball and I now imported them into a git
branch for further work:
http://cgit.openezx.org/dfu-util/log/?h=windows

And yes, we had working widnows support before. The underlying problem
is that I don't have a working windows installation nor ever done any
development work under windows. As far as I understand it there are
multiple ways to build a dfu-util executable under windows. One is
mingw and another one is natively with visual studio.

I can't say which one is better though. This support is mainly driven
by people who are interested in it. In this case it is Satz. I just
want to integrate it as cleanly as possible in the end.

> Now to the new suffix tool. I like how the implementation is clean and
> the tool is kept simple. Just a few comments (I haven't gotten to any
> building and testing yet):

Thanks.

> > Values for device, product and vendor id can be in decimal or hex
> > notation. Later needs a 0x prefix.
> 
> USB tools like lsusb (and dfu-util :) ) always treat product and
> vendor IDs as hexadecimal, with or without 0x. Would anyone ever like
> to use such an ID in decimal?
> 
> It could even be an idea to use the same syntax as the -d option of dfu-util.

Well with strtol we get thze correct handling for either decimal or
hex for free. The downside is that we must use the 0x prefix for
correct detection of hex values.

I'm not bound to this, but I like the idea to support both notation
easily.

> <dfu-file.c>
> 
> + /* Calculate CRC. Its calculated over file and suffix excluding the CRC
> 
> Typo -> It is
> 
> + dfusuffix[0] = file->bcdDevice;
> + dfusuffix[1] = file->bcdDevice >> 8;
> 
> Here we are doing a truncating cast from 16 bit to 8 bit. Everything
> is fine. But it kind of looks strange. I think I would prefer to add a
> "& 0xff" to the first line just for readability.

Good point. Again a bit lazy on my side :) Will fix this and the other
occurances as well.

> + ret = lseek(file->fd, 0, SEEK_END);
> + if (ret < 0) {
> + fprintf(stderr, "Could not seek to DFU suffix\n");
> + perror(file->name);
> + goto rewind;
> + }
> 
> Here you would be at the end of the file anyway, right? So the seek is
> unnecessary.

Nicely spotted. The read() already advanced the file position to the
end. Removed.

> + /* After seeking to the end of the file add the suffix */
> + ret = write(file->fd, dfusuffix, sizeof(dfusuffix));
> + if (ret < 0) {
> + fprintf(stderr, "Could not read DFU suffix\n");
> + perror(file->name);
> + goto rewind;
> + } else if (ret < sizeof(dfusuffix)) {
> + fprintf(stderr, "Could not read whole DFU suffix\n");
> + ret = -EIO;
> + goto rewind;
> + }
> +
> +rewind:
> + lseek(file->fd, 0, SEEK_SET);
> + return ret;
> +}
> 
> The messages above should say "write", not "read".
> 
> The two last gotos are NOPs and can be deleted.

Even better. With the above seeking removed nothing and these NOPs
nothing uses the rewind label anymore. All removed.

> <suffix.c>
> 
> + if (pid)
> + file->idProduct = pid;
> +
> + if (vid)
> + file->idVendor = vid;
> +
> + if (did)
> + file->bcdDevice = did;
> 
> So if the user does not specify vid/pid/did, these fields in the
> "file" structure are left uninitialized? Maybe the compiler
> initializes all local variables/structures to zero but we should not
> rely on that.
> 
> Also, a sane default for these fields would be 0xffff, which means
> "any" according to the DFU specification.

Doh, good point, again. :)

Actually I was thinking about using the "any" feature while I read the
suffix part of the spec again but forgot to add it while coding.
Changed now.

> Incidentally, the setting of these fields could be done directly to
> "file" in main() so that they need not to be passed as separate
> function arguments. Well, at least if you modify check_suffix to not
> modify "file" but a copy of it.

Hmm, not sure which one I like more. I always try to avoid copying
stuff around and having to keep in mind which one is used for which
case. I left it as is right no. We can come back to this if you have
strong opinion on this.

> This reminds me of one thing I am plentiful guilty of myself in the
> dfu-util code: The variable name "file" is poorly chosen, and is
> confusingly sometimes used for the structure, sometimes for a pointer
> to it. "file" is not a reserved name in C, but it is very generic name
> and close to "FILE" from stdio.h, so we should show some better
> imagination and use more descriptive name (of course here we always
> have only one file so it can not be misunderstood, but the day we add
> a second file to the code...).

And there we have it again. Engineers are bad at choosing variable
names. :)

What options do we have for it?

The term firmware is used in the specs, but I find it long to write
and fw not meaningful enough as abbreviation. Other then that I only
can think of dfu_fw or dfu_file also for the variable name. Any good
idea?

> On Thu, Mar 1, 2012 at 5:20 PM, Stefan Schmidt wrote:
> >
> > On Thu, 2012-03-01 at 17:09, Patryk Benderz wrote:
> >>
> >> > Why should they be in a define? The compiler detects if the variables
> >> > are constant and optimizes this case anyway.
> >> I take that argument. I was not aware that present compilers do such
> >> job. It was long ago (20 years) when was interested how compilers work,
> >> and it was Pascal compilers at the beginning.
> >
> > https://en.wikipedia.org/wiki/Constant_folding
> 
> We should use defines for readability, not for code optimization. When
> I see a variable declared I expect it to be a variable that can change
> (if it is not const'ified). Seeing it const or define'd I can forget
> about tracking it when I read the code to understand it or look for
> bugs.
>
> > are constant and optimizes this case anyway. For the case that one
> > should avoid magic values like 16 here with a named constant I would
> > agree. I was just to lazy for doing it here. :)
> 
> Sometimes reading 16 is clearer than any define, maybe also here where
> array members [0..15] are spelled out anyway. Also I interpret some of
> these variables as default settings that can change, for instance
> bcdDFU if we add an option to specify it, or suffixlen, if future DFU
> revisions use longer suffices. So there is a trade-off between make
> the current code "correct" or have it prepared for expected
> enhancements. And there is healthy laziness :)

I went ahead and added a DFU_SUFFIX_LENGTH = 16 define and added a
comment about bcdDFU.

> On Thu, 2012-03-01 at 11:45, Patryk Benderz wrote:
> > First what I have noticed is different bcdDFU field. You have hard-coded
> > 0x0100(=0100h, right?), while [1] document is labeled as DFU_1.1.pdf. So
> > the title of file mentions next minor version number. However in
> > appendix B (page 41) it is written:
> > "The bcdDFU field is a two-byte specification revision number. The value
> > as of this revision of the specification is 0100h, representing version
> > 1.0."
> > so the document [1] seems to be inconsistent with itself. Shouldn't this
> > be clarified with usb.org , before hard coding bcdDFU?
> 
> The bcdDFU field of a DFU file suffix should not necessary be the same
> as the DFU version used by the device IMO. The only use for this field
> is, in my interpretation, to declare to the program parsing it
> (downloading program or DFU file checker) what the fields in the
> suffix represent. There are no changes to this between 1.0 and 1.1 so
> it does not make sense to specify 1.1 in a file suffix AFAICS. I
> haven't seen many DFU suffices around but I think none of them say
> 1.1. I am not sure if the DFU 1.1 specification document was intended
> to be written like that, but for now it says we should use 1.0 in this
> field.

Agreed. They stay with 1.0 and I haven't seen anything indicating that
this was an error. Getting more firmware files with DFU suffix would
help us here to see some patterns. If anyone has links to public
firmware images that contain such a suffix I would be interested in
them. I need to check where I got the ones from I'm using right now.

> Other than that, dfu-util will not accept other values than 1.0 or
> 1.1a for now :)
> 
> If I am wrong, or if this changes in the future, we should add an
> option to the suffix tool to choose bcdDFU version.

I see no new DFU spec coming. 1.0 is from 1999 and 1.1 is a small
update in 2004. Nothing since then. We can think about adding
something if its needed.

I did all changes based on your review in one commit:
http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix

regards
Stefan Schmidt



More information about the devel mailing list