On 16-12-14 20:06, Anton Lundin wrote:
On 16 December, 2014 - Jef Driesen wrote:
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
...
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.
After i started to rewrite the code for like the .. 7th time i spotted the bug in the hw_ostc3_transfer-based variant, and indeed it was the extra WRITE_BLOCK i never spotted.
Now the hw_ostc3_device_write_block-helper looks like this:
static dc_status_t hw_ostc3_device_write_block (hw_ostc3_device_t *device, unsigned int addr, unsigned char block[], unsigned int block_size) { unsigned char buffer[3 + block_size]; array_uint24_be_set(buffer, addr); memcpy(buffer + 3, block, block_size);
return hw_ostc3_transfer(device, NULL, WRITE_BLOCK, buffer, sizeof(buffer), NULL, 0); }
Just like hw_ostc3_device_read_block, and works just fine.
Excellent news! Just one comment. Use a fixed size array here. Variable Length Arrays (VLA) are not supported in C++, so this breaks msvc compatibility :-(
There are also a few other minor issues for msvc. See the attached patch.
(For the snprintf function, I should make some helper function, because the semantics of the msvc _snprintf is quite different. For example it does not always null terminated the string, which is really bad. What the hell where they thinking?)
Jef