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