[PATCH v2 12/15] Firmware upgrade for OSTC3

[Anton Lundin]

On 20 December, 2014 - Anton Lundin wrote:

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

I did do a round of cleanup, and to test how things looked with the
hw_ostc3_device_init-code and they are all how cleaned and fixed on
github.

If you would like to see them, i can send them out, or you can look at:
https://github.com/glance-/libdivecomputer/tree/ostc3_fw


//Anton


-- 
Anton Lundin	+46702-161604