[PATCH 10/11] Firmware upgrade for OSTC3

[Jef Driesen]

On 2014-11-21 21:28, Anton Lundin wrote:
> +dc_status_t
> +hw_ostc3_device_fwupdate (dc_device_t *abstract, const char *filename)
> +{
> +	dc_status_t rc = DC_STATUS_SUCCESS;
> +	hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
> +	dc_context_t *context = (abstract ? abstract->context : NULL);
> +
> +	// Enable progress notifications.
> +	dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER;
> +
> +	if (!ISINSTANCE (abstract))
> +		return DC_STATUS_INVALIDARGS;
> +
> +	if (device->state == OPEN) {
> +		if ((rc = hw_ostc3_device_service_mode(abstract)) != 
> DC_STATUS_SUCCESS)
> +			return rc;
> +	} else if (device->state != SERVICE) {
> +		return DC_STATUS_INVALIDARGS;
> +	}
> +	// load, erase, upload FZ, verify FZ, reprogram
> +	progress.maximum = 3 + SZ_FIRMWARE * 2 / SZ_FIRMWARE_BLOCK;
> +	device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
> +
> +	hw_ostc3_device_display(abstract, " Loading FW...");
> +
> +	// Allocate memory for the firmware data.
> +	hw_ostc3_firmware_t *firmware = (hw_ostc3_firmware_t *) malloc
> (sizeof (hw_ostc3_firmware_t));
> +	if (firmware == NULL) {
> +		ERROR (context, "Failed to allocate memory.");
> +		return DC_STATUS_NOMEMORY;
> +	}
> +
> +	// Read the hex file.
> +	rc = hw_ostc3_firmware_readfile (firmware, context, filename);
> +	if (rc != DC_STATUS_SUCCESS) {
> +		free (firmware);
> +		return rc;
> +	}
> +
> +	// Firmware loaded
> +	progress.current++;
> +	device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);

I think it's better to load the firmware file before trying to boot into 
service mode. If there is anything wrong with the firmware file, then 
there is no point to boot into service mode. That might save user some 
trouble.

Reading the firmware file should be instant compared to the time it 
takes to flash it. So the fact that we won't reporting any progress for 
this stage won't be an issue.

BTW, how do you exit service mode? Here, I'm thinking about the case 
where something goes wrong and we have to abort the firmware upgrade. If 
I understand correctly, the original firmware will be preserved, so the 
device can't be bricked. But I assume it's more re-assuring to the user 
if the device boots again, rather than staying in service mode. Note 
that I haven't tested the firmware update yet, so if this is something 
obvious just ignore the question :-)

> +	hw_ostc3_device_display(abstract, " Erasing FW...");

I'm not really sure what's the best way to deal with these 
notifications. When doing this from within libdivecomputer, then it 
won't be possible to localize these messages. That might be confusing 
because the ostc firmware is available in several languages. Doing this 
on the application is not impossible, but there you only have access to 
the progress events, so you could show the overall percentage but not 
the stage (e.g. erasing, uploading, verifying and flashing).

Suggestions are welcome.

> +		rc = hw_ostc3_device_read_block (device, FIRMWARE_AREA + len,
> block, sizeof(block));
> +		if (rc != DC_STATUS_SUCCESS || memcmp (firmware->data + len, block,
> sizeof (block)) != 0) {
> +			hw_ostc3_device_display (abstract, " Verify FAILED");
> +			free (firmware);
> +			ERROR (context, "Failed verify.");
> +			return DC_STATUS_IO;
> +		}

If hw_ostc3_device_read_block() fails, you should return rc, like you 
already do elsewhere. Not DC_STATUS_IO. And when the memcmp() fails, the 
most appropriate error code is DC_STATUS_PROTOCOL.

Jef