<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Fri, Jun 2, 2017 at 5:18 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">John,<br>
<br>
Can you reformat your commit message to follow the standard git format? You can find a very nice description here:<br>
<br>
<a href="https://chris.beams.io/posts/git-commit/" rel="noreferrer" target="_blank">https://chris.beams.io/posts/g<wbr>it-commit/</a></blockquote><div><br></div><div>It's been a while since I used git and it became clear after doing a couple commits that I forgot how commit messages appear. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
I wouldn't mind a more detailed explanation either.<br>
<br>
Some more comments below.<span class=""><br>
<br>
On 2017-06-01 01:24, John Van Ostrand wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+typedef enum cochran_profile_size_t {<br>
+ PROFILE_FULL_SIZE,<br>
+ PROFILE_READ_SIZE,<br>
+} cochran_profile_size_t;<br>
</blockquote>
<br></span>
Just a matter of style, but I prefer to use a common prefix, like this:<br>
<br>
typedef enum cochran_profile_size_t {<br>
PROFILE_SIZE_FULL,<br>
PROFILE_SIZE_READ,<br>
} cochran_profile_size_t;</blockquote><div><br></div><div>Done and done. </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">
+static int<br>
+cochran_commander_profile_siz<wbr>e(cochran_commander_device_t *device,<br>
cochran_data_t *data, int dive_num, cochran_profile_size_t type)<br>
</blockquote>
<br></span>
Can this function ever return a negative value? Since it's a size, I guess the answer is no. In that case use an unsigned int. Makes it easier to read the code because you immediately know the value can never be negative!<br>
<br>
(Obviously the code where the return value is used needs to be adjusted as well.)</blockquote><div><br></div><div>Also done. </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">
+ const unsigned char *log_entry = data->logbook + dive_num *<br>
device->layout->rb_logbook_ent<wbr>ry_size;<br>
+ unsigned int sample_start_address = -1;<br>
</blockquote>
<br></span>
If you want to initialize the value to 0xFFFFFFFF, use that and not -1.</blockquote><div><br></div><div>Done. </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">
+ // Calculate the size of the profile only<br>
+ int sample_size = sample_end_address - sample_start_address;<br>
+<br>
+ if (sample_size < 0)<br>
+ // Adjust for ring buffer wrap-around<br>
+ sample_size += device->layout->rb_profile_end -<br>
device->layout->rb_profile_beg<wbr>in;<br>
+<br>
+ return sample_size;<br>
</blockquote>
<br></span>
You can easily avoid the signed integer here. Even better is to use the ringbuffer_distance function:<br>
<br>
ringbuffer_distance(sample_sta<wbr>rt_address, sample_end_address, 0, device->layout->rb_profile_beg<wbr>in, device->layout->rb_profile_end<wbr>)<br>
<br>
One of the advantages is that it contains asserts, which forces you to validate the pointers before using them. That's a good thing!<span class=""></span></blockquote><div><br></div><div>Cool. I'll look for other places to use ringbuffer functions. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
+/*<br>
+ * Do several things. Find the log that matches the fingerprint,<br>
+ * calculate the total read size for progress indicator,<br>
+ * Determine the most recent dive without profile data.<br>
+ */<br>
+<br>
+static int<br>
cochran_commander_find_finger<wbr>print(cochran_commander_device<wbr>_t<br>
*device, cochran_data_t *data)<br>
</blockquote>
<br></span>
Same comment about signed/unsigned return value here.</blockquote><div><br></div><div>Done.</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">
+ // Determine if sample exists<br>
+ if (profile_capacity_remaining > 0) {<br>
+ // Subtract this dive's profile size including post-dive events<br>
+ profile_capacity_remaining -= sample_size;<br>
+ if (profile_capacity_remaining < 0) {<br>
+ // Save the last dive that is missing profile data<br>
+ data->invalid_profile_dive_<wbr>num = i;<br>
+ }<br>
+ // Accumulate read size for progress bar<br>
+ sample_read_size += read_size;<br>
+ }<br>
+<br>
+ if (profile_capacity_remaining < 0) {<br>
+ // There is no profile for this dive<br>
+ sample_size = 0;<br>
+ }<br>
</blockquote>
<br></span>
This if statement can be removed. It doesn't have any effect. You set a local variable, which goes out of scope immediately after.</blockquote><div><br></div><div>Done. </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">
@@ -772,9 +791,64 @@ cochran_commander_device_forea<wbr>ch (dc_device_t<br>
*abstract, dc_dive_callback_t call<br>
cochran_data_t data;<br>
data.logbook = NULL;<br>
data.sample = NULL;<br>
</blockquote>
<br></span>
The data.sample field is no longer used for anything. Remove it!</blockquote><div><br></div><div>Good catch. </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">
+ // Allocate space for log book.<br>
+ data.logbook = (unsigned char *) malloc(data.logbook_size);<br>
+ if (data.logbook == NULL) {<br>
+ ERROR (abstract->context, "Failed to allocate memory.");<br>
+ return DC_STATUS_NOMEMORY;<br>
+ }<br>
+<br>
+ // Request log book<br>
+ rc = cochran_commander_read(device, &progress, 0, data.logbook,<br>
data.logbook_size);<br>
+ if (rc != DC_STATUS_SUCCESS)<br>
+ return rc;<br>
</blockquote>
<br></span>
There is a memory leak here, because you are not freeing data.logbook!</blockquote><div><br></div><div>Fixed. </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">
unsigned int dive_count = 0;<br>
if (data.dive_count < device->layout->rb_logbook_ent<wbr>ry_count)<br>
dive_count = data.dive_count;<br>
@@ -808,44 +879,11 @@ cochran_commander_device_forea<wbr>ch (dc_device_t<br>
*abstract, dc_dive_callback_t call<br>
unsigned int sample_start_address = array_uint32_le (log_entry +<br>
device->layout->pt_profile_beg<wbr>in);<br>
unsigned int sample_end_address = array_uint32_le (log_entry +<br>
device->layout->pt_profile_end<wbr>);<br>
<br>
- // Validate<br>
- if (sample_start_address < device->layout->rb_profile_beg<wbr>in ||<br>
- sample_start_address > device->layout->rb_profile_end ||<br>
- sample_end_address < device->layout->rb_profile_beg<wbr>in ||<br>
- (sample_end_address > device->layout->rb_profile_end &&<br>
- sample_end_address != 0xFFFFFFFF)) {<br>
- continue;<br>
- }<br>
</blockquote>
<br></span>
Why did you remove the validation? Now you are using data that is potentially bogus. That's a bad idea!</blockquote><div><br></div><div>The cochran_commander_profile_size() function returns sample_size of 0 if these values are bad. If the sample_size is 0 we don't use the data. Although I notice that in one case it will alter sample_end_address, so I added that to the foreach function too.</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">
+ do {<br>
+ rc = cochran_commander_read (device, &progress,<br>
sample_start_address, dive + device->layout->rb_logbook_ent<wbr>ry_size,<br>
sample_size);<br>
+ } while (rc != DC_STATUS_SUCCESS && tries++ < 3);<br>
</blockquote>
<br></span>
What's the reason for the retrying here? You don't do it elsewhere (for example when reading the logbook, or from the cochran_commander_device_{read<wbr>,dump} functions), so I wonder what's different here.</blockquote><div><br></div><div>Reading from the Cochran at the high baud rates isn't reliable. Very large reads (multiple MB) result in errors about 1/3rd of the time. This is reason why I decided to read each dive profile individually. Small reads are more reliable but when reading hundreds of times it's likely a read error will occur.</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">
+ if (rc != DC_STATUS_SUCCESS) {<br>
+ ERROR (abstract->context, "Failed to read the sample data.");<br>
+ return rc;<br>
</blockquote>
<br></span>
Again a memory leak here! There are two similar ones a few lines below.<span class="HOEnZb"><font color="#888888"></font></span></blockquote><div><br></div><div>Fixed. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888">
<br>
Jef<br>
</font></span></blockquote></div><div class="gmail_extra"><br></div><br clear="all"><div><br></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>