[dfu-util] New dfu-suffix manipulation tool

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[dfu-util] New dfu-suffix manipulation tool

Stefan Schmidt-2
Hello Tormod and others.

One major feature we lacked from the beginning was support for the DFU
suffix manipulation. Tormad added parsing support some time ago and
now I finally found some time to finish a small stand alone
manipulation tool for DFU suffices.

Nothing big, the suffix itself is small, but finally a way to add a
suffix to existing fimrware files and allow dfu-util to verify the
firmware against corruption (CRC) or wrong usage (USB product and
vendor ids).

It allows to verify, remove and add a suffix to a given file:

Verify:
dfu-suffix -c $FILE

Remove:
dfu-suffix -D $FILE

Add:
dfu-suffix -a $FILE -d 0x1000 -p 0x92AD -v 0x403

Values for device, product and vendor id can be in decimal or hex
notation. Later needs a 0x prefix.

@Tormod: I haven't applied the code to the master branch yet. Would be
glad if you could give it a fast review before I do this:

http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix&id=15757085aa10294b9a58719a42e922fe30405ff8
http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix&id=f77d394e98f790efef043f4eec88c9bd2dde06eb

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.

For the next release cycle I would like to see how we can integrate
the windows support Satz Klauer wrote.

regards
Stefan Schmidt

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Patryk Benderz
[cut]
Hi,
I didn't compile this, just looked on logical consistency with DFU
specification I was able to find [1]. If this is not proper one, please
correct me.

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?

> http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix&id=15757085aa10294b9a58719a42e922fe30405ff8
1.I am not skilled programmer, but I was taught that constant values
like bcdDFU or bLength, in code should be in #define declaration... if
it makes any difference, apart from good practices.

2.I could not find ucDfuSignature field, or similar one anywhere.

3.Order of filling dfusuffix[] matrix is different from what Appendix B
says. This indeed might indicate that you have used different version of
DFU documentation than 1.1. If that is true, do you consider adapting
your patch to fit 1.1 DFU version?

4. And again, do not take it personal, but I was taught to avoid 'goto'
directive, as compilers do not like it. Additionally this kind of
directives are not elegant. So maybe instead using "rewind:" label, it
could be written this way (intentionally left a lot of new lines to ease
reading)"

+ret = lseek(file->fd, 0, SEEK_END);
+if (ret < 0)
+{
+  fprintf(stderr, "Could not seek to DFU suffix\n");
+  perror(file->name);
+}
+else
+{
+  /* 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);
+  }
+  /* Inserting "ret >=0 && " to avoid entering statement when ret<0 */
+  else if (ret >=0 && ret < sizeof(dfusuffix))
+  {
+    fprintf(stderr, "Could not read whole DFU suffix\n");
+    ret = -EIO;
+  }
+}
+
+lseek(file->fd, 0, SEEK_SET);
+return ret;
+}

I hope I didn't make mistakes.

> release. If you, or anyone else, has some fixes around let me know.
Just a final question about version of DFU. Did you used 1.0 or 1.1?
Will try to look at another commit later.

[1] http://www.google.com/url?q=http://www.usb.org/developers/devclass_docs/DFU_1.1.pdf&sa=U&ei=UkBPT-ivLoKd0QXOz-TjCw&ved=0CAQQFjAA&client=internal-uds-cse&usg=AFQjCNFHUYhSddqeEROv0PVDWXPCYUC8zA

--
Patryk "LeadMan" Benderz
Linux Registered User #377521
()  ascii ribbon campaign - against html e-mail
/\  www.asciiribbon.org   - against proprietary attachments


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Stefan Schmidt-2
Hello.

Thanks for taking some time looking into this.

On Thu, 2012-03-01 at 11:45, Patryk Benderz wrote:
>
> I didn't compile this, just looked on logical consistency with DFU
> specification I was able to find [1]. If this is not proper one, please
> correct me.

The USB forum has the specs available:

