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