[PATCH 08/11] Add code to write rom of the OSTC3

[Jef Driesen]

On 2014-11-21 21:28, Anton Lundin wrote:
> +static dc_status_t
> +hw_ostc3_device_write_block (dc_device_t *abstract, unsigned int
> addr, unsigned char block[], unsigned int block_size)
> +{
> +	dc_status_t rc = DC_STATUS_SUCCESS;
> +	hw_ostc3_device_t *device = (hw_ostc3_device_t *) abstract;
> +	dc_context_t *context = (abstract ? abstract->context : NULL);
> +#if 0
> +	// FIXME: This variant doens't work
> +	// Things doesn't get written to memory. Timing issues?
> +	// Version below works, serial_sleep (2) is probably the thing.
> +	unsigned char buffer[4 + block_size];
> +	buffer[0] = WRITE_BLOCK;
> +	uint24_be_array(addr, buffer + 1);
> +	memcpy(buffer + 4, block, block_size);
> +
> +	rc = hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer,
> sizeof(buffer), NULL, 0);
> +	serial_sleep (device->port, 1100);
> +	return rc;
> +#else
> +	int n;
> +	unsigned char response;
> +	unsigned char buffer[4];
> +	buffer[0] = WRITE_BLOCK;
> +	uint24_be_array(addr, buffer + 1);
> +
> +	n = serial_write (device->port, buffer, sizeof(buffer));
> +	if (n != sizeof(buffer)) {
> +		ERROR (context, "failed to send address to device");
> +		return EXITCODE (n);
> +	}
> +
> +	serial_flush (device->port, SERIAL_QUEUE_BOTH);
> +	serial_sleep (device->port, 2);
> +
> +	// Send the new block
> +	n = serial_write (device->port, block, block_size);
> +	if (n != block_size) {
> +		ERROR (context, "failed to send block to device");
> +		return EXITCODE (n);
> +	}
> +
> +	serial_flush (device->port, SERIAL_QUEUE_BOTH);

Do we really need those two flushes? They might cause serious problems. 
When writing data to a serial port, the data ends up in the output 
buffer of the driver/hardware, and is transmitted async. Thus when the 
write call returns, the data is not always guaranteed to be transmitted 
yet. That's why libdivecomputer also calls tcdrain internally, but even 
that is not always reliable.

Normally this isn't a real problem, because if the data isn't 
transmitted yet, the next read, to receive the response, will just take 
a bit longer. But if you flush the input/output buffers, then you might 
throw away the data you just send. And if the response arrives fast, you 
might throw that away as well.

The serial_flush() call is typically only necessary after opening the 
serial port, in order to discard any garbage data that may have been 
generated while connecting the serial port, or data that somehow was 
somehow still sitting in the internal buffers. The second use case is 
when receiving invalid packets. Then you flush the buffers in order in 
order to discard all received data and start again from a clean state. 
But neither of that applies here.

I just checked the ostc companion source, and their implementation of 
the flush function does something completely different than the 
libdivecomputer one. They call tcdrain internally. The libdivecomputer 
flush function corresponds to the purge function in the ostc companion. 
So I'm pretty sure that confirms those two serial_flush() calls should 
be removed.


I'm still surprised by the need for that 2ms delay. But okay, if it's 
necessary then so be it. Note that on Windows the delay will be a lot 
longer than those 2ms. A Windows Sleep() call will typically take at 
least 10-15ms.

> +	// A sleep lenght copied from ostc-companion
> +	// Feels a bit slower than that what that does?
> +	serial_sleep (device->port, 1100);

Why do we need this one? I might be wrong, but I think it does something 
different than what you think. Based on the comments in the companion 
code, it waits the time it takes to write the block to flash. I'm not 
exactly sure how it's calculated.

In theory, the 3000ms timeout we're using for reading data should be 
able to handle this delay already. Unless the S_READY is send much 
later. You should be able to see this in the timings when you enable the 
libdivecomputer debug log.


Another thing I noticed is that your sequence is quite different from 
the companion one. When mapping the companion calls to the equivalent 
libdivecomputer calls, it does this:

sleep(100)
flush(SERIAL_QUEUE_BOTH)

write(buffer + 0, 1)
read(echo, 1)
write(buffer + 1,3)
sleep(2)
write(block, block_size)
sleep(1100)
read(S_READY,1)

The first sleep and flush are most likely unnecessary. Such a cleanup is 
only necessary when something went wrong. Since we have no code to 
recover from errors, that's not necessary.

With the exception of those sleep calls, that's identical to the 
hw_ostc3_transfer() function. Notice how your code is missing the read 
of the echo. I'm pretty sure that got lost due to the flushing. So this 
probably works correctly only by accident.

The reason why hw_ostc3_transfer doesn't work for you, might be because 
you include the WRITE_BLOCK byte in the input buffer. That causes it to 
send twice. The ostc probably doesn't like that. Can you have a look at 
this? I have the feeling that we can use hw_ostc3_transfer here after 
all. Worst case we may need to add those sleep calls there, wrapped in a 
"if (cmd == WRITE_BLOCK)".

> +	n = serial_read (device->port, &response, sizeof (response));
> +	if (n != sizeof (response) || response != S_READY)
> +		return EXITCODE (n);

You should split those checks in two parts. Right now you'll return 
DC_STATUS_TIMEOUT when response!=S_READY, and that's misleading. If we 
can re-use hw_ostc3_transfer, then this is no longer an issue.

Jef