Re: [PATCH v2 12/15] Firmware upgrade for OSTC3

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