search

jimmykane / fit-parser

Parse your FIT files easily, directly from JS (Garmin, Polar, Suunto)
Other
89 stars 33 forks source link

Fix support both summary first and last #33

Closed gie3d closed 10 months ago

gie3d commented 10 months ago

Found a bug report from @krzysztof-cislo.

This patch aims to fixed

I also produce a JSON outputs.

Archive.zip

krzysztof-cislo commented 10 months ago

@gie3d , Why did you add parsed output produced by quite old 1.8.4? The latest version before your changes is 1.9.3

Can you also add one more file to compare? Output of parsed summary last example generated by 1.9.3 version? This could be a reference file to compare as in 1.9.3 summary last fit file is parsed correctly. Then we can compare this PR generated versions (first and last) with 1.9.3 version.

I noticed also one difference between output_summary_last.json and output_summary_first.json. The structure if JSON is indeed the same but all timestamp values are different.

In the article: https://forums.garmin.com/developer/fit-sdk/b/news-announcements/posts/important-fit-activity-file-message-change it is written:

To properly decode FIT Activity Files created using either the “Summary First” or “Summary Last” method, the End Time for the summary messages should be calculate as: ​ end_time = start_time + total_elasped_time This technique will allow platforms to process FIT Activity files using either message ordering scheme. ​

I think this is missing in the current implementation.

What is more interesting... maybe it is the only think what was missing in the 1.9.3 version because I used this version to parse multisport file and it was parsed correctly but the start time of all activities was identical.

gie3d commented 10 months ago

@krzysztof-cislo

Why did you add parsed output produced by quite old 1.8.4? The latest version before your changes is 1.9.3

Can you also add one more file to compare? Output of parsed summary last example generated by 1.9.3 version? This could be a reference file to compare as in 1.9.3 summary last fit file is parsed correctly. Then we can compare this PR generated versions (first and last) with 1.9.3 version.

Ok, I got your point. The version in tag shows that the latest version before my changed was 1.9.3 (but I mentioned as 1.8.4). I can assure you that what I mean by 1.8.4 was the latest commit of the master branch before merging mine. (so in this case, it is 1.9.3) https://github.com/jimmykane/fit-parser/commit/e1a641fb81ec64cbc58c2535966a213eb29e041b

So the file in my Archive.zip, the triathlon_summary_last-1.8.4.json was created using 1.9.3. and the others were created using the version in this PR.

I noticed also one difference between output_summary_last.json and output_summary_first.json. The structure if JSON is indeed the same but all timestamp values are different.

Yes, it should and must be different. The timestamp was the time when the record was created. So in summary first, the summary sessions, laps were created before record, that's why the timestamp is different from summary last which sessions and laps were created after records. CleanShot 2023-11-22 at 18 08 26@2x But all start time must be identical.

And since there is no end_time in the fit file data. So it's up to the user to calculate by themselves. I think the article tries to convince that to get the correct calculation end_time, the start_time must be used, not timestamp

krzysztof-cislo commented 10 months ago

You're right! I mixed timestamp with start_date. So the calculation looks correct.

However, I found one problem with new implementation. Both timestamp and start_date values are now strings in the parsed objects. Previously it was Date object.

When parsed object is serialized it is not visible because the string values is the same as serialized Date object values.

Look at the screens:

This PR implementation (timestamp is string)

image

1.9.3 implementaion (timestamp is Date object)

image

Can you check this?

gie3d commented 10 months ago

@krzysztof-cislo Good point! Wonder why I didn't see it on the json output files. I should have time to check tonight (I'm UTC+7). Will update. Thanks!

gie3d commented 10 months ago

@krzysztof-cislo I think the reason the start_date and timestamp are string in my PR because of I used JSON.parse(JSON.stringify()) when I pass sessions and laps into the new function. So I updated the PR to use spread operator instead. https://github.com/jimmykane/fit-parser/commit/4ced9531cf4c4bb23703ee0515688ef868361590