www.usb.org/developers/devclass_docs/usbdfu10.pdf
www.usb.org/developers/devclass_docs/DFU_1.1.pdf

That are the ones I use.

> 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?
>
> > http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix&id=15757085aa10294b9a58719a42e922fe30405ff8
> 1.I am not skilled programmer, but I was taught that constant values
> like bcdDFU or bLength, in code should be in #define declaration... if
> it makes any difference, apart from good practices.

Well, good practices is in the eye of the beholder. :)

Why should they be in a define? The compiler detects if the variables
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. :)

> 2.I could not find ucDfuSignature field, or similar one anywhere.

Its in dfusuffix[8,9,10]. Written in dfu_file.c line 217-219.

> 3.Order of filling dfusuffix[] matrix is different from what Appendix B
> says. This indeed might indicate that you have used different version of
> DFU documentation than 1.1. If that is true, do you consider adapting
> your patch to fit 1.1 DFU version?

Appendix B is identical for version 1.0 and 1.1. I think you might be
confused with the negative offset given in the table. Let me quote the
spec here:

"The offsets are negative. This is a file suffix, and the negative
offsets indicate that the last byte of the file is specified in the dwCRC field"

Considering this the bytes are appended to the file in the correct
order. Other tools have been able to decode a suffix I added with this
code.

> 4. And again, do not take it personal, but I was taught to avoid 'goto'
> directive, as compilers do not like it. Additionally this kind of
> directives are not elegant. So maybe instead using "rewind:" label, it
> could be written this way (intentionally left a lot of new lines to ease
> reading)"
>
> +ret = lseek(file->fd, 0, SEEK_END);
> +if (ret < 0)
> +{
> +  fprintf(stderr, "Could not seek to DFU suffix\n");
> +  perror(file->name);
> +}
> +else
> +{
> +  /* 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);
> +  }
> +  /* Inserting "ret >=0 && " to avoid entering statement when ret<0 */
> +  else if (ret >=0 && ret < sizeof(dfusuffix))
> +  {
> +    fprintf(stderr, "Could not read whole DFU suffix\n");
> +    ret = -EIO;
> +  }
> +}
> +
> +lseek(file->fd, 0, SEEK_SET);
> +return ret;
> +}

And looking at it I know why goto was chosen here. All this if else
and testing of different values makes the code not cleaner in my
opinion. Handling failure cases during initialization is actually the
only use case I use goto for. :)

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. Its a matter of coding style in the end.

> Just a final question about version of DFU. Did you used 1.0 or 1.1?
> Will try to look at another commit later.

I mainly use the DFU 1.0 spec. There are not much differences in the
1.1 spec though.

Thanks again for your comments.

regards
Stefan Schmidt

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Patryk Benderz
[cut]
> Well, good practices is in the eye of the beholder. :)
To quote Sheldon's mother from "Big Bang Theory": "And that is your opinion." ;)

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

[cut]
>  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. :)
In fact you were the hard-working one. Lazy was I, who just wrote a
comment instead of writing a code itself.

[cut]
> "The offsets are negative. This is a file suffix, and the negative
> offsets indicate that the last byte of the file is specified in the dwCRC field"
OK, now I get the negative offsets idea, thanks :)

> Other tools have been able to decode a suffix I added with this code.
...well, since you say so, I have no more concern.

[cut]
> 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?

--
Patryk "LeadMan" Benderz
Linux Registered User #377521
()  ascii ribbon campaign - against html e-mail
/\  www.asciiribbon.org   - against proprietary attachments


_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Stefan Schmidt-2
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

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

regards
Stefan Schmidt

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Tormod Volden
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

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Stefan Schmidt-2
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

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Tormod Volden
On Wed, Mar 7, 2012 at 1:03 PM, Stefan Schmidt wrote:

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

Hi,

I got a reply from Leaflabs and they were going to test things. But
since then I haven't heard anything. At the minimum I would like a
confirmation from them on which USB ID's need to be quirked.

