[dfu-util] New dfu-suffix manipulation tool

Tormod Volden lists.tormod at gmail.com
Thu Mar 1 22:17:58 CET 2012


Hi,

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

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 :)

> 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?


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):

> 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.

<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.

+
+ 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.

+
+ /* 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.

<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.

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.

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...).


On Thu, Mar 1, 2012 at 5:20 PM, Stefan Schmidt wrote:
> Hello.
>
> 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 :)

>> > Other tools have been able to decode a suffix I added with this code.
>> ...well, since you say so, I have no more concern.
>
> I only have access to a limited set of tools and firmware images.
> (dfutool from the bluez project, the dfu suffix reference handling
> code from the spec and some firmware image I found on the net claiming
> to have a valid DFU suffix and working with with both test tools)
>
> If anyone has access to more DFU tools, perhaps under windows, and or
> images that are known to have a correct suffix I would be glad to hear
> test results.
>
>> > Maybe I'm biased due to some kernel work here but in my opinion it
>> > looks cleaner and is easier to understand instead of repeating the
>> > failure code all the time.
>> I understand. Probably compiler also takes care of that, right?
>
> Well, haven't thought about that. Just got used to goto for failure
> handling in init and I'm happy with it for this one use case. :)

You should not code your algorithms with the help of goto, but it is
considered pretty civilized for non-probable, critical error paths
jumping to the end of a function. Going around it with complicated,
often convoluted if-else structures reduces readability and thus
enhances the risk of bugs. I prefer to the see the normal "six-sigma
case" code flow right there on the left margin, with error checking
and exception traps stuffed inside their indented if clauses.

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.

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.

Cheers,
Tormod



More information about the devel mailing list