dfu-util pull request

Hello.

Greetings from UK! I'm here for one week now and and work and life
still needs a lot of time to settle. Thus I really appreciate you
preparing a pull request putting all the stuff in one place for me to
review and pull from. Saves a lot of my time.

Post by Tormod Volden
I think we are close to 0.6 now :) I fixed the overwriting files issue
in the stdio porting, and finished off other small things on my to-do
list. There are many patches so I am not going to spam the list with
them. Please pull or review it from
http://gitorious.org/unofficial-clones/dfuse-dfu-util/graph/master-patches
If you see any issues or want to comment on a patch, I can also submit
individual patches to the list for discussion, just let me know.

I just tested the complete set with all my devices and found no
regression. Good job.

Besides some smaller comments this all looks pretty nice.

Post by Tormod Volden
Fix segfault regression in option handling (2012-02-24 23:24:55 +0100)
git://gitorious.org/~tormod/unofficial-clones/dfuse-dfu-util.git
master-patches
Add quirk for Maple since it reports wrong DFU version (2012-04-18
22:38:28 +0200)
----------------------------------------------------------------
dfu_file: Add function to generate and add a DFU suffix to a file

Thanks for the leak fix on this one.

Post by Tormod Volden
dfu-suffix: New DFU suffix manipulation tool
dfu_file: Verify that we could read the whole file
dfu_file: Return failure if CRC does not match
dfu-suffix: Print version before banner, and do it every time

All above are fine.

Post by Tormod Volden
Port file handling to stdio streams
Do not overwrite existing files when uploading

Might be a good idea to squeeze the overwrite fix into the file
handling commit. Its a fix for this commit anyway. Changes the other
things incremental is fine but without this squashed we would have a
regression. Especially one that overrides a user file. :/ Sure
therefore the user would need to compile it with the file IO port
patch but without the fix. Still good practice.

The fseek() for sync read/write streams was interesting, btw.

Post by Tormod Volden
dfu-suffix: Check for availability of truncate() function

You happen to know which platforms are missing this?

Post by Tormod Volden
Check for availability of getpagesize()
Replace usleep() and sleep() by milli_sleep() macro

Is HAVE_WINDOWS_H really the preferred way to check for windows? Satz
used ENV_WINDOWS (which also looked odd to me). Could be that it
really depends on what you are using to create the executable. MinGW
vs. Windows compiler. Anyway, as you have tested this under Windows
and I also have no problems with it under Linux thats fine.

Post by Tormod Volden
configure.ac: Remove unnecessary tests
Remove some obsolete, unused code
Use standard library (C99) types instead of OS types
Only conditionally include unistd.h
usb_dfu.h: Pack struct properly for Microsoft compilers
Various "pedantic" code compliance fixes

The usage string split into three printfs looks pretty ugly to me.

Post by Tormod Volden
Const'ify some strings and add some explicit casts
Do not use leading underscores in #include guards
dfuse: Add progress indication on download

For consistency I would go with a # here instead of a dot. The
non-DfuSe part of dfu-util uses it for progress indication.

Post by Tormod Volden
Detect DfuSe device correctly on big-endian architectures
main: Stop guessing transfer size, error out instead

I agree, its about time to rely on the functional descriptor. Lets see
if any bug reports will come for this. :)

Post by Tormod Volden
main: Update copyright banner
Add quirk for Maple since it reports wrong DFU version

So this is without confirmation from leaflabs, right? Its fine anyway
as it can only improve the situation. :)

The squashing of the bug fix in the previous change is the only thing
that would block me from pulling this in. If you could fix this up I
would happily pull in an updated pull request.

Tormod Volden

2012-04-21 22:07:26 UTC

Post by Tormod Volden
----------------------------------------------------------------
dfu_file: Add function to generate and add a DFU suffix to a file

Thanks for the leak fix on this one.

All above are fine.

Post by Tormod Volden
Port file handling to stdio streams
Do not overwrite existing files when uploading

Yes, totally fine with me. I had just kept it in a separate commit
because the porting itself was kind of search and replace, and this
append/fseek "hack" was the only non-obvious thing.

So I have now squashed it and re-pushed, but kept the long explaining
commit messages.

(Of course, the first commit would not overwrite a user file unless
the user tried to overwrite it!)

Post by Stefan Schmidt
The fseek() for sync read/write streams was interesting, btw.

Post by Tormod Volden
dfu-suffix: Check for availability of truncate() function

You happen to know which platforms are missing this?

No, but I would guess everything non-posix. I believe, unless you are
using cygwin, the posix layer on Windows is not so complete. I know
that when compiling with MinGW, truncate() is not available. I just
found this http://stackoverflow.com/questions/584639/truncate-file so
it looks like we can use SetEndOfFile() on Windows instead. I will
look into that another day.

