On 15 December, 2014 - Jef Driesen wrote:
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.)
Zomg, but i hear you. Fixing.
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.
I just thought it would be sane to actually represent all the states we can have, if we where to change something in the future in how we do things.
+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)
This might be a bit over cautious, Fixed.
@@ -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.
Makes sense. Fixed.
// 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.
Fixed.
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.
Fixed.
//Anton