Added --crc argument to upload

[kivkiv12345]

Tested with firmware update.

Why don't we have this already?

[troelsjessen]

This might cause a WDT on Zynq or alternatively a timeout if configured to dlf 1 sec, when uploading a 4 MB PL, but let's try it out.

[johandc]

A few additional thoughts from my side:

Program was a combination of the upload, followed by the download commannd, and a local checksum being calculated. Then we got the checksum on-board, and program got the -C or --crc32 argument which is fine. And we also have just the checksum command.

I feel like upload, download and checksum should remain as low level commands that do just what they say. But yes it might be a good idea to add --crc as an optional argument. That means it becomes essentially the same as the program command. (except for the slot logic) You could even add the slot logic to the upload command, and then they become fully the same.

Separation of return values might be important, so if the script is:

upload 0x00004000 firmware.bin
checksum 0x00004000 firmware.bin

Then you know that if upload return failure, you should resume. And if checksum fails, you should abort and start all over.

upload --crc 0x00004000 firmware.bin

Might fail with multiple error codes and be more complex to manage logically. Handling multiple exit codes is not a thing in CSH anyways, so when humans are operating, it may be preferable to say "run upload command til it finishes, then run the checksum"

[troelsjessen]

The crc32 command does what the checksum command proposed by Johan is assumed to do.

But I see not problem with adding -C / --crc to the uplink (and downlink?) command

[troelsjessen]

Instead of duplicating this piece of code, please see if you can re-format the code from vmem_client_slash_crc32(...) to enable re-use.

[kivkiv12345]

I think the only piece that meakes sense to reuse is:

if (crc_file == crc) {
	printf("\033[32m\n");
	printf("  Success\n");
	printf("\033[0m\n");
} else {
	printf("\033[31m\n");
	printf("  Failure: %"PRIX32" != %"PRIX32"\n", crc, crc_file);
	printf("\033[0m\n");
}

Is it worth making a standalone function for a print?

[troelsjessen]

Generally yes, if the same print is performed from multiple places, but why not reuse the call to csp_crc32_memory and vmem_client_calc_crc32?

[troelsjessen]

Remember this book? :)
https://en.wikipedia.org/wiki/Don%27t_repeat_yourself

[kivkiv12345]

I'm not at all opposed to different return values, I don't think it makes much of a difference now that we no longer show it to the user. For exception handling we have Python, where the different errors raise different exceptions.

2 commands allow us to avoid redundant uploads:

upload 0x00004000 firmware.bin
checksum 0x00004000 firmware.bin

But I doubt redundant uploads are much slower for general usage:

upload --crc 0x00004000 firmware.bin

[johandc]

I have only one problem which is, if the packet containing the checksum does not go through. The upload command may have to restart, and do some work, in order to trigger a new checksum to be requested. Having checksum as a separate request, means it can be safely retried multiple times. But of course, it does not hurt adding the option to the upload command as such.

[kivkiv12345]

I agree with that concern, but I highly doubt users will start using the checksum functionality if it is a separate command (A flag is probably even pushing it).
Perhaps including retry logic (at least for checksum) is the best compromise?