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