dfu-util pull request
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
(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.
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.
> 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.
More information about the devel