Skip to content

Python: never write the FIT invalid value as a measurement. #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

Open
sanderroosendaal wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Python: never write the FIT invalid value as a measurement. #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d80904e
Python: never write the FIT invalid value as a measurement.
Oct 13, 2026
46d5e02
Merge remote-tracking branch 'github/main' into python/invalid-values
Oct 14, 2026
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
11 changes: 8 additions & 3 deletions python/src/rowing_data/codec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,14 @@
``physical = raw / scale`` and ``raw = round(physical * scale)``.

Absent values are ``None`` and MUST be omitted from the file — never encoded
as a placeholder such as -1 or 999.
as a placeholder such as -1 or 999. Each base type's FIT invalid value (spec
§7.2) means "no data": ``encode`` never produces it for a measurement, and
``decode`` returns ``None`` for it.
"""

from __future__ import annotations

from .fields import BASE_TYPE_RANGE, FieldDef
from .fields import BASE_TYPE_INVALID, BASE_TYPE_RANGE, FieldDef


class CodecError(ValueError):
Expand All @@ -27,12 +29,15 @@ def encode(field: FieldDef, physical: float | int | None) -> int | None:
f"{field.name}: encoded value {raw} does not fit {field.base_type} "
f"range [{lo}, {hi}] (physical={physical!r}, scale={field.scale})"
)
invalid = BASE_TYPE_INVALID[field.base_type]
if raw == invalid:
return invalid - 1

@JBlaak JBlaak Oct 14, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So we're altering data, doesn't sound like a best practice. I would throw an CodecError as it simply doesn't fit the codec.

Also while we're on this topic, when physical is None shouldn't it return BASE_TYPE_INVALID[field.base_type]?

@sanderroosendaal sanderroosendaal Oct 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

On clipping: the merged spec (#38, §7.2) asks for exactly this. Producers MUST NOT write the invalid value for a measurement, and "a measurement that would encode to it is clipped to the next valid value". A value beyond the type limit still raises CodecError; only the single value that collides with "no data" is clipped, by one raw step. If we would rather raise there, that is a spec change, so it should be raised as an issue against §7.2 rather than decided in this PR.

@sanderroosendaal sanderroosendaal Oct 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

On None: encode(None) returning None means "omit the field", which is what §7.2 asks for ("Missing or unavailable fields SHOULD be omitted from the file"). Writing the invalid value instead would also be legal FIT, but it would write a field the spec says to leave out, and it would change how the writer builds records. decode is the symmetric side: it maps the invalid value back to None, so a file from a producer that does write it reads as missing.

return raw


def decode(field: FieldDef, raw: int | None) -> float | int | None:
"""Convert a FIT raw integer to a physical value, or ``None`` if absent."""
if raw is None:
if raw is None or raw == BASE_TYPE_INVALID[field.base_type]:
return None
if field.scale == 1:
return int(raw)
Expand Down
9 changes: 9 additions & 0 deletions python/src/rowing_data/fields.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ class BaseType(StrEnum):
BaseType.SINT32: (-2_147_483_648, 2_147_483_647),
}

# FIT invalid value per base type: it means "no data" and is never a measurement.
BASE_TYPE_INVALID: dict[BaseType, int] = {
BaseType.UINT8: 255,
BaseType.UINT16: 65535,
BaseType.UINT32: 4_294_967_295,
BaseType.SINT16: 32767,
BaseType.SINT32: 2_147_483_647,
}


@dataclass(frozen=True, slots=True)
class FieldDef:
Expand Down
18 changes: 17 additions & 1 deletion python/tests/test_codec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,22 @@ def test_encode_overflow_raises() -> None:

def test_uint16_upper_bound_accepted() -> None:
work = field_by_id(19)
assert encode(work, 65535) == 65535
assert encode(work, 65534) == 65534
with pytest.raises(CodecError):
encode(work, 65536)


@pytest.mark.parametrize(
("name", "invalid"),
[
("StrokeWork", 65535), # UINT16
("WorkoutState", 255), # UINT8
],
)
def test_invalid_value_is_never_written_as_a_measurement(
name: str, invalid: int
) -> None:
field = field_by_name(name)
assert encode(field, invalid / field.scale) == invalid - 1
assert decode(field, invalid) is None
assert decode(field, invalid - 1) is not None
45 changes: 45 additions & 0 deletions python/tests/test_reader.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,51 @@ def test_unknown_stroke_state_is_unknown(tmp_path: Path) -> None:
assert any(issue.code == "stroke_state" for issue in loaded.read_issues)


def test_invalid_value_is_read_as_missing(tmp_path: Path) -> None:
path = tmp_path / "invalid.fit"
builder = FitFileBuilder(auto_define=True, min_string_size=64)
file_id = FileIdMessage()
file_id.type = FileType.ACTIVITY
file_id.manufacturer = Manufacturer.DEVELOPMENT
file_id.time_created = _ts_ms()
builder.add(file_id)
dev_id = DeveloperDataIdMessage()
dev_id.application_id = APPLICATION_ID
dev_id.developer_data_index = 0
builder.add(dev_id)
desc = FieldDescriptionMessage()
desc.developer_data_index = 0
desc.field_definition_number = 19
desc.fit_base_type_id = BaseType.UINT16.value
desc.field_name = "StrokeWork"
desc.scale = 1
desc.offset = 0
desc.units = ""
builder.add(desc)
work = DeveloperField(
developer_data_index=0,
field_id=19,
size=2,
name="StrokeWork",
base_type=BaseType.UINT16,
scale=1,
offset=0,
units="",
)
work.set_value(0, 65535)
session = SessionMessage()
session.start_time = _ts_ms()
session.timestamp = _ts_ms()
builder.add(session)
rec = RecordMessage(developer_fields=[work])
rec.timestamp = _ts_ms()
builder.add(rec)
builder.build().to_file(str(path))

loaded = read_fit(path)
assert loaded.records[0].stroke_work_j is None


def test_invalid_recording_strategy_is_unknown_with_warning(tmp_path: Path) -> None:
path = tmp_path / "bad_strategy.fit"
builder = FitFileBuilder(auto_define=True, min_string_size=64)
Expand Down
Loading