Skip to content

Implement handling of invalid values in arrays #46

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Implement handling of invalid values in arrays #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4b77bb6
Implement handling of invalid values in arrays
stadelmanma Jan 4, 2025
c683331
Update tests
stadelmanma Jan 4, 2025
7e6a6dd
Update CHANGELOG
stadelmanma Jan 4, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
# Changelog

## HEAD
* Improve support for array handling in FIT files (mat-kie)

## v0.8.0
* Bump packaged FIT SDK version to 21.141.00 (lingepumpe)
* Implement developer field data parsing (lingepumpe)


## v0.7.0
* Bump packaged FIT SDK version to 21.133.00 (robinkrahl)
* Remove dead code files in generate-fit-profile (robinkrahl)
Expand Down
37 changes: 24 additions & 13 deletions fitparser/src/de/parser.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider let mut value = Vec::with_capacity(size as _); in line 574.

Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ impl Value {
Value::UInt64z(val) => *val != 0x0,
// TODO: I need to check this logic, since for Byte Arrays it's only invalid if
// all the values are invalid. Is that the case for all array fields or just "byte arrays"?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider removing this comment since this should be resolved with this PR

Value::Array(vals) => !vals.is_empty() && vals.iter().all(|v| v.is_valid()),
Value::Array(vals) => !vals.is_empty() && vals.iter().any(|v| v.is_valid()),
Value::Invalid => false,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a hypothetical issue here. Since values are abstracted as enum variants, we could in theory create a Value::Array which contains values of differing types. If those values would be valid, this code would return True. However an array of values should never contain values of differing types.

As long as the parsing code is well-behaved this case should never appear but if this should be checked I would propose something like this:

 if let Some(first) = vals.first().map(|v|discriminant(v)) {
    vals.iter().all(|x|matiches!(x, Value::Invalid) || discriminant(x) == first) && vals.iter().any(|v| v.is_valid())
 } else {
    false
}

or a bit less concise but without double iteration:

if let Some(first) = vals.first().map(|v| discriminant(v)) {
    let mut same_type = true;
    let mut any_valid = false;
    for v in vals{
        same_type &= matiches!(x, Value::Invalid) ||  discriminant(v) == first;
        any_valid |= v.is_valid();
    }
    same_type && any_valid
} else {
    false
}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, I have that noted in the actual
Value enum as a known edge case for Arrays. It’s not possible to parse a multi-type array directly due to how the FIT spec defines fields in the definition messages.

If FIT file writing ever gets implemented then we’d want to validate array contents somewhere along the way in the serialization process.

}
}
}
Expand Down Expand Up @@ -595,7 +596,11 @@ fn data_field_value(
_ => le_u8(input).map(|(i, v)| (i, Value::UInt8(v)))?, // Treat unexpected like Byte
};
bytes_consumed += base_type.size();
values.push(value);
if value.is_valid() {
values.push(value);
} else {
values.push(Value::Invalid)
}
Comment on lines -598 to +603
Copy link
Contributor

@mat-kie mat-kie Jan 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the FIT file specification again, I think there might be an Issue here. As I unuderstand it now, the special remark regarding the Byte value and arrays of it addresses the following situation:

For all Value types except the Byte type, value validity for values inside an array is determined on a per value basis. For byte values however, if one value inside the array is considered valid, all should be considered valid.

I think this might be the case, because otherwise not a single byte inside a Byte array filed would be allowed to be 0xFF.

Currently, a byte array [0xFE, 0xFF] would be returned as vec![Byte(0xFE), Invalid], I think what sould be returned in this case (and only for the Byte value type) would be vec![Byte(0xFE), Byte(0XFF)]. However I am not 100% shure about this.

My proposal:

if matches!(base_type, FitBaseType::Byte) || value.is_valid()  {
    values.push(value);
} else {
    values.push(Value::Invalid)
}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are spot on, great catch on that bug.

input = i;
}

Expand All @@ -606,7 +611,7 @@ fn data_field_value(
Value::Array(values)
};

