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

[Jef Driesen]

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.