-
Notifications
You must be signed in to change notification settings - Fork 227
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fit: Added support for ANT+ FIT format (used by Garmin devices) #863
Conversation
Nice! will have a look tomorrow, but some quick notes: Some tests and test files would be nice. Any idea if there are any good ones around under liberal license? alternatively is there some good way to produce own test files? Possible to include the scripts used to generate the tables, even if messy? can also have a README.md under the |
Yes, I will check the license on the SDK example files, and if they are not liberal enough I will throw in some of my own files. I'll also do some linting/cleanup and put my nodeJS script in the dev-folder, along with some markdown documentation in the fit-folder as suggested. |
👍
I usually try keep all format specific things in btw to add authorship, references, known limitations, help etc you can add a |
Output looks really good, impressive work 👍 hope i get more time to review tomorrow |
Should have been reviewing the code but ended up play around with it! try this:
|
Yup, that is the route for STHLM Bike 2023 =) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks overall good i think, will continue tomorrow
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments. Do you have something left you want to change or add before merge?
format/fit/fit.go
Outdated
|
||
func fitDecodeDefinitionMessage(d *decode.D, drc *dataRecordContext, lmfd localFieldDefMap, dmfd devFieldDefMap, isDevMap localMsgIsDevDef) { | ||
d.FieldU8("reserved") | ||
endian := d.FieldU8("architecture") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sym map this?
format/fit/fit.go
Outdated
fDefLookup, isSet := mappers.FieldDefMap[messageNo][fieldDefNo] | ||
if isSet { | ||
var foundName = fDefLookup.Name | ||
lmfd[drc.localMessageType][i] = mappers.FieldDef{Name: foundName, Type: typ, Size: size, Format: fDefLookup.Type, Unit: fDefLookup.Unit, Scale: fDefLookup.Scale, Offset: fDefLookup.Offset} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style nit: i usually prefer one field init per line in if not very smal structs, maybe there are some more like this?
fieldUint(d.FieldU16, 2, fDef, uintFormatter, &fdc) | ||
case "uint32", "uint32z": | ||
fieldUint(d.FieldU32, 4, fDef, uintFormatter, &fdc) | ||
case "uint64", "uint64z": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder what the deal with the *z variants, not allowed to be zero? i guess we could map description if invalid for the different variants... do if you feel like it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already handled by the uintFormatter in mappers/values.go.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I've figured out the *z variants. They are used as bitflags, meaning 0 is invalid because then it lacks any value. It also means another weekend project for me =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's already handled by the uintFormatter in mappers/values.go.
Aha yes forgot that
I think I've figured out the *z variants. They are used as bitflags, meaning 0 is invalid because then it lacks any value. It also means another weekend project for me =)
:) that makes sense, and the flags are defined somewhere?
But let me know if want to do it as separate future PR or so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There aren't that many of them, but they are defined in the Profile.xlsx.
None of their example files nor mine have any of those fields though, so I can't test it either way. I'm good for merging now if you are. Do you want me to squash rebase it first?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha i see, add if you happen to find files using them.
In this case i would probably squash into one or very few commits, but the commit history looks reasonable so i'm ok to merge as is also.
|
||
``` | ||
$ fq '[.dataRecords[] | select(.dataRecordHeader.messageType == 0).dataMessage]' file.fit | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we can come up with some more fancy usage examples 🤔 maybe add later
| | | data_message{}: 0x1f-0x28 (9) | ||
0x10| 00| .| manufacturer: 256 0x1f-0x21 (2) | ||
0x20|01 |. | | ||
0x20| 03 dc | .. | product: 56323 0x21-0x23 (2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this and product be mapped? i see a manufacturer or product map in types_generated.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Profile.xlsx (and thus types_generated.go) has no match for manufacturer 256. Product is a dynamic field (sometimes called a subfield) that has different lookup_tables depending on the value of another field, in this case manufacturer. Maybe I'll give this perticular nut another go at cracking this weekend. =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha i see, ok give it a go, no hurry to merge :)
format/fit/fit.go
Outdated
d.Fatalf("Unknown endian %d", endian) | ||
} | ||
messageNo := d.FieldU16("global_message_number", mappers.TypeDefMap["mesg_num"]) | ||
if messageNo == 206 { // developer field_description |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we could generate constants for these but i have a feeling very few of them would be used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make a manual constant for this "magic" number, and then have a gathered place for the if more are needed in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good
I have some stuff left that I know of, e.g (dynamic/subfields) and accumulated timestamp values from compressed messages. But the subfields are a bigger task, and I have no files with compressed message headers to test on. So I think they'll both have to wait for future versions. |
Added support for array values. Added documentation and tests.
Merge once CI is green? |
🥳 Thanks a lot and hope for more PRs in the future! btw i know a colleague of mine that is super excited about fit support, ping @KristianKarl |
I have already started on my next format: a nes rom decoder, with a to_asm function for decompiling to 6502 assembly code. =) |
Oh nice, it's addictive 😄 I have played around with the idea of having opcode decoders but got a bit stuck, didn't feel very neat, maybe could be useful? ex draft PR #215 |
0x40| 00 | . | local_message_type: 0 0x4d.4-0x4e (0.4) | ||
| | | data_message{}: 0x4e-0x50 (2) | ||
0x40| 00 64| .d| hrm_ant_id: 25600 0x4e-0x50 (2) | ||
0x50|39 50| |9P| | crc: 20537 (valid) 0x50-0x52 (2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nitpick: In some future PR maybe this can be presented in hex? add a scalar.UintHex
mapper. Maybe there are more fields more suitable in hex?
I've thought about generalising this for crc-ish fields somehow not sure how, would be nice to provide both calculated and expected value.
BTW i'm think of will do a new release later today or tomorrow, has been a while. Just do a PR if you want to include some smaller fixes. And if you happen to have some nice examples to show up the fit decoder drop them here and will include in the release notes! was thinking about including covert to KML maybe? |
Most awesome PR! Great job both! 👏 I already used fq and it's new fit decoder to create a summary of each of my 1300+ fit files in 1 json file. This file is super easy to query using jq to get my longest, fastest etc ride, or to get the total distance for a specific bike. Very handy, I'll do a write-up shortly how to do that, |
🥳 Nice! i've thought about splitting up https://github.com/wader/fq/blob/master/doc/formats.md somehow to have a page per format, maybe even use the github wiki? then write ups like that could be included? |
No description provided.