[PATCH] Import Datatrak/WLog files

[Salvador Cuñat]

2014-10-27 4:16 GMT+01:00 Dirk Hohndel <dirk at hohndel.org>:

>
> > Datatrak/Wlog files include a lot of diving data which are not directly
> > supported in Subsurface, in these cases we choose mostly to use "tags".
>
> Why is this (and the following comments) in "we" form? Did you collaborate
> with someone on these patches?
>

I didn't, but, when  writing english I tend to use "we" to avoid the
constat repetition I,I,I,me,me,me ...
I know, I know, I'm too shy  ;-)   We (spanish) don't have that problem as
our verb forms includes
the person in most situations.


> > BTW, in Example.log, 0x00 identifier is used for some DC dives and from
> my own
> > divelogs is inferred that 0x00 is used for manually entered dives, this
> could
> > easily be an error in Example.log coming from a preproduction DC model.
>
> Have you found others who have old divelogs from this software?
>

No.  The divers I know  who are still using Aladin  (Air X/Z, Pro, ...) nor
even download the
info from their DCs  (and probably never did).  Said this, I still see a
number of those devices in use,
but don't know  all those guys.  My son (16 y.o. and recently certified
OWD) has just inherited his
uncle's Aladin Pro;  now he only needs his father to build an interface to
download the data (which
can be very, very, long time as I'm a real moron with the soldering iron).

> +
> +
> +static int two_bytes_to_int(unsigned char x, unsigned char y) {
> +
> +     return (x << 8) + y;
> +}

Take a look at the coding style. The opening '{' on a function should be
> in column 1 of the folowing line. This seems to be wrong everywhere.
>
>
Unforgivable.  Never did like this before. When I noticed, I thought it was
because of the
settings in QT-Creator taken from the CodingStyle file and didn't paid more
attention.
Corrected in the whole file.

Also, this function will get you in trouble on systems where int is only
> 16 bits (admittedly, not a major concern these days, but on principle...
> why is the result signed?)
>
> > +static long four_bytes_to_long(unsigned char x, unsigned char y,
> unsigned char z, unsigned char t) {
> > +
> > +     return (((long)x << 24) + ((long)y << 16) + ((long)z << 8) +
> ((long)t));
> > +}
>
> Same comments again. If long is 32 bit you are in trouble. Can this be
> unsigned, please?
>
> > +static unsigned char* byte_to_bits(unsigned char byte) {
> > +
> > +     unsigned char i, *bits = (unsigned char*) malloc(8 *
> sizeof(unsigned char));
>
> sizeof(unsigned char) is always 1. Well, technically that's only
> guaranteed in C++, I guess. But I'm not aware of any C compiler today
> (certainly not on Windows, Mac, Linux) that does anything else.
>
> Changed to unsigned both of them. You are right, off course.


> > +
> > +     for (i = 0; i < 8; i++) {
> > +             bits[i] = (byte & (unsigned char) pow(2,i))/pow(2,i);
>
> Yikes... this can be done much easier with shifts...
>

Sure.  Also, bits[i] don't needs to be 0 or 1, just  != 0 so changed to
                            bits[i] = (byte & (1 << i));

>
> > +static time_t date_time_to_ssrfc(long date, int time) {
> > +
> > +     time_t tmp;
> > +     tmp = ((date -135140)*86400)+(time*60);
> > +     return tmp;
> > +}
>
> Fascinating formula - seriously, this is in days since Jan 1st, 1600?
> This really deserves a comment, don't you think?
>
>
Seriously.  It don't seems to come from aladin data, probably is a datatrak
"feature".
May be those guys were remembering Giordano Bruno burnt to death in the
pyre of
the Inquisition.  Don't know.
Commented on the code.


This also deserves some comments
> > +
> > +/*
> > + * Shameless copy paste from dive.c add_sample() with ligth changes
>
> ligth?
>
> I meant "slight".  add_sample() seems excessively complex for its job. I
thik we don't
really need to pass the pointer to the predefined sample structure, just to
catch the
returned one.  This said add_sample() is working fine from long, so I've
moved the
code to it, removing add_dt_sample().


> > + */
> > +static struct sample *add_dt_sample(int time, struct divecomputer *dc)
> > +{
> > +     struct sample *p = prepare_sample(dc);
> > +
> > +     if (p) {
> > +             p->time.seconds = time;
> > +             finish_sample(dc);
> > +     }
> > +     return p;
> > +}
>


> > +     if (two_bytes_to_int (lector_bytes[0],lector_bytes[1]) != 0xA000) {
>
> more whitespace issues.....^^^
>

> +             printf("ERROR, Byte = %4x\n", two_bytes_to_int
> (lector_bytes[0],lector_bytes[1]));
>
> and more of the same.........................................^^^^
>
> Hmmmm, this kind issues are difficult to completely avoid.   Re-readed the
full code and found some
more, all  fixed, but may be there are more here and there.

> +     switch (tmp_1byte) {
> > +             case 1:
> > +                     dt_dive->dc.surface_pressure.mbar = 1013;
> > +                     break;
> > +             case 2:
> > +                     dt_dive->dc.surface_pressure.mbar = 928;
> > +                     break;
> > +             case 3:
> > +                     dt_dive->dc.surface_pressure.mbar = 806;
> > +                     break;
> > +             case 4:
> > +                     dt_dive->dc.surface_pressure.mbar = 684;
> > +                     break;
>
> It would be nice to have the altitude ranges in comments.
> 735m, 1888m, 3194m seem very odd thresholds. Or maybe you used a different
> conversion formula?
>
> Those values were a bold approximation to be recalculated, just forgot to
do it.  I 've changed them
to more accurate values.  Included the ranges and the formula in the code
comments.


> > +                     dt_dive->suit =
> strdup(QT_TRANSLATE_NOOP("gettextFromC","No suit"));
>
> Again, whitespace... you want a space between comma and double
> quote..........^^^
>

There were a lot of these   (copy/paste).  Cleaned all of them  (I think).


> > +     if (tmp_2bytes != 0x7fff) {
> > +             dt_dive->watertemp.mkelvin = dt_dive->dc.watertemp.mkelvin
> = C_to_mkelvin((double)(tmp_2bytes/100));
> > +     } else {
> > +             dt_dive->watertemp.mkelvin = 0;
> > +     }
>
> You don't really need the curly braces here...
>

No, really not.  Just a personal taste, but its aginst the CodingStyle.
Removed all I've seen.


> Not a lot of issues, but enough for me to ask you to resubmit.
>
> /D
>

Attached is the improved patch.  It merges against git-  4fa3f89  and seems
to break nothing, although
really adds some strings to translate.

Regards.

Salva.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20141031/71ba6186/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Import-Datatrak_WLog-files.patch
Type: text/x-patch
Size: 50845 bytes
Desc: not available
URL: <http://lists.subsurface-divelog.org/pipermail/subsurface/attachments/20141031/71ba6186/attachment-0001.bin>