<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">2014-10-27 4:16 GMT+01:00 Dirk Hohndel <span dir="ltr"><<a href="mailto:dirk@hohndel.org" target="_blank">dirk@hohndel.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span><br>
> Datatrak/Wlog files include a lot of diving data which are not directly<br>
> supported in Subsurface, in these cases we choose mostly to use "tags".<br>
<br>
</span>Why is this (and the following comments) in "we" form? Did you collaborate<br>
with someone on these patches?<br></blockquote><div><br></div><div>I didn't, but, when writing english I tend to use "we" to avoid the constat repetition I,I,I,me,me,me ... <br></div><div>I know, I know, I'm too shy ;-) We (spanish) don't have that problem as our verb forms includes<br></div><div>the person in most situations.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
</span><span><br>
> BTW, in Example.log, 0x00 identifier is used for some DC dives and from my own<br>
> divelogs is inferred that 0x00 is used for manually entered dives, this could<br>
> easily be an error in Example.log coming from a preproduction DC model.<br>
<br>
</span>Have you found others who have old divelogs from this software?<div></div></blockquote><div><br></div><div>No. The divers I know who are still using Aladin (Air X/Z, Pro, ...) nor even download the <br></div><div>info from their DCs (and probably never did). Said this, I still see a number of those devices in use,<br></div><div>but don't know all those guys. My son (16 y.o. and recently certified OWD) has just inherited his<br>uncle's Aladin Pro; now he only needs his father to build an interface to download the data (which<br></div><div>can be very, very, long time as I'm a real moron with the soldering iron).<br></div><div><br>
> +<br>
> +<br>
> +static int two_bytes_to_int(unsigned char x, unsigned char y) {<br>
> +<br>
> + return (x << 8) + y;<br>
> +}<br>
<br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Take a look at the coding style. The opening '{' on a function should be<br>
in column 1 of the folowing line. This seems to be wrong everywhere.<br>
<br></blockquote><div><br></div><div>Unforgivable. Never did like this before. When I noticed, I thought it was because of the<br></div><div>settings in QT-Creator taken from the CodingStyle file and didn't paid more attention.<br></div><div>Corrected in the whole file.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Also, this function will get you in trouble on systems where int is only<br>
16 bits (admittedly, not a major concern these days, but on principle...<br>
why is the result signed?)<br>
<span><br>
> +static long four_bytes_to_long(unsigned char x, unsigned char y, unsigned char z, unsigned char t) {<br>
> +<br>
> + return (((long)x << 24) + ((long)y << 16) + ((long)z << 8) + ((long)t));<br>
> +}<br>
<br>
</span>Same comments again. If long is 32 bit you are in trouble. Can this be<br>
unsigned, please?<br>
<span><br>
> +static unsigned char* byte_to_bits(unsigned char byte) {<br>
> +<br>
> + unsigned char i, *bits = (unsigned char*) malloc(8 * sizeof(unsigned char));<br>
<br>
</span>sizeof(unsigned char) is always 1. Well, technically that's only<br>
guaranteed in C++, I guess. But I'm not aware of any C compiler today<br>
(certainly not on Windows, Mac, Linux) that does anything else.<br>
<span><br></span></blockquote><div>Changed to unsigned both of them. You are right, off course.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
> +<br>
> + for (i = 0; i < 8; i++) {<br>
> + bits[i] = (byte & (unsigned char) pow(2,i))/pow(2,i);<br>
<br>
</span>Yikes... this can be done much easier with shifts...<br></blockquote><div><br></div><div>Sure. Also, bits[i] don't needs to be 0 or 1, just != 0 so changed to<br> bits[i] = (byte & (1 << i));<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
<br>
> +static time_t date_time_to_ssrfc(long date, int time) {<br>
> +<br>
> + time_t tmp;<br>
> + tmp = ((date -135140)*86400)+(time*60);<br>
> + return tmp;<br>
> +}<br>
<br>
</span>Fascinating formula - seriously, this is in days since Jan 1st, 1600?<br>
This really deserves a comment, don't you think?<br>
<div><div><br></div></div></blockquote><div><br></div><div>Seriously. It don't seems to come from aladin data, probably is a datatrak "feature".<br></div><div>May be those guys were remembering Giordano Bruno burnt to death in the pyre of<br></div><div>the Inquisition. Don't know.<br></div><div>Commented on the code.<br><br><br>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">This also deserves some comments<br>
<span>> +<br>
> +/*<br>
> + * Shameless copy paste from dive.c add_sample() with ligth changes<br>
<br>
</span>ligth?<br>
<span><br></span></blockquote><div>I meant "slight". add_sample() seems excessively complex for its job. I thik we don't<br></div><div>really need to pass the pointer to the predefined sample structure, just to catch the<br></div><div>returned one. This said add_sample() is working fine from long, so I've moved the <br></div><div>code to it, removing add_dt_sample().<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
> + */<br>
> +static struct sample *add_dt_sample(int time, struct divecomputer *dc)<br>
> +{<br>
> + struct sample *p = prepare_sample(dc);<br>
> +<br>
> + if (p) {<br>
> + p->time.seconds = time;<br>
> + finish_sample(dc);<br>
> + }<br>
> + return p;<br>
> +}<br></span></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
> + if (two_bytes_to_int (lector_bytes[0],lector_bytes[1]) != 0xA000) {<br>
<br>
</span>more whitespace issues.....^^^<br>
<span></span> </blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
> + printf("ERROR, Byte = %4x\n", two_bytes_to_int (lector_bytes[0],lector_bytes[1]));<br>
<br>
</span>and more of the same.........................................^^^^<br>
<span><br></span></blockquote><div>Hmmmm, this kind issues are difficult to completely avoid. Re-readed the full code and found some<br></div><div>more, all fixed, but may be there are more here and there.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
> + switch (tmp_1byte) {<br>
> + case 1:<br>
> + dt_dive->dc.surface_pressure.mbar = 1013;<br>
> + break;<br>
> + case 2:<br>
> + dt_dive->dc.surface_pressure.mbar = 928;<br>
> + break;<br>
> + case 3:<br>
> + dt_dive->dc.surface_pressure.mbar = 806;<br>
> + break;<br>
> + case 4:<br>
> + dt_dive->dc.surface_pressure.mbar = 684;<br>
> + break;<br>
<br>
</span>It would be nice to have the altitude ranges in comments.<br>
735m, 1888m, 3194m seem very odd thresholds. Or maybe you used a different<br>
conversion formula?<br>
<span><br></span></blockquote><div>Those values were a bold approximation to be recalculated, just forgot to do it. I 've changed them<br>to more accurate values. Included the ranges and the formula in the code comments.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>
> + dt_dive->suit = strdup(QT_TRANSLATE_NOOP("gettextFromC","No suit"));<br>
<br>
</span>Again, whitespace... you want a space between comma and double quote..........^^^<br></blockquote><div><br></div><div>There were a lot of these (copy/paste). Cleaned all of them (I think).<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<span><br>
> + if (tmp_2bytes != 0x7fff) {<br>
> + dt_dive->watertemp.mkelvin = dt_dive->dc.watertemp.mkelvin = C_to_mkelvin((double)(tmp_2bytes/100));<br>
> + } else {<br>
> + dt_dive->watertemp.mkelvin = 0;<br>
> + }<br>
<br>
</span>You don't really need the curly braces here...<br></blockquote><div><br></div><div>No, really not. Just a personal taste, but its aginst the CodingStyle. Removed all I've seen.<br> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Not a lot of issues, but enough for me to ask you to resubmit.<br>
<span><font color="#888888"><br>
/D<br>
</font></span></blockquote></div><br></div><div class="gmail_extra">Attached is the improved patch. It merges against git- 4fa3f89 and seems to break nothing, although<br></div><div class="gmail_extra">really adds some strings to translate.<br></div><div class="gmail_extra"><br></div><div class="gmail_extra">Regards.<br><br></div><div class="gmail_extra">Salva.<br></div></div>