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