FitFileParser icon indicating copy to clipboard operation
FitFileParser copied to clipboard

Missing "custom_target_value_low" field for "heart_rate" target_type

Open hesselbom opened this issue 2 years ago • 14 comments

First of all, thanks for a great lib. Most things are working great but found an issue.

The issue

I'm importing a structured workout .fit file where a workout step has target_type: "heart_rate" and a "custom_target_value_low"=240 & custom_target_value_high="260".

When viewing the file through https://www.fitfileviewer.com/ it shows these fields. Screenshot 2023-01-13 at 14 37 42

In the README.md of this project it recommends FIT File Explorer, and when using that tool it also shows these fields, but named custom_target_heart_rate_high, which actually might be more correct when looking at documentation (https://developer.garmin.com/fit/file-types/workout/#targettype). Screenshot 2023-01-13 at 14 41 58

But when using this lib, FitFileParser, they are not included. Instead there's a target_hr_zone with a value of 0. Screenshot 2023-01-13 at 14 37 50

Steps to reproduce

  1. Create a new swift project
  2. Write the following code:
        if let fit = FitFile(file: selectedFile) {
            let steps = fit.messages(forMessageType: .workout_step)
            for step in steps {
                print(step.interpretedFieldKeys())
            }
        }
  1. Use the attached .fit file and run
  2. Notice the output is:
["target_hr_zone", "duration_time", "intensity", "message_index_value", "target_type", "duration_type", "wkt_step_name", "message_index"]
["message_index_value", "duration_type", "intensity", "message_index"]

Expected to happen

The output should include the custom_target_value_low/custom_target_heart_rate_low and custom_target_value_high/custom_target_heart_rate_high fields.

.fit file I use

2023-01-13_heartratef.fit.zip

hesselbom avatar Jan 13 '23 13:01 hesselbom

An interesting finding. It looks like the issue happened between version 1.4.5 and 1.4.6 because if I change the Swift package to use version 1.4.5 and run the following code (same as before):

        if let fit = FitFile(file: selectedFile) {
            let steps = fit.messages(forMessageType: .workout_step)
            for step in steps {
                print(step.interpretedFieldKeys())
            }
        }

I now instead get the following (expected) output (notice how custom_target_heart_rate_high + custom_target_heart_rate_low is included now):

["target_hr_zone", "custom_target_heart_rate_high", "message_index", "duration_type", "duration_time", "wkt_step_name", "custom_target_heart_rate_low", "intensity", "target_type"]
["intensity", "duration_type", "message_index"]

As soon as I go up to 1.4.6 or higher this starts failing as described initially.

hesselbom avatar Jan 13 '23 14:01 hesselbom

Next finding! If I change the code to the following (parsingType: .generic, with the latest version, 1.4.8) it sort of works:

      if let fit = FitFile(file: selectedFile, parsingType: .generic) {
            let steps = fit.messages(forMessageType: .workout_step)
            for step in steps {
                print(step.interpretedFieldKeys())
                print(step.interpretedFields())
            }
        }

The interpreted field looks like this:

"custom_target_heart_rate_high": FitField(withName: workout_hr_260)

Compared to the one in version 1.4.5:

"custom_target_heart_rate_high": FitField(withValue: 260.0, andUnit: % or bpm)

So I can get the value now in 1.4.8 but I get it as a String (workout_hr_260) that I have to parse, instead of as a Double (260.0) as I can get in 1.4.5.

hesselbom avatar Jan 13 '23 14:01 hesselbom

Thanks for the detailed issue. I understand this issue, it has to do with the interpretation of the profile.xls, in the case of this field, it's a special type that says:

0 - 100 indicates% of max hr; >100 indicates bpm (255 max) plus 100

I will add logic for that. This was introduced in the new version as it included a fix to handle definition with masked value like left_balance. workout_hr appears initially like a masked value but it's not so it confuses the logic...

I'll add a fix

roznet avatar Jan 14 '23 13:01 roznet

do you mind if I use your example file and include it as a test case?

roznet avatar Jan 14 '23 13:01 roznet

Go ahead, it's a simple test file for me too.

hesselbom avatar Jan 14 '23 14:01 hesselbom

pushed a fix, thanks for the bug. let me know if you still have some issues

roznet avatar Jan 15 '23 06:01 roznet

Note also that it does not really get the unit resolved, the specification for field with offset in the profile file is a bit incomplete, and I already had to do some specific handling for the workout_hr type, so I felt it was not worth make the code overly specialised to handle a unit that would be dependent on another field like in that case and kept it as "% or bpm"

roznet avatar Jan 15 '23 06:01 roznet

Awesome!

I noticed one thing though, I now get the value with offset already applied. So in this case I get "custom_target_heart_rate_low" as 140 but it should be 240 so that I can use my own logic to determine whether it's relative or absolute, as indicated on https://developer.garmin.com/fit/file-types/workout/#settingpowerandheartratevalues.

Same for power, custom_target_power_lowwas in the file set as 1100 (to indicate an absolute 100 value) but I now get it as 100 triggering my code to believe it's a relative value instead.

I think "rzfit_swift_value_from_workout_power" and "rzfit_swift_value_from_workout_hr" should probably just return the value as is to be more in line with the documentation.

hesselbom avatar Jan 15 '23 09:01 hesselbom

Hum. you make a very good point. but your message got me a bit confused as to how I should deal with it :)

