[PATCH 03/11] Lift the OSTC3 INIT out of open

[Anton Lundin]

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

-- 
Anton Lundin	+46702-161604