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

[John Van Ostrand]

On Fri, Aug 11, 2017 at 10:18 AM, Jef Driesen <jef at libdivecomputer.org>
wrote:

> 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 :-)


Good idea.

-- 
John Van Ostrand
At large on sabbatical
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20170811/e9f19a25/attachment.html>