The library attempts to interpret as much as possible based on the Profile.xlsx, so for example it will convert the value to the fields based on reference fields, apply the masks, etc.

For the mask, the library separate the interpretation of the mask and the value, rather than leaving the raw value containing the mask as well. (for example left_right_balance)

I felt for consistency I should do the same for offset rather than just return the raw value. But because the offset fields seem more like a value, and the interpretation a unit, instead of using _value, I use _unit to store what unit it was in.

So you can find the field custom_target_heart_rate_high and custom_target_heart_rate_high_unit which will contain % or bpm.

Maybe not be exactly what you were after, but I think it solves your problem and it keeps the library behaviour consistent. Fair?

roznet avatar Jan 15 '23 15:01 roznet

Sounds like it should work. For HR any value above 100 is considered absolute (offset by 100) and below relative. So a value of 70 would be 70% of max HR and 170 would be exactly 70 BPM, if I understand correctly.

I don't know the ins and outs of this lib's code but it looked like rzfit_swift_value_from_workout_hr would give a value of -30 (70-100, i.e. minus the absolute offset) instead of 70 if the raw value was 70.

Please do correct me if I'm wrong though.

hesselbom avatar Jan 15 '23 15:01 hesselbom

You were right, but no longer :) I fixed it in the last commit...

So for HR now:

  • if custom_target_heart_rate_high = 70 you will get custom_target_heart_rate_high = 70 and custom_target_heart_rate_high_unit = %
  • if custom_target_heart_rate_high = 250 you will get custom_target_heart_rate_high = 150 and custom_target_heart_rate_high_unit = bpm

Hope I got it right this time. I don't know if you know how to generate a file with % as an input so I could test it?

Anyway, I'll get there eventually... thanks for your patience...

roznet avatar Jan 15 '23 17:01 roznet

I see, terrific!

My test file was generated from a service that only deals with absolute values. Could try looking around for one dealing with relative ones.

hesselbom avatar Jan 15 '23 20:01 hesselbom

Don't worry then, it was just if easy to generate.

Let me know if latest version works for you and I will close the issue

roznet avatar Jan 16 '23 04:01 roznet

Almost!

return input < offset ? Double(input) : Double(input - offset) should be return input <= offset ? Double(input) : Double(input - offset)

I.e. 100 = 100% and 101 = 1bpm for HR, and 1000 = 100% and 1001 = 1power for power.

hesselbom avatar Jan 16 '23 11:01 hesselbom