> On Sep 4, 2018, at 6:47 PM, Linus Torvalds <[email protected]> > wrote: > > On Tue, Sep 4, 2018 at 6:32 PM Dirk Hohndel <[email protected]> wrote: >>> On Sep 4, 2018, at 6:11 PM, Linus Torvalds <[email protected]> >>> 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 <[email protected]> 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 <[email protected]> --- 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_DECO 2 +#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; + garmin->record_data.pending |= RECORD_DEVICE_INFO; } // SPORT @@ -694,6 +711,7 @@ DECLARE_MESG(EVENT) = { DECLARE_MESG(DEVICE_INFO) = { .maxfield = 6, .field = { + SET_FIELD(DEVICE_INFO, 0, device_index, UINT8), SET_FIELD(DEVICE_INFO, 3, serial_nr, UINT32Z), SET_FIELD(DEVICE_INFO, 4, product, UINT16), SET_FIELD(DEVICE_INFO, 5, firmware, UINT16), @@ -1124,7 +1142,7 @@ garmin_parser_is_dive (dc_parser_t *abstract, const unsigned char *data, unsigne if (devinfo_p) { devinfo_p->firmware = garmin->cache.firmware; - devinfo_p->serial = garmin->cache.serial_nr; + devinfo_p->serial = garmin->cache.serial; devinfo_p->model = garmin->cache.product; } return garmin->cache.sub_sport >= 53 && garmin->cache.sub_sport <= 57; -- 2.15.2 (Apple Git-101.1)
_______________________________________________ subsurface mailing list [email protected] http://lists.subsurface-divelog.org/cgi-bin/mailman/listinfo/subsurface
