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

[John Van Ostrand]

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

> On 2017-07-15 22:39, John Van Ostrand wrote:
>
>>  static dc_status_t
>> -cochran_commander_read_config (cochran_commander_device_t *device,
>> dc_event_progress_t *progress, unsigned char data[], unsigned int
>> size)
>> +cochran_commander_read_config (cochran_commander_device_t *device,
>> dc_event_progress_t *progress, unsigned char data[])
>>
>
> Removing the size parameter is a bad idea. Any function that takes some
> buffer as argument should also have a corresponding size parameter. Even if
> it's only used to verify whether the caller passed the right amount:
>
> if (size % 512 != 0)
>    return some_error;
>
> If you simply trust the caller to always pass a buffer of the right size,
> you'll run into trouble someday!


Read on.


>
>
>         dc_device_t *abstract = (dc_device_t *) device;
>>         dc_status_t rc = DC_STATUS_SUCCESS;
>>
>> +       unsigned int pages = 2;
>> +
>> +       if (device->layout->model == COCHRAN_MODEL_COMMANDER_TM)
>> +               pages = 1;
>> +
>>         // Read two 512 byte blocks into one 1024 byte buffer
>> -       for (unsigned int i = 0; i < 2; i++) {
>> -               const unsigned int len = size / 2;
>> +       for (unsigned int i = 0; i < pages; i++) {
>>
>>                 unsigned char command[2] = {0x96, i};
>> -               rc = cochran_commander_packet(device, progress, command,
>> sizeof(command), data + i * len, len, 0);
>> +               unsigned int command_size = 2;
>> +               if (device->layout->model == COCHRAN_MODEL_COMMANDER_TM)
>> +                       command_size = 1;
>> +
>> +               rc = cochran_commander_packet(device, progress, command,
>> command_size, data + i * 512, 512, 0);
>>                 if (rc != DC_STATUS_SUCCESS)
>>                         return rc;
>>
>
> How about using the size argument to calculate the number of pages? That
> way the function becomes even more generic. Only the caller needs to know
> how many pages there are. The function just needs to handle the difference
> between the 1 or 2 byte command variant (and even that can be based on the
> number of pages).
>
> This was the resolution I chose.


> +               if (device->layout->baudrate == 9600) {
>> +                       // Older commander, use low-speed read command
>> +                       command[0] = 0x05;
>> +                       command[1] = (address      ) & 0xff;
>> +                       command[2] = (address >>  8) & 0xff;
>> +                       command[3] = (address >> 16) & 0xff;
>> +                       command[4] = (size         ) & 0xff;
>> +                       command[5] = (size >> 8    ) & 0xff;
>> +                       command_size = 6;
>> +               } else {
>> +                       // Newer commander with high-speed read command
>> +                       command[0] = 0x15;
>> +                       command[1] = (address      ) & 0xff;
>> +                       command[2] = (address >>  8) & 0xff;
>> +                       command[3] = (address >> 16) & 0xff;
>> +                       command[4] = (size         ) & 0xff;
>> +                       command[5] = (size >>  8   ) & 0xff;
>> +                       command[6] = (size >> 16   ) & 0xff;
>> +                       command[7] = 0x04;
>> +                       command_size = 8;
>> +               }
>>
>
> 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.


>
>
> -       // 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.

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