ubuntu builds

[Miika Turkia]

Dirk, can you kick the 4.8.1 builds for Ubuntu? We are still on 4.8.0 on 
launchpad.

miika
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Garmin support: ignore FIT files that aren't dives

[Linus Torvalds]

On Tue, Sep 4, 2018 at 6:55 PM Dirk Hohndel  wrote:
>
> I didn't find any mention that the indices are ordered. Feel free to
> ignore the attached patch - but it seems a very small price to ensure
> we don't use the data for a different device index by mistake.

So that patch looks correct.

As to the indexes being ordered, the whole "device_index" field isn't
even guaranteed to exist at all, according to the docs I have.

But when they talk about "Common fields" (4.7 in the FIT protocol
specs) they do make it clear that both message index and part index
has to start at 0 and increment.

But device_index isn't actually mentioned there at all.

In the XLS file, it says that device_index 0 is the "creator of the
file", but don't explain what that even means.

So I'm a bit leery of the whole device_index simply because it doesn't
seem to make a lot of sense.

Basically I'm not sure that "device_index=0" has any more meaning than
"first DeviceInfo you see" does.

I'd much prefer to just use the FILE message, which doesn't have this
ambiguity at all, but which also doesn't have that firmware field at
all.

Maybe somebody else has better FIT/Garmin docs.

   Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Garmin support: ignore FIT files that aren't dives

[Dirk Hohndel]

> On Sep 4, 2018, at 6:47 PM, Linus Torvalds  
> wrote:
> 
> On Tue, Sep 4, 2018 at 6:32 PM Dirk Hohndel  wrote:
>>> On Sep 4, 2018, at 6:11 PM, Linus Torvalds  
>>> wrote:
>>> 
>>> I took all your commits. Minor changes, like not checking that
>>> device_index, because it actually changes *after* you've already seen
>>> the values.
>> 
>> As you have seen, there are several devices inside your Garmin.
> 
> Yes, yes. But because you checked device_index, you actually got the
> *wrong* data.

I understand that - see my explanation that the fact that index 0 and 1
ended up having the same data... which is why I didn't notice this.