>> > For the next release cycle I would like to see how we can integrate
>> > the windows support Satz Klauer wrote.
>
> 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

I had a quick look. It is based off an older tree so we must be
careful not to revert anything accidentally. But I suppose your plan
is to introduce the changes piece by piece anyway. Some of the changes
there make sense in any case, like adding "const" qualifiers. We can
also port all file handling to use FILE, fread() and friends from
stdio instead of the less portable read().

There were things there I did not like, but we can get back to this
once we have split up patches for this.

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

These are IDs and not numbers or addresses. So adding the possibility
to enter these values in decimal is only confusing. Other tools like
lsusb and dfu-util always expect the hex codes, which you also find in
all pci.ids lists or even Windows .inf files.

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

I think "firmware" is a bit better than "file". Even if the part of
the file that we manipulate is the part that is not the actual
firmware :) It is really a "dfu_file" but we use that for the struct
so it would also be confusing.

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

I think you should rebase/squash the commits there before you pull
this into master.

Cheers,
Tormod

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Stefan Schmidt-2
Hello.

On Sat, 2012-03-24 at 10:41, Tormod Volden wrote:

> On Wed, Mar 7, 2012 at 1:03 PM, Stefan Schmidt wrote:
> >
> >> 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.
>
> I got a reply from Leaflabs and they were going to test things. But
> since then I haven't heard anything. At the minimum I would like a
> confirmation from them on which USB ID's need to be quirked.

I'm not in a rush with 0.6 here. Still waiting for an unknown time
might also not really help the issue. :)

> >> > For the next release cycle I would like to see how we can integrate
> >> > the windows support Satz Klauer wrote.
> >
> > 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
>
> I had a quick look. It is based off an older tree so we must be
> careful not to revert anything accidentally. But I suppose your plan
> is to introduce the changes piece by piece anyway. Some of the changes
> there make sense in any case, like adding "const" qualifiers. We can
> also port all file handling to use FILE, fread() and friends from
> stdio instead of the less portable read().

Indeed, the plan was picking things piece by piece and keeping the
current state without regressions. My time schedule might get tight
again in the next weeks to months (I'm relocating into another
country) but I still plan to work on this. Once I have split some
things out into smaller changes I will push them to a branch and let
you know for review.

> There were things there I did not like, but we can get back to this
> once we have split up patches for this.

Agreed

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

Heh, maybe not, but who knows? :)

> >> It could even be an idea to use the same syntax as the -d option of dfu-util.

I dislike this syntax of squeezing both vendor and product IDs into
one string. Here it would be even a third ID with the device ID. I
prefer to keep them separate here.

> > 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.
> >
>
> These are IDs and not numbers or addresses. So adding the possibility
> to enter these values in decimal is only confusing. Other tools like
> lsusb and dfu-util always expect the hex codes, which you also find in
> all pci.ids lists or even Windows .inf files.

OK, you won. I will move it back to accept the ID as hex without 0x
prefix. Still I will keep them as three distinct arguments.

> >> 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?
>
> I think "firmware" is a bit better than "file". Even if the part of
> the file that we manipulate is the part that is not the actual
> firmware :) It is really a "dfu_file" but we use that for the struct
> so it would also be confusing.

Hmm, firmware is still bothering. Not much better then file imho to
just change it everywhere.

> > I did all changes based on your review in one commit:
> > http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix
>
> I think you should rebase/squash the commits there before you pull
> this into master.

Sure, that was the plan. Are you happy with the changes as is? If yes
I will push them into master after squashing and rebasing. The change
for the ID format will go in before obviously.

regards
Stefan Schmidt

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Tormod Volden
On Tue, Mar 27, 2012 at 5:10 PM, Stefan Schmidt wrote:
>> I got a reply from Leaflabs and they were going to test things. But
>> since then I haven't heard anything. At the minimum I would like a
>> confirmation from them on which USB ID's need to be quirked.
>
> I'm not in a rush with 0.6 here. Still waiting for an unknown time
> might also not really help the issue. :)

