<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Thu, Aug 10, 2017 at 11:00 AM, Jef Driesen <span dir="ltr"><<a href="mailto:jef@libdivecomputer.org" target="_blank">jef@libdivecomputer.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 2017-07-15 22:39, John Van Ostrand wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
static dc_status_t<br>
-cochran_commander_read_config (cochran_commander_device_t *device,<br>
dc_event_progress_t *progress, unsigned char data[], unsigned int<br>
size)<br>
+cochran_commander_read_config (cochran_commander_device_t *device,<br>
dc_event_progress_t *progress, unsigned char data[])<br>
</blockquote>
<br></span>
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:<br>
<br>
if (size % 512 != 0)<br>
return some_error;<br>
<br>
If you simply trust the caller to always pass a buffer of the right size, you'll run into trouble someday!</blockquote><div><br></div><div>Read on.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
dc_device_t *abstract = (dc_device_t *) device;<br>
dc_status_t rc = DC_STATUS_SUCCESS;<br>
<br>
+ unsigned int pages = 2;<br>
+<br>
+ if (device->layout->model == COCHRAN_MODEL_COMMANDER_TM)<br>
+ pages = 1;<br>
+<br>
// Read two 512 byte blocks into one 1024 byte buffer<br>
- for (unsigned int i = 0; i < 2; i++) {<br>
- const unsigned int len = size / 2;<br>
+ for (unsigned int i = 0; i < pages; i++) {<br>
<br>
unsigned char command[2] = {0x96, i};<br>
- rc = cochran_commander_packet(devic<wbr>e, progress, command,<br>
sizeof(command), data + i * len, len, 0);<br>
+ unsigned int command_size = 2;<br>
+ if (device->layout->model == COCHRAN_MODEL_COMMANDER_TM)<br>
+ command_size = 1;<br>
+<br>
+ rc = cochran_commander_packet(devic<wbr>e, progress, command,<br>
command_size, data + i * 512, 512, 0);<br>
if (rc != DC_STATUS_SUCCESS)<br>
return rc;<br>
</blockquote>
<br></span>
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).<span class=""><br>
<br></span></blockquote><div>This was the resolution I chose.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+ if (device->layout->baudrate == 9600) {<br>
+ // Older commander, use low-speed read command<br>
+ command[0] = 0x05;<br>
+ command[1] = (address ) & 0xff;<br>
+ command[2] = (address >> 8) & 0xff;<br>
+ command[3] = (address >> 16) & 0xff;<br>
+ command[4] = (size ) & 0xff;<br>
+ command[5] = (size >> 8 ) & 0xff;<br>
+ command_size = 6;<br>
+ } else {<br>
+ // Newer commander with high-speed read command<br>
+ command[0] = 0x15;<br>
+ command[1] = (address ) & 0xff;<br>
+ command[2] = (address >> 8) & 0xff;<br>
+ command[3] = (address >> 16) & 0xff;<br>
+ command[4] = (size ) & 0xff;<br>
+ command[5] = (size >> 8 ) & 0xff;<br>
+ command[6] = (size >> 16 ) & 0xff;<br>
+ command[7] = 0x04;<br>
+ command_size = 8;<br>
+ }<br>
</blockquote>
<br></span>
So this means that the byte sequences:<br>
<br>
05 9D FF 00 43 00<br>
05 BD 7F 00 43 00<br>
<br>
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.</blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- // Read the sample data, from 0 to sample end will include logbook<br>
- rc = cochran_commander_read (device, &progress,<br>
device->layout->rb_logbook_beg<wbr>in, dc_buffer_get_data(buffer),<br>
device->layout->rb_profile_end<wbr>);<br>
+ // Read the sample data, logbook and sample data are contiguous<br>
+ rc = cochran_commander_read (device, &progress,<br>
device->layout->rb_logbook_beg<wbr>in, dc_buffer_get_data(buffer), size);<br>
if (rc != DC_STATUS_SUCCESS) {<br>
ERROR (abstract->context, "Failed to read the sample data.");<br>
return rc;<br>
</blockquote>
<br></span>
This can't work. The size variable is calculated as:<br>
<br>
layout->rb_profile_end - layout->rb_logbook_begin<br>
<br>
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.</blockquote><div><br></div><div>I thought so too but when I ask for ask for 0 bytes it returns 32K.</div><div> </div></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div>John Van Ostrand<br></div><div>At large on sabbatical<br></div><br></div></div>
</div></div>