[PATCH 5/6] Cochran: Dump of Commander TM devices is now supported

[Jef Driesen]

On 2017-08-10 19:32, John Van Ostrand wrote:
> On Thu, Aug 10, 2017 at 11:00 AM, Jef Driesen <jef at libdivecomputer.org>
>> So this means that the byte sequences:
>> 
>> 05 9D FF 00 43 00
>> 05 BD 7F 00 43 00
>> 
>> that are send in the cochran_commander_read_id() function are actually
>> read operations at the addresses 0xFF9D and 0x7FBD (and a size of 
>> 0x0043
>> bytes). Interesting. I didn't realize that before.
> 
> 
> It seems so obvious, but yes, that's exactly what it means. It didn't 
> dawn
> on me until I had to get creative reading the Commander TM.

Having less "magic values" is always nice!

>> -       // Read the sample data, from 0 to sample end will include 
>> logbook
>>> -       rc = cochran_commander_read (device, &progress,
>>> device->layout->rb_logbook_begin, dc_buffer_get_data(buffer),
>>> device->layout->rb_profile_end);
>>> +       // Read the sample data, logbook and sample data are 
>>> contiguous
>>> +       rc = cochran_commander_read (device, &progress,
>>> device->layout->rb_logbook_begin, dc_buffer_get_data(buffer), size);
>>>         if (rc != DC_STATUS_SUCCESS) {
>>>                 ERROR (abstract->context, "Failed to read the sample
>>> data.");
>>>                 return rc;
>>> 
>> 
>> This can't work. The size variable is calculated as:
>> 
>> layout->rb_profile_end - layout->rb_logbook_begin
>> 
>> For the Commander TM that is 0x010000 bytes (0x020000 - 0x010000). But 
>> the
>> size field in the command is only 16 bits wide. So you'll try to read 
>> 0
>> bytes. Oops. I think the only solution here is to split the large read
>> operation into multiple smaller blocks.
> 
> 
> I thought so too but when I ask for ask for 0 bytes it returns 32K.

That's a bit weird, but if it works, fine. The rationale behind it is 
probably that doing a read operation for 0 bytes makes little sense, so 
they can re-use that value to mean 0x10000. In that case we should 
probably add some extra checks:

if (size > 0x10000)
     return DC_STATUS_INVALIDARGS;

if (size == 0)
     return DC_STATUS_SUCCESS;

and some comment saying that 0 is interpreted by the device as 0x10000. 
Otherwise someone will ask the same question in the future. And by that 
time we probably forgot about that detail :-)

Jef