[PATCH v2 12/15] Firmware upgrade for OSTC3

[Anton Lundin]

On 19 December, 2014 - Jef Driesen wrote:

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

Yea, i think this one looks better. Go with this one.


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

Of course, failing on the goal line.

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

I still like my way of parsing the firmware files better than yours, but
if you want it that way, fine.


//Anton


-- 
Anton Lundin	+46702-161604