[dfu-util] TI Stellaris prefix support for dfu-suffix

[Tommi Keisala]

On 07/04/2012 11:09 PM, Tormod Volden wrote:
>>> Address argument is not required when dfu-suffix is used to delete or check
>>> prefix. Deleting prefix needs to happen at the same time when DFU suffix is
>>> removed.
> I don't think that options can have optional argument when using
> getopt(). I think the -s option will swallow whatever word comes next
> whether it starts with a hyphen or not.

Yes you are correct. There is optional_argument option for getopt_long 
but that does not seem to work as I want it. And there seems to be a 
reason why it is implemented as it is. Anyhow I do not want to use it. 
So I added another option -t that requires no argument and is meant to 
use with deleting prefix (with -D). And option -s is for adding (-a).

Suggestions are very welcome how to handle this better.

>>> To add DFU suffix and prefix use:
>>> dfu-suffix -s 0x2000 -v 0x1cbe -p 0x00ff -d 0x0000 -a image.bin
>>>
>>> To remove DFU suffix and prefix use:
>>> dfu-suffix -s -D image.bin
> Did you test this? So I would think -D will be interpreted as a
> Stellaris address.

I guess I did not test this since it was not working.

> diff --git a/src/lmdfu.c b/src/lmdfu.c
> new file mode 100644
> index 0000000..d7a5bb7
> --- /dev/null
> +++ b/src/lmdfu.c
> @@ -0,0 +1,181 @@
> +/* This implements the TI Stellaris DFU
> + * as per the Application Update Using the USB Device Firmware Upgrade Class
> + * (Document AN012373)
> Maybe add the document link since it is difficult to find.
Added.
>>> + *
>>> + * (C) 2007-2008 by Harald Welte<laforge at gnumonks.org>
> Bad paste? I am not sure what Harald has to do with this code?
Not a bad paste. First patch had a lot copy-paste code from Harald. Not 
anymore so this is remove.
>
>> +
>> +/* dfu_prefix payload length excludes prefix and suffix */
>> +unsigned char dfu_prefix[] = {
> Maybe call this lmdfu_dfu_prefix[] or lmdfu_prefix[] for consistency?
Done.

>>> + }
>>> +
>>> + ret = ftruncate(fileno(file->filep), 0);
> We guard this with #ifdef HAVE_FTRUNCATE in src/suffix.c, just in case
> someone manage to compile it in some exotic environment.

Whole function implementation is now guarded.

>
>> +
>> +int lmdfu_check_prefix(struct dfu_file *file)
>> +{
>> + unsigned char *data;
>> + int ret;
>> +
>> + data = malloc(sizeof(dfu_prefix));
>> +
>> + ret = fread(data, 1, sizeof(dfu_prefix), file->filep);
>> + if (ret<  sizeof(dfu_prefix)) {
>> + fprintf(stderr, "Could not read prefix\n");
>> + perror(file->name);
>> + }
>> +
>> + if ((data[0] != 0x01)&&  (data[1] != 0x00)) {
>> + printf("Not valid TI Stellaris DFU prefix\n");
> This will be printed out for all "normal" files, since this function
> is unconditionally called from check_suffix, right? Would it be
> possible to reserve this to the case -s is used? Or are there
> advantages enough to always report on a missing TI prefix?
Good point. Don't see any reason to do this without -t or -s. 
Implementation changed.
>
>> diff --git a/src/suffix.c b/src/suffix.c
>> index fe19194..7b3f843 100644
>> --- a/src/suffix.c
>> +++ b/src/suffix.c
>> @@ -22,6 +22,7 @@
>>   #include<getopt.h>
>>
>>   #include "dfu_file.h"
>> +#include "lmdfu.h"
>>   #ifdef HAVE_CONFIG_H
>>   #include "config.h"
>>   #endif
>> @@ -47,6 +48,7 @@ static void help(void)
>>    "  -d --did\tAdd device ID into DFU suffix in<file>\n"
>>    "  -c --check\tCheck DFU suffix of<file>\n"
>>    "  -a --add\tAdd DFU suffix to<file>\n"
>> + "  -s --stellaris Add TI Stellaris prefic to<file>\n"
> s/prefic/prefix
>
> You do not mention the address argument here?
Fixed.
>
>> @@ -134,6 +132,8 @@ int main(int argc, char **argv)
>>    struct dfu_file file;
>>    int pid, vid, did;
>>    enum mode mode = MODE_NONE;
>> + unsigned int prefix_address=0;
> Maybe you should call this lmdfu_prefix_address? Or even lmdfu_start_address?
Changed to lmdfu_flash_address to implicate it is address in target 
flash memory.
> [sorry, gmail ate all the tabs when I quoted the patch] After taking a 
> look at the TI application note, I see TI offer a dfu-wrap command 
> line utility. Is your dfu-suffix version pretty much equivalent to 
> this? Then you might want to mention it somewhere to help people 
> googling for alternatives. Oh, I just did ;) We can also add that to 
> the dfu-util web page. Cheers, Tormod 

Yes you can do pretty much the same with dfuwrap tool from StellarisWare.

Thank you for the comments. And here's new patch.

-Tommi