• Replies 20 replies
  • Subscribers 5 subscribers
  • Views 1139 views
  • Users 0 members are here
Options
Related

Bug decoding component fields when expandComponents is true

markw65
1 month ago

Using the js sdk, given a file with records containing the field compressedSpeedDistance, if I try to decode it with expandComponents true (the default), it fails.

Decoder.#expandComponents will first create a speed field from compressedSpeedDistance. When it does so, it notices that speed should be expanded to enhancedSpeed, so it pushes speed onto this.#fieldsToExpand. Then it creates a distance field, and there's nothing more to do for that. Finally it tries to expand the speed field, and throws an error on this line, because speed doesn't exist in message (yet). It's been added to mesg, but that doesn't get appended to message until all component expansion has finished.

There's a simple fix - change that line to 

const { rawFieldValue, fieldDefinitionNumber, isSubField } = message[name] || mesg[name];

And with that change, the file is successfully decoded. But there's still a problem. expandComponents added both speed and enhancedSpeed, but enhancedSpeed is only one tenth the value of speed (speed was correct). And the problem now is that when compressedSpeedDistance was expanded, it created a speed field with the same raw data as the speed part of compressedSpeedDistance. But the scaling factor for speed in compressedSpeedDistance is 100, while the scaling factor for speed itself is 1000.

Again, the fix isn't too hard. Currently, it always copies the rawFieldValue and creates a fieldValue by scaling it according to the source field. Instead it should create the fieldValue the same way, then reverse-transform the fieldValue based on the target to create the target's rawFieldValue.

I was going to create an issue/pull request, but then noticed that this file is auto-generated (and that the repo isn't configured for issues). So I'm guessing there's a master file which is used to generate all the different language decoders, and that's the one that needs to be fixed. And indeed, reading the python version, it appears to have exactly the same two bugs.

  • 1 month ago in reply to seilogramp

    Thanks for the link. I had seen other people complaining about this, but not found any workarounds. For me, clearing the entire browser history sometimes works, and sometimes doesnt.

  • 1 month ago

    I will also note that accumulation is broken. I had assumed that I was responsible for doing that after calling decoder.read, but looking at the code, it looks like it is trying to accumulate the distance values, but it fails because there is no message definition that mentions the distance field  - only one for the compressedSpeedDistance field.

    In fact, I think there is an unrelated bug with the accumulator code.

    Every time a record is read which contains an accumulated field, a new accumulator is created for it, overwriting the old one, and defeating the purpose of accumulation.

    In addition, when those fields are read directly, the accumulator isnt used. Its only used when a field is on the fieldsToExpand list. This probably doesnt matter, because all those fields are 32 bit values, and arent likely to overflow (has anyone done a 40000 km bike ride recently).

    But when a field - such as compressedSpeedDistance - is expanded to speed and distance, it does call the accumulator for distance. And it needs to, because the distance overflows at 256m. But calling accumulate doesnt do anything, because nothing created a distance accumulator.

    If I change the code to create the accumulator when it expands the field, it still doesnt work, because we create a new accumulator for every record.

    So Ive changed the Accumulator code so that add is a no-op if theres already an accumulator, added a call to accumulator.add when expanding fields, and now my code works without having to post-process the distance values.

    My fix: github.com/markw65/fit-javascript-sdk/commit/743435a6574ac4924c0adbc6ad10f61a0f95e4a5

  • 1 month ago

    What device/app is creating the file with compressed_speed_distance? I have not seen a file with that field in 10+ years, but I did make a mental note that field might be an issue in the JavaScript and Python SDKs during development of those SDKs. 

    The other SDKs apply the scale and offset in the getters/setters, so the raw value read from the file is still available for component expansion and other operations. Whereas in the JavaScript and Python SDKs it is a bit of a dance to make it all work.

    There are other fields that use the Accumulator. The the mergeHeartRates utility depends on an accumulated field and we know that is working. 

    We will definitely look at the code that is causing the crash in the decoder. For the rest, we will need to take a closer look.

  • 1 month ago in reply to Ben FIT

    I am yet again unable to post anything meaningful because of the forum bugs. I created an issue here github.com/markw65/fit-file-writer/issues/6 to facilitate the discussion.

  • 1 month ago in reply to markw65

    Ive also added a zip file there including a fit file with compressedSpeedDistance

  • 1 month ago in reply to Ben FIT

    Also, I can confirm that FitCSVTool.jar from the java sdk successfully decodes the compressed_speed_data field into distance, speed, and enhanced_speed.

  • 1 month ago in reply to markw65

    We will add this to our backlog to get fixed. We want the JavaScript and Python SDKs to be robust. Based on the number of impacted users, the time it has taken to discover this issue, and that there is a work around (use the separate speed and distance fields) it may not get fixed until sometime next year. 

  • 12 days ago in reply to Ben FIT

    Thanks. That version passes all my tests.