dfu-util pull request
Tormod Volden
lists.tormod at gmail.com
Sun Apr 22 00:07:26 CEST 2012
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
More information about the devel
mailing list