Post by Tormod Volden
Check for availability of getpagesize()
Replace usleep() and sleep() by milli_sleep() macro

http://msdn.microsoft.com/en-us/library/windows/desktop/ms686298%28v=vs.85%29.aspx
says that you should include windows.h to use the Sleep() function. So
maybe not the preferred way to check for Windows but seems a good
guess for checking for Sleep().

The usage string split into three printfs looks pretty ugly to me.

Yes, that seems pretty random, but I estimated it to keep each string
below the standard max length, and I kind of grouped options together
logically. Another way would be to have printf's on every line...

Post by Tormod Volden
Const'ify some strings and add some explicit casts
Do not use leading underscores in #include guards
dfuse: Add progress indication on download

For consistency I would go with a # here instead of a dot. The
non-DfuSe part of dfu-util uses it for progress indication.

Yes, but I did not add the text and delimiters around it either. I see
many tools use dots for this kind of progress. For now it makes it
easier for me to spot in a log if the right download option is being
used, so consistency is not a goal :) I can reconsider this later,
maybe we can use dots everywhere.

Post by Tormod Volden
Detect DfuSe device correctly on big-endian architectures
main: Stop guessing transfer size, error out instead

I agree, its about time to rely on the functional descriptor. Lets see
if any bug reports will come for this. :)

BTW, I have come to realize that we are getting no bug reports, so we
have to google for use of dfu-util and proactively discover the users'
problems. That's how I found the Maple fall-out!

Post by Tormod Volden
main: Update copyright banner
Add quirk for Maple since it reports wrong DFU version

So this is without confirmation from leaflabs, right? Its fine anyway
as it can only improve the situation. :)

Yes, they are not responsive. Since they are shipping dfu-util in
their product, I expected a little bit more of cooperation from them.

Post by Stefan Schmidt
The squashing of the bug fix in the previous change is the only thing
that would block me from pulling this in. If you could fix this up I
would happily pull in an updated pull request.

Done!

Tormod Volden

2012-04-22 09:02:36 UTC

Post by Tormod Volden
dfu-suffix: Check for availability of truncate() function

You happen to know which platforms are missing this?

I found ftruncate() on MinGW, so I changed to use that (with help of
fileno() to avoid reopening the file this got much cleaner too). For
non-MinGW Windows, it seems like _chsize_s is an equivalent (forget
about SetEndOfFile() which acts on Windows file handles). Since I can
not test that variant, and MinGW works fine, I am not including it.

Post by Tormod Volden
Check for availability of getpagesize()
Replace usleep() and sleep() by milli_sleep() macro

Since we are using HAVE_WINDOWS_H it implies auto-tools is being used,
so it could seem like checking for Sleep() would be the proper way.
However that does not work when cross-compiling on MinGW. Similar to
the getpagesize() issue this might be a bug in auto-tools or I don't
know how to use it properly.

Done!

Done again, rebased with the ftruncate fix. Before repulling, do a
fetch so you can git diff the small difference.

Tormod Volden

2012-04-22 09:50:20 UTC

Post by Tormod Volden
dfu-suffix: Check for availability of truncate() function

You happen to know which platforms are missing this?

BTW, I squashed this commit into the "porting to stdio" commit, since
it ended up as a small change, and even more so combined (it was
originally using ftruncate, so otherwise it would be a lot of back and
forth between commits).

Post by Tormod Volden
We could maybe wait to see if I can get suffix truncation work on
Windows easily, that would save some documentation and make dfu-util
fully consistent across architectures.

Should be fine now.

Tested and works fine on Windows 7.

Tormod

Stefan Schmidt

2012-04-22 13:41:21 UTC

Hello.

Post by Tormod Volden
dfu-suffix: Check for availability of truncate() function

You happen to know which platforms are missing this?

Great, works fine here as well.

Post by Tormod Volden
We could maybe wait to see if I can get suffix truncation work on
Windows easily, that would save some documentation and make dfu-util
fully consistent across architectures.

Should be fine now.

Tested and works fine on Windows 7.

Cool. I pulled it in, did a changelog, tagged it and now I'm in the
final testing before I push it all out (ETA 30min). I will wait with the
announcement for you to prepare the windows executable from either the
git tag or the release tarball. Docs about building for windows would
be a nice bonus, but for the release I only would like the windows
binary. Please include the version in the filename and also prepare a
.md5 file with the checksum. This way it can go straight to the
release folder with the tarball. Obviously take your time for this.
The final announcement can wait a day or two if you have more
important or better stuff to do. :)

regards
Stefan Schmidt

Stefan Schmidt

2012-04-22 14:14:09 UTC