// Only return "something" if it's in the valid range
// Strip invalid values from the output
if value.is_valid() {
Ok((input, Some(value)))
} else {
Expand Down Expand Up @@ -684,7 +689,7 @@ mod tests {

#[test]
fn data_field_value_test_array_value() {
let data = [0x00, 0x01, 0x02, 0x03, 0xFF];
let data = [0x00, 0x01, 0x02, 0x03, 0xFF, 0x05];

// parse off a valid byte
let (rem, val) =
Expand All @@ -701,18 +706,24 @@ mod tests {
),
None => panic!("No value returned."),
}
assert_eq!(rem, &[0xFF]);
assert_eq!(rem, &[0xFF, 0x05]);

// parse off an invalid byte
let (rem, val) =
data_field_value(&data, FitBaseType::Uint8, Endianness::Native, 5).unwrap();
if val.is_some() {
panic!("None should be returned for invalid bytes.")
}
assert_eq!(rem, &[]);

if val.is_some() {
panic!("None should be returned for array with an invalid size.")
data_field_value(&data, FitBaseType::Uint8, Endianness::Native, 6).unwrap();
match val {
Some(v) => assert_eq!(
v,
Value::Array(vec![
Value::UInt8(0x00),
Value::UInt8(0x01),
Value::UInt8(0x02),
Value::UInt8(0x03),
Value::Invalid,
Value::UInt8(0x05),
])
),
None => panic!("No value returned."),
}
assert_eq!(rem, &[]);
}
Expand Down
17 changes: 16 additions & 1 deletion fitparser/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,8 @@ pub enum Value {
/// Array of Values, while this allows nested arrays and mixed types this is not possible
/// in a properly formatted FIT file
Array(Vec<Self>),
/// Placeholder for invalid values in output
Invalid,
}

impl fmt::Display for Value {
Expand All @@ -224,7 +226,8 @@ impl fmt::Display for Value {
Value::Float32(val) => write!(f, "{}", val),
Value::Float64(val) => write!(f, "{}", val),
Value::String(val) => write!(f, "{}", val),
Value::Array(vals) => write!(f, "{:?}", vals), // printing arrays is hard
Value::Array(vals) => write!(f, "{:?}", vals), // printing arrays is hard,
Value::Invalid => write!(f, ""),
}
}
}
Expand Down Expand Up @@ -257,6 +260,10 @@ impl convert::TryInto<f64> for Value {
Value::Array(_) => {
Err(ErrorKind::ValueError(format!("cannot convert {} into an f64", self)).into())
}
Value::Invalid => Err(ErrorKind::ValueError(format!(
"cannot convert an invalid value into an f64"
))
.into()),
}
}
}
Expand Down Expand Up @@ -293,6 +300,10 @@ impl convert::TryInto<i64> for Value {
Value::Array(_) => {
Err(ErrorKind::ValueError(format!("cannot convert {} into an i64", self)).into())
}
Value::Invalid => Err(ErrorKind::ValueError(format!(
"cannot convert an invalid value into an i64"
))
.into()),
}
}
}
Expand Down Expand Up @@ -329,6 +340,10 @@ impl convert::TryInto<i64> for &Value {
Value::Array(_) => {
Err(ErrorKind::ValueError(format!("cannot convert {} into an i64", self)).into())
}
Value::Invalid => Err(ErrorKind::ValueError(format!(
"cannot convert an invalid value into an i64"
))
.into()),
}
}
}
Expand Down
8 changes: 7 additions & 1 deletion fitparser/src/profile/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ impl Value {
Value::UInt64(val) => val.to_ne_bytes().to_vec(),
Value::UInt64z(val) => val.to_ne_bytes().to_vec(),
Value::Array(vals) => vals.iter().flat_map(|v| v.to_ne_bytes()).collect(),
Value::Invalid => Vec::new(),
}
}
}
Expand Down Expand Up @@ -192,6 +193,9 @@ pub fn calculate_cumulative_value(
.into())
}
}
Value::Invalid => {
Err(ErrorKind::ValueError("Cannot accumlate invalid fields".to_string()).into())
}
}
} else {
accumulate_fields.insert(key, value.clone());
Expand Down Expand Up @@ -282,7 +286,9 @@ fn convert_value(
}

fn apply_scale_and_offset(value: Value, scale: f64, offset: f64) -> Result<Value> {
if ((scale - 1.0).abs() > f64::EPSILON) || ((offset - 0.0).abs() > f64::EPSILON) {
if value != Value::Invalid
&& (((scale - 1.0).abs() > f64::EPSILON) || ((offset - 0.0).abs() > f64::EPSILON))
{
let val: f64 = value.try_into()?;
Ok(Value::Float64(val / scale - offset))
} else {
Expand Down
Loading