Garmin support: ignore FIT files that aren't dives

[Dirk Hohndel]

Hey Linus,

Reading the Garmin parser is fun. In a trippy kinda way :-)

It bugged me that we kept offering all the other activities for download, so I
added a small commit that would make us look at the sub_sport before
offering the "dive" to Subsurface...

/D

[Dirk Hohndel]

Stupid Mac.

[Wojciech Więckowski]

Post by Dirk Hohndel
Stupid Mac.

That's why I called my script stupid. It parses log once but keeps some
data from earlier messages in cache buffers. When it reaches 'sport'
message which is always before 'record' (samples) parsing the rest of the
file may be continued or interrupted. Of course in Subsurface there is
another pass required for file picker.
BTW my watch just received FW 4.0.0 with a lot of changes in diving apps.

/WW

[Dirk Hohndel]

Post by Wojciech Więckowski
BTW my watch just received FW 4.0.0 with a lot of changes in diving apps.

I'm on Firmware 2.6.0...
but a quick Google finds this :-)

https://www8.garmin.com/support/download_details.jsp?id=14157 <https://www8.garmin.com/support/download_details.jsp?id=14157>

The changelog lists a few things that I disliked:

- Previously, the safety stop timer started counting down at 15 feet. This has been changed to 20 feet (6m).
- Improved dive log to exclude End Dive Delay in total time. This is now listed as 'Bottom Time'. This will not be recalculated for dives prior to this update.

Lots of other changes which sound at least reasonable.

Still no "Taekwondo" setting for workouts (or even generic martial arts... :-(

/D

[Linus Torvalds]

Post by Dirk Hohndel
https://www8.garmin.com/support/download_details.jsp?id=14157

Grr. Is there some way to use this without a Windows box?

Linus

[Dirk Hohndel]

Yes, works with a Mac :-)

Post by Linus Torvalds

Post by Dirk Hohndel
https://www8.garmin.com/support/download_details.jsp?id=14157

Grr. Is there some way to use this without a Windows box?
Linus

[Linus Torvalds]

Thanks for nothing.

Actually, it looks like it might be downloading the new firmware asi
connect it to the Android Garmin Connect app. We'll see..

Linus

Post by Dirk Hohndel
Yes, works with a Mac :-)
On September 3, 2018 8:34:49 PM PDT, Linus Torvalds <

Post by Linus Torvalds

Post by Dirk Hohndel
https://www8.garmin.com/support/download_details.jsp?id=14157

Grr. Is there some way to use this without a Windows box?
Linus

--
From my phone

[Linus Torvalds]

Post by Dirk Hohndel
It bugged me that we kept offering all the other activities for download, so I
added a small commit that would make us look at the sub_sport before
offering the "dive" to Subsurface...

Patch looks fine to me. Not optimal, but not wrong.

I've emailed Jef about the whole "parsing and downloading shouldn't be
so separate", but that's a long-term thing.

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.

Linus

[Dirk Hohndel]

Post by Linus Torvalds

Post by Dirk Hohndel
It bugged me that we kept offering all the other activities for download, so I
added a small commit that would make us look at the sub_sport before
offering the "dive" to Subsurface...

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.

Good idea. Ignore this patch. I'll send a better one that solves this at the
same time.

/D

[Dirk Hohndel]

Post by Linus Torvalds
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

[Linus Torvalds]

Post by Dirk Hohndel
- 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.

Post by Dirk Hohndel
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.

Post by Dirk Hohndel
- 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

[Linus Torvalds]

On Tue, Sep 4, 2018 at 4:38 PM Linus Torvalds

Post by Linus Torvalds

Post by Dirk Hohndel
- 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

[Dirk Hohndel]

Post by Linus Torvalds
On Tue, Sep 4, 2018 at 4:38 PM Linus Torvalds

Post by Linus Torvalds

Post by Dirk Hohndel
- 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 <I have no idea - device type 4>
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 <I still have no idea, device type 3>, 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.

Post by Linus Torvalds
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
- 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.

Post by Linus Torvalds
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

[Linus Torvalds]

Post by Dirk Hohndel

Post by Linus Torvalds
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.

Post by Dirk Hohndel
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

[Dirk Hohndel]

Post by Linus Torvalds

Post by Dirk Hohndel

Post by Linus Torvalds
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.

Post by Linus Torvalds

Post by Dirk Hohndel
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

[Linus Torvalds]

Post by Dirk Hohndel
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

[Linus Torvalds]

On Tue, Sep 4, 2018 at 6:47 PM Linus Torvalds

Post by Linus Torvalds
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).

Side note: I considered trying to make the field decoder give some
sane ordering. Because it would definitely help with a lot of cases.

It's essentially what the Garmin automated code generation does: it
seems to turn messages into a full structure, so that you don't have
that "individual fields" model of parsing that we do.

But it ends up being incredibly ugly and painful, and then you'd still
want to have some "individual fields might not have valid data" case,
so it seemed very annoying.

The "struct record_data" thing is kind of a "give me a whole struct",
but in general I'd really suggest trying to avoid it unless you really
have to.

And the reason is partly exactly the "individual fields might not have
valid data". If that ever happens, then the "struct record_data" would
simply have the previous value (or zero, for the first time of a
parse) for that field. It also needs to set the whole "I have pending
data bit" for each kind of structure you record, which adds some extr
a pain.

So that's mainly why I would suggest using the "one field at a time"
model when at all possible. It isn't always possible, but..

What I *really* fear is that some record ends up having the timestamp
not be the first field. Because if we ever end up seeing *that*, then
we're kind of screwed. Right now it really depends on "yeah, we always
see the timestamp first, so we can just do all the sample data as
individual fields without buffering them up".

Linus