>> Clearly we need to use the device_info message that is for the correct 
>> device_index.
> 
> No, we probably just need to use the *first* one.
> 
> The fields inside the record aren't ordered (well, they are, but the
> ordering is an insane one based on the size of the field).
> 
> But the various messages do end up being ordered. In fact, the FIT
> specification even enforces some of the index ordering, although it's
> not spelled out for the device_index one. But other index cases (the
> generic message index and part index) are literally specified that
> they have to start with zero and increment sequentially.
> 
> So the assumption that device_index 0 is the first one is actually a
> fairly safe one just going by some of the other indexing FIT files do,
> unlike the assumption that device_index comes before the other fields
> (which is not only not safe, it's not actually the case).

I didn't find any mention that the indices are ordered. Feel free to
ignore the attached patch - but it seems a very small price to ensure
we don't use the data for a different device index by mistake.

/D

From 0861335bb7ddac5d28204b90e7bf166906a49de2 Mon Sep 17 00:00:00 2001
From: Dirk Hohndel 
Date: Tue, 4 Sep 2018 18:50:06 -0700
Subject: [PATCH] Garmin: don't assume that the first device index is 0

While it seems like a safe assumption, the cost of being careful and assembling
the full record and taking the one for device_index 0 seems worth it to me.

Signed-off-by: Dirk Hohndel 
---
 src/garmin_parser.c | 36 +++-
 1 file changed, 27 insertions(+), 9 deletions(-)

diff --git a/src/garmin_parser.c b/src/garmin_parser.c
index 51ba677..c33400f 100644
--- a/src/garmin_parser.c
+++ b/src/garmin_parser.c
@@ -70,11 +70,15 @@ struct record_data {
// RECORD_EVENT
unsigned char event_type, event_nr, event_group;
unsigned int event_data, event_unknown;
+
+   // RECORD_DEVICE_INFO
+   unsigned int device_index, firmware, serial, product;
 };
 
-#define RECORD_GASMIX 1
-#define RECORD_DECO   2
-#define RECORD_EVENT  4
+#define RECORD_GASMIX  1
+#define RECORD_DECO2
+#define RECORD_EVENT   4
+#define RECORD_DEVICE_INFO 8
 
 typedef struct garmin_parser_t {
dc_parser_t base;
@@ -91,7 +95,7 @@ typedef struct garmin_parser_t {
struct {
unsigned int initialized;
unsigned int sub_sport;
-   unsigned int serial_nr;
+   unsigned int serial;
unsigned int product;
unsigned int firmware;
unsigned int protocol;
@@ -218,6 +222,11 @@ static void flush_pending_record(struct garmin_parser_t 
*garmin)
garmin->cache.initialized |= 1 << DC_FIELD_GASMIX_COUNT;
garmin->cache.initialized |= 1 << DC_FIELD_TANK_COUNT;
}
+   if (pending & RECORD_DEVICE_INFO && record->device_index == 0) {
+   garmin->cache.firmware = record->firmware;
+   garmin->cache.serial = record->serial;
+   garmin->cache.product = record->product;
+   }
return;
}
 
@@ -493,18 +502,26 @@ DECLARE_FIELD(DEVICE_SETTINGS, utc_offset, UINT32) { 
garmin->cache.utc_offset =
 DECLARE_FIELD(DEVICE_SETTINGS, time_offset, UINT32) { 
garmin->cache.time_offset = (SINT32) data; } // wrong type in FIT
 
 // DEVICE_INFO
-// We just pick the first data we see
+// collect the data and then use the record if it is for device_index 0
+DECLARE_FIELD(DEVICE_INFO, device_index, UINT8)
+{
+   garmin->record_data.device_index = data;
+   garmin->record_data.pending |= RECORD_DEVICE_INFO;
+}
 DECLARE_FIELD(DEVICE_INFO, product, UINT16)
 {
-   if (!garmin->cache.product) garmin->cache.product = data;
+   garmin->record_data.product = data;
+   garmin->record_data.pending |= RECORD_DEVICE_INFO;
 }
 DECLARE_FIELD(DEVICE_INFO, serial_nr, UINT32Z)
 {
-   if (!garmin->cache.serial_nr) garmin->cache.serial_nr = data;
+   garmin->record_data.serial = data;
+   garmin->record_data.pending |= RECORD_DEVICE_INFO;
 }
 DECLARE_FIELD(DEVICE_INFO, firmware, UINT16)
 {
-   if (!garmin->cache.firmware) garmin->cache.firmware = data;
+   garmin->record_data.firmware = data;
+   

Re: Garmin support: ignore FIT files that aren't dives

[Linus Torvalds]

On Tue, Sep 4, 2018 at 6:32 PM Dirk Hohndel  wrote:
> > On Sep 4, 2018, at 6:11 PM, Linus Torvalds  
> > wrote:
> >
> > I took all your commits. Minor changes, like not checking that
> > device_index, because it actually changes *after* you've already seen
> > the values.
>
> As you have seen, there are several devices inside your Garmin.

Yes, yes. But because you checked device_index, you actually got the
*wrong* data.

Because you first got the firmware for device index 0, and took it,
but then you gfot the firmware field for device index 1, and you took
that *too*, because the device index=1 hadn't actually shown up yet.

> Clearly we need to use the device_info message that is for the correct 
> device_index.

No, we probably just need to use the *first* one.

The fields inside the record aren't ordered (well, they are, but the
ordering is an insane one based on the size of the field).

But the various messages do end up being ordered. In fact, the FIT
specification even enforces some of the index ordering, although it's
not spelled out for the device_index one. But other index cases (the
generic message index and part index) are literally specified that
they have to start with zero and increment sequentially.

So the assumption that device_index 0 is the first one is actually a
fairly safe one just going by some of the other indexing FIT files do,
unlike the assumption that device_index comes before the other fields
(which is not only not safe, it's not actually the case).

 Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Garmin support: ignore FIT files that aren't dives

[Dirk Hohndel]

> On Sep 4, 2018, at 6:11 PM, Linus Torvalds  
> wrote:
> 
> On Tue, Sep 4, 2018 at 4:38 PM Linus Torvalds
>  wrote:
>> 
>>> - in the FIT file, the Descent Mk1 is identified as product 2859; I think 
>>> this would
>>> make sense to use as model number (instead of 0)
>> 
>> I'll take a look.
> 
> I took all your commits. Minor changes, like not checking that
> device_index, because it actually changes *after* you've already seen
> the values.

As you have seen, there are several devices inside your Garmin. 
device index 0 is the whole device, it has no device type
device index 1 is the 
in my case it happens to have the information as index 0, which is why my code
happened to work
device index 2 is the , no serial, no fw 
version
device index 3 is of device type 8 and has again no serial, but fw version 30.45

etc

Clearly we need to use the device_info message that is for the correct 
device_index.
So no, I don't think we can ignore this.

> The garmin field numbering is confusing. For example, device_index is
> indeed field #0, but it's not the first field in the record stream.
> It's one of the last fields because it's a small one-byte field.
> 
> So when the FIT file stream is parsed, you will see things in this order
> 
> first DEVICE_INFO message:
> - firmware field
> - device index: 0
> 
> second DEVICE_INFO message
> - firmware field
> - device index: 1
> 
> so when you checked "is device index 0", that check triggered for both
> of those firmware fields, because the device index updated to 1 only
> _after_ you'd seen the second firmware field.
> 
> I could have done it the "proper" way with actually batching it up
> using the record_data thing, but it seemed unnecessary, and I just did
> that "only overwrite firmware when the old number was zero".
> 
> Which isn't necessarily the technically proper way to do it, but it
> gets the right result, and we get the firmware field details from the
> first/main DEVICE_INFO message.

But that makes the assumption that the first DEVICE_INFO message that
you get is the one for device index 0. Now that I understand better how this
works, I'm not sure I like that assumption all that much, either. We really
SHOULD do this in the record_data function.

> Of course, the serial number and the product data we could have just
> gotten from the FILE message, which only happens once. But that one
> doesn't contain the firmware information, and yeah, the firmware field
> has different values for the different device_index values. I don't
> know why, but at least now it takes that first one.

I'll play with this some more, but I may send you a follow up commit
to make sure we really grab the info from device index 0.

Thanks for merging - I will update the submodule now because at least
with my FIT files the current code happens to work :-)

/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Garmin support: ignore FIT files that aren't dives

[Linus Torvalds]

On Tue, Sep 4, 2018 at 4:38 PM Linus Torvalds
 wrote:
>
> > - in the FIT file, the Descent Mk1 is identified as product 2859; I think 
> > this would
> > make sense to use as model number (instead of 0)
>
> I'll take a look.

I took all your commits. Minor changes, like not checking that
device_index, because it actually changes *after* you've already seen
the values.

The garmin field numbering is confusing. For example, device_index is
indeed field #0, but it's not the first field in the record stream.
It's one of the last fields because it's a small one-byte field.

So when the FIT file stream is parsed, you will see things in this order

first DEVICE_INFO message:
 - firmware field
 - device index: 0

second DEVICE_INFO message
 - firmware field
 - device index: 1

so when you checked "is device index 0", that check triggered for both
of those firmware fields, because the device index updated to 1 only
_after_ you'd seen the second firmware field.

I could have done it the "proper" way with actually batching it up
using the record_data thing, but it seemed unnecessary, and I just did
that "only overwrite firmware when the old number was zero".

Which isn't necessarily the technically proper way to do it, but it
gets the right result, and we get the firmware field details from the
first/main DEVICE_INFO message.

Of course, the serial number and the product data we could have just
gotten from the FILE message, which only happens once. But that one
doesn't contain the firmware information, and yeah, the firmware field
has different values for the different device_index values. I don't
know why, but at least now it takes that first one.

It all seems to work for me still.

   Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Garmin support: ignore FIT files that aren't dives

[Linus Torvalds]

On Tue, Sep 4, 2018 at 3:00 PM Dirk Hohndel  wrote:
>
> - in order to do that, my code makes the assumption that in the device_info
> message the device_index will always be included before the other elements

I really don't like that assumption, and it's wrong anyway. The device
index comes after the serial number in the stream, because the garmin
field packing seems to be big-to-small, probably due to structure
packing reasons.

And even if true, it just seems unnecessary. You could just check
whether the old data was zero or not, and not replace an already
filled-in field.

IOW, for the firmare fill, just do

DECLARE_FIELD(DEVICE_INFO, firmware, UINT16)
{
if (!garmin->cache.firmware)
garmin->cache.firmware = data;
}

and never mind the device_index at all - you'll just pick up the first
firmware version you see.

Oh, and no need to cast the 'data' to the right type. It will already
*be* of that type, the macros that create the field definition
functions will do that for you.

> Is there a hook that get's called when a message is completed, so
> when all fields are read?

Yes, this is what the

struct record_data record_data;

is for: you can fill in fields in there, set the "pending" bit that
the collection of fields has been filled in, and then
flush_pending_record() will be called at the end of the full message.

This is needed for things like "gasmix" that aren't just a single
value, but a collection of values. And you *could* use it for this
too: first marshall the data into the record_data, and then in the
flush_pending_record() you can now look at the group of data together
(ie device_index and firmware fields)

However, for your case, I think the easier thing to do is to just do
that "just pick the first firmware version" and be done with it.

> - in the FIT file, the Descent Mk1 is identified as product 2859; I think 
> this would
> make sense to use as model number (instead of 0)

I'll take a look.

  Linus
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Re: Garmin support: ignore FIT files that aren't dives

[Dirk Hohndel]

> On Sep 4, 2018, at 7:38 AM, Dirk Hohndel  wrote:
>> Patch looks fine to me. Not optimal, but not wrong.
>> 
>> Note that doing this right, you should also be able to do do the
>> DC_EVENT_DEVINFO event from the downloader (just once), since you'd
>> have the firmware/serial info now.

Here are a couple more commits, let me know what you think.
Things to note:

- the FIT file format has a TON of redundant information. I /think/ I found
the most relevant message to get us serial/firmware/product
- in order to do that, my code makes the assumption that in the device_info
message the device_index will always be included before the other elements;
this has been the case in all records that I've seen, but I'm not sure that this
is guaranteed. So that's a potential concern with the "reading this as a stream"
approach. Is there a hook that get's called when a message is completed, so
when all fields are read? That would be the safer way to make sure we pick
the correct device_info.
- in the FIT file, the Descent Mk1 is identified as product 2859; I think this 
would
make sense to use as model number (instead of 0)

Comments? Thoughts?

I'm doing this on a Mac and this one isn't set up to send git email. And as we
saw yesterday attaching patches is suboptimal as well... So instead I pushed 
this to a git repo: 
https://github.com/dirkhh/libdivecomputer-for-linus/commits/Subsurface-NG

/D
___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface

Fwd: Re: Opening dive log in android version

[Willem Ferguson]

On 04/09/2018 07:45, Markley LeLeigh wrote:
Appreciate the reasoning, but still seems like a very basic missing 
functionality. Not everyone wants their un-encrypted data in the 
cloud. The play store description you quoted pretty clearly states 
that it will work without cloud (which isn't true unless you ONLY want 
your precious log on that one single [easily lost/broken/damaged] 
device). Even 8.1 of the manual doesn't make it clear that that basic 
function is missing but really should do.

Markley, what would you like in the user manual? I do not quite get your
point. Please explain.

If you store your data as a git repository in a local directory, you
will see it is much smaller than the XML text version. This is because
git compresses the data, as it presumably does in the cloud. In
addition, the compressed data can only be accessed using git because of
the fragmented nature of the sequential updates to the dive log. I
cannot see that the git repository in the cloud could be accessible with
any normal text viewer. Try and access a private git repository with a
text editor: you will see what I mean. I backup my dive log to both
Subsurface cloud as well as Dropbox. The dropbox action for me is a
simple drag-and-drop of the file. No problem.

If you are doing military dives or dives for a company then you should
not be putting your dives in ANY public location anyway but back it up
offline.

Kind regards,
willem


--
This message and attachments are subject to a disclaimer.

Please refer to 
http://upnet.up.ac.za/services/it/documentation/docs/004167.pdf 
 for
full 
details.

___
subsurface mailing list
subsurface@subsurface-divelog.org
http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface