[PATCH v2 12/15] Firmware upgrade for OSTC3

[Jef Driesen]

On 2014-12-17 23:11, Anton Lundin wrote:
> +dc_status_t
> +hw_ostc3_device_fwupdate (dc_device_t *abstract, const char *filename)
> +{
> +	[...]
> +
> +	// Make sure the device is in service mode
> +	if (device->state == OPEN) {
> +		rc = hw_ostc3_device_init_service(abstract);
> +		if (rc != DC_STATUS_SUCCESS) {
> +			free (firmware);
> +			return rc;
> +		}
> +	} else if (device->state != SERVICE) {
> +		free (firmware);
> +		return DC_STATUS_INVALIDARGS;
> +	}

I've been playing with the idea of merging this functionality into the 
hw_ostc3_check_state_or_init() function. This is what I came up with:

static dc_status_t
hw_ostc3_device_init (hw_ostc3_device_t *device, hw_ostc3_state_t state)
{
     dc_status_t rc = DC_STATUS_SUCCESS;

     if (device->state == state) {
         // No change.
         rc = DC_STATUS_SUCCESS;
     } else if (device->state == OPEN) {
         // Change to download/or service mode.
         if (state == DOWNLOAD) {
             rc = hw_ostc3_device_init_download(device);
         } else if (state == SERVICE) {
             rc = hw_ostc3_device_init_service(device);
         } else {
             rc = DC_STATUS_INVALIDARGS;
         }
     } else if (device->state == SERVICE && state == DOWNLOAD) {
         // Switching between service and download mode is not possible.
         // But in service mode, all download commands are supported too,
         // so there is no need to change the state.
         rc = DC_STATUS_SUCCESS;
     } else {
         // Not supported.
         rc = DC_STATUS_INVALIDARGS;
     }

     return rc;
}

What do you think? From my point of view, the main advantage is that all 
state changes are now done in a single function, where it's easy to see 
what kind of state transitions are allowed or not.

> +	// Programing done!
> +	progress.current++;
> +	device_event_emit (abstract, DC_EVENT_PROGRESS, &progress);
> +
> +	// Finished!
> +	return DC_STATUS_SUCCESS;
> +}

There is a memory leak here. You forgot to free the firmware structure.


PS: I have already implemented most of the improvements and bugfixes I 
mentioned in this and the 3 other emails. So there is no need to spend 
any time on them. If you're fine with those changes, just tell me, and 
I'll just squash them onto your commits.

I fixed a few style issues too (e.g. whitespace, signed vs unsigned int, 
and dc_device_t vs hw_ostc3_device_t in function parameters).

Jef