Yeah, I have pinged them again.

> Indeed, the plan was picking things piece by piece and keeping the
> current state without regressions. My time schedule might get tight
> again in the next weeks to months (I'm relocating into another
> country) but I still plan to work on this. Once I have split some
> things out into smaller changes I will push them to a branch and let
> you know for review.
>

Good luck with moving!

>> >> It could even be an idea to use the same syntax as the -d option of dfu-util.
>
> I dislike this syntax of squeezing both vendor and product IDs into
> one string. Here it would be even a third ID with the device ID. I
> prefer to keep them separate here.
>

No, you must like vid:pid :) everybody is using it :) But keeping it
separate is totally fine here.

>> > 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?
>>
>> I think "firmware" is a bit better than "file". Even if the part of
>> the file that we manipulate is the part that is not the actual
>> firmware :) It is really a "dfu_file" but we use that for the struct
>> so it would also be confusing.
>
> Hmm, firmware is still bothering. Not much better then file imho to
> just change it everywhere.
>

Yes, just leave it as it is for now.

>> > I did all changes based on your review in one commit:
>> > http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix
>>
>> I think you should rebase/squash the commits there before you pull
>> this into master.
>
> Sure, that was the plan. Are you happy with the changes as is? If yes
> I will push them into master after squashing and rebasing. The change
> for the ID format will go in before obviously.

Please let us test from the rebased branch for a bit. Otherwise I
think everything looks fine, but I haven't tested it.

Cheers,
Tormod

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Stefan Schmidt-2
Hello.

On Tue, 2012-03-27 at 21:04, Tormod Volden wrote:
> On Tue, Mar 27, 2012 at 5:10 PM, Stefan Schmidt wrote:
> >> I got a reply from Leaflabs and they were going to test things. But
> >> since then I haven't heard anything. At the minimum I would like a
> >> confirmation from them on which USB ID's need to be quirked.
> >
> > I'm not in a rush with 0.6 here. Still waiting for an unknown time
> > might also not really help the issue. :)
>
> Yeah, I have pinged them again.

Thanks

> Good luck with moving!

Thanks as well. :)

> >> >> It could even be an idea to use the same syntax as the -d option of dfu-util.
> >
> > I dislike this syntax of squeezing both vendor and product IDs into
> > one string. Here it would be even a third ID with the device ID. I
> > prefer to keep them separate here.
> >
>
> No, you must like vid:pid :) everybody is using it :) But keeping it
> separate is totally fine here.

:)

> >> > 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?
> >>
> >> I think "firmware" is a bit better than "file". Even if the part of
> >> the file that we manipulate is the part that is not the actual
> >> firmware :) It is really a "dfu_file" but we use that for the struct
> >> so it would also be confusing.
> >
> > Hmm, firmware is still bothering. Not much better then file imho to
> > just change it everywhere.
> >
>
> Yes, just leave it as it is for now.

Yup, maybe revisit this this "issue" later.

> >> > I did all changes based on your review in one commit:
> >> > http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix
> >>
> >> I think you should rebase/squash the commits there before you pull
> >> this into master.
> >
> > Sure, that was the plan. Are you happy with the changes as is? If yes
> > I will push them into master after squashing and rebasing. The change
> > for the ID format will go in before obviously.
>
> Please let us test from the rebased branch for a bit. Otherwise I
> think everything looks fine, but I haven't tested it.

Will let you know once I did the ID change, squashed, rebased and
tested myself.

regards
Stefan Schmidt

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Tormod Volden
On Wed, Mar 28, 2012 at 12:09 PM, Stefan Schmidt wrote:

