On Fri, Aug 11, 2017 at 10:18 AM, Jef Driesen <jef@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@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 :-)