dfu-util pull request

[Tormod Volden]

On Sat, Apr 21, 2012 at 11:01 PM, Stefan Schmidt wrote:
>> ----------------------------------------------------------------
>> Stefan Schmidt (2):
>>       dfu_file: Add function to generate and add a DFU suffix to a file
>
> Thanks for the leak fix on this one.
>
>>       dfu-suffix: New DFU suffix manipulation tool
>
>
>> Tormod Volden (21):
>>       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.
>
>>       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.

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

>
> The fseek() for sync read/write streams was interesting, btw.
>
>>       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.

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

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

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

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

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

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

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

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

> From my side we would be ready for a 0.6 release afterwards. :)

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.

Cheers,
Tormod