>
>> >> > I did all changes based on your review in one commit:
>> >> > http://cgit.openezx.org/dfu-util/commit/?h=dfu-suffix
>> >>
>> >> I think you should rebase/squash the commits there before you pull
>> >> this into master.
>> >
>> > Sure, that was the plan. Are you happy with the changes as is? If yes
>> > I will push them into master after squashing and rebasing. The change
>> > for the ID format will go in before obviously.
>>
>> Please let us test from the rebased branch for a bit. Otherwise I
>> think everything looks fine, but I haven't tested it.
>
> Will let you know once I did the ID change, squashed, rebased and
> tested myself.
>
> regards
> Stefan Schmidt

Hi,
In the meantime, I have done the ID change, squash and rebase and
pushed it here:
http://gitorious.org/~tormod/unofficial-clones/dfuse-dfu-util/commits/master-patches
and have started to add some patches on top of this. But I can need to
test more.

Nothing new from LeafLabs yet, but I have added a tentative patch for
a Maple quirk.

Next I would like to port the file handling to stdio streams and fix
some C standards compliance
that should help for porting to other OS environments.

Cheers,
Tormod

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [dfu-util] New dfu-suffix manipulation tool

Tormod Volden
On Wed, Apr 11, 2012 at 11:25 PM, Tormod Volden wrote:

> Hi,
> In the meantime, I have done the ID change, squash and rebase and
> pushed it here:
> http://gitorious.org/~tormod/unofficial-clones/dfuse-dfu-util/commits/master-patches
> and have started to add some patches on top of this. But I can need to
> test more.
>
> Nothing new from LeafLabs yet, but I have added a tentative patch for
> a Maple quirk.
>
> Next I would like to port the file handling to stdio streams and fix
> some C standards compliance
> that should help for porting to other OS environments.

Finally, I have done all that, and I would welcome any testing,
especially on Windows,
which I can not test myself. Please mail me your full build logs. I
have tested on
linux/i686 and macosx 10.4.11 (powerpc), both with gcc.

Porting to stdio streams has caused one temporary change: Uploading to
a file (-U) will
overwrite existing files, which it would refuse to do earlier. It
seems difficult to fix this in
a clean, portable way, but I will come up with a hack later. But for
your testing, please
have your firmware files backed up somewhere :)

Cheers,
Tormod

_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Re: [dfu-util] New dfu-suffix manipulation tool

Daniel Li
Tormod Volden,

Is there any simple guide or steps to build a windows version?

>On Wed, Apr 11, 2012 at 11:25 PM, Tormod Volden wrote:
>> Hi,
>> In the meantime, I have done the ID change, squash and rebase and
>> pushed it here:
>> http://gitorious.org/~tormod/unofficial-clones/dfuse-dfu-util/commits/master-patches 
>> and have started to add some patches on top of this. But I can need to
>> test more.
>>
>> Nothing new from LeafLabs yet, but I have added a tentative patch for
>> a Maple quirk.
>>
>> Next I would like to port the file handling to stdio streams and fix
>> some C standards compliance
>> that should help for porting to other OS environments.
>
>Finally, I have done all that, and I would welcome any testing,
>especially on Windows,
>which I can not test myself. Please mail me your full build logs. I
>have tested on
>linux/i686 and macosx 10.4.11 (powerpc), both with gcc.
>
>Porting to stdio streams has caused one temporary change: Uploading to
>a file (-U) will
>overwrite existing files, which it would refuse to do earlier. It
>seems difficult to fix this in
>a clean, portable way, but I will come up with a hack later. But for
>your testing, please
>have your firmware files backed up somewhere :)
>
>Cheers,
>Tormod
>
>_______________________________________________
>devel mailing list
>[hidden email]
>https://lists.openmoko.org/mailman/listinfo/devel 
>
>
>-----
>No virus found in this message.
>Checked by AVG - www.avg.com
>Version: 2012.0.1913 / Virus Database: 2411/4936 - Release Date: 04/14/12
>
_______________________________________________
devel mailing list
[hidden email]
https://lists.openmoko.org/mailman/listinfo/devel
Loading...