On 21-11-14 21:28, Anton Lundin wrote:
--- a/src/hw_ostc3.c +++ b/src/hw_ostc3.c @@ -67,6 +67,11 @@ typedef struct hw_ostc3_device_t { dc_device_t base; serial_t *port; unsigned char fingerprint[5];
- enum hw_ostc3_state {
NONE,OPEN,DOWNLOAD,- } state; } hw_ostc3_device_t;
Move the enum outside of the struct and give it a proper typedef. (This is mainly for the msvc C++ compiler. When defined inside the struct you need to prefix the values with the C++ hw_ostc3_device_t:: syntax. And that doesn't work in C mode.)
static dc_status_t hw_ostc3_device_set_fingerprint (dc_device_t *abstract, const unsigned char data[], unsigned int size); @@ -220,6 +225,7 @@ hw_ostc3_device_open (dc_device_t **out, dc_context_t *context, const char *name // Set the default values. device->port = NULL; memset (device->fingerprint, 0, sizeof (device->fingerprint));
device->state = NONE;
// Open the device. int rc = serial_open (&device->port, context, name);
@@ -249,18 +255,48 @@ hw_ostc3_device_open (dc_device_t **out, dc_context_t *context, const char *name // Make sure everything is in a sane state. serial_sleep (device->port, 300); serial_flush (device->port, SERIAL_QUEUE_BOTH);
- device->state = OPEN;
- *out = (dc_device_t *) device;
- return DC_STATUS_SUCCESS;
+}
Why do we need both NONE and OPEN? I don't see any difference between the two. The api prevents you from returning an handle in the NONE state. So I see no reason to keep the OPEN state.
+static dc_status_t +hw_ostc3_device_init (dc_device_t *abstract) +{
- hw_ostc3_device_t *device = (hw_ostc3_device_t*) abstract;
- dc_context_t *context = (abstract ? abstract->context : NULL);
- if (device->state != OPEN)
return DC_STATUS_INVALIDARGS;
This condition can never happen. (see previous comment)
@@ -269,14 +305,20 @@ static dc_status_t hw_ostc3_device_close (dc_device_t *abstract) { hw_ostc3_device_t *device = (hw_ostc3_device_t*) abstract;
- dc_status_t status;
- // Send the exit command.
- dc_status_t status = hw_ostc3_transfer (device, NULL, EXIT, NULL, 0, NULL, 0);
- if (status != DC_STATUS_SUCCESS) {
ERROR (abstract->context, "Failed to send the command.");serial_close (device->port);free (device);return status;
- if (device->state == NONE)
return DC_STATUS_INVALIDARGS;
Same here. If it where possible, you even have a memory leak here. The close function should always clean up all resources. If the close fails, how are you supposed to clean up? The return value is purely informational.
// Close the device. @@ -313,6 +355,7 @@ dc_status_t hw_ostc3_device_version (dc_device_t *abstract, unsigned char data[], unsigned int size) { hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
- dc_status_t rc;
Initialize variables as much as possible.
if (!ISINSTANCE (abstract)) return DC_STATUS_INVALIDARGS; @@ -320,8 +363,11 @@ hw_ostc3_device_version (dc_device_t *abstract, unsigned char data[], unsigned i if (size != SZ_VERSION) return DC_STATUS_INVALIDARGS;
- if ((rc = hw_ostc3_check_state_or_init(abstract)) != DC_STATUS_SUCCESS)
return rc;
Move the assignment out of the if condition. That makes the code more readable.