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

[Jef Driesen]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: wip_msvc.patch
Type: text/x-patch
Size: 1172 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141217/78ccd3bb/attachment-0001.bin>