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

On 21 December, 2014 - Jef Driesen wrote:

On 20-12-14 12:13, Anton Lundin wrote:

...

On 20 December, 2014 - Anton Lundin wrote:

...

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

It's a bit less efficient then your version (e.g. a few more fread calls), but I prefer to use the same pattern throughout the entire codebase. We already had this code for the ostc, so it doesn't make much sense to introduce another variant for the ostc3.

...

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

I did a few minor style cleanups here and there, and pushed everything to master.

Thanks for your work!

Thank you for doing a proper review and keeping my code honest.

//Anton