[PATCH 10/11] Firmware upgrade for OSTC3

[Anton Lundin]

On 16 December, 2014 - Jef Driesen wrote:

> 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.
> 

Thats true. If a application where to open the device, and then call
hw_ostc3_device_fwupdate() and the read firmware call fails due to ex.
user choosing the wrong file, the application may want to keep the dc
descriptor open and do something else.

> 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 :-)
> 

You can either exit it by sending EXIT, as done in
hw_ostc3_device_close, or when unplugging the device from the usb-port.

The firmware upgrade is done by the bootloader in the device, and only
after the command to boot into the bootloader is sent, so you can abort
the upgrade process at any point and not brick the device.

> >+	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.
> 

I'd say its really nice to have some feedback on the device, thats why i
copied that pattern from ostc-companion and implemented it here. The
strings are also a bit tricky to craft due to the length limitations so
I'm just fine with the strings being in English and "hard-coded".

If there where a way to send more generic callbacks to the app, it would
be great to send the current state that way too, but you don't want the
app calling hw_ostc3-functions in the middle of your firmware upgrade
process.

I'd say we keep em as-is until someone comes up with a better solution.

> >+		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.
> 

Split them now.


//Anton

-- 
Anton Lundin	+46702-161604