As there is no way on github to submit an bug report, please find it here:
Subject:
Encoder: _MesgDefinition.equals() ignores field order, causing silent data corruption when the same field set is written in a different order
Description:
When using garmin_fit_sdk.Encoder to write two messages with the same mesg_num and the same set of field names, but inserted in a different order in the dict passed to write_mesg(), the second message's data gets silently corrupted on decode — with zero errors raised anywhere in either the encode or decode path.
Root cause
_MesgDefinition.equals() (garmin_fit_sdk/mesg_definition.py) compares two field-definition lists using an order-insensitive check:
for lhs in self.field_definitions:
found = any(
lhs['num'] == rhs['num']
and lhs['size'] == rhs['size']
and lhs['base_type'] == rhs['base_type']
for rhs in other.field_definitions
)
if not found:
return False
This only checks that the same fields are present, never that they're in the same position. But the FIT wire format is strictly positional: a Definition Message declares field order once, and every following Data Message for that local slot is raw bytes with no field names — the decoder matches byte N to whatever field is in position N of the last Definition Message it saw for that slot.
Encoder.on_mesg() uses equals() (via _lookup_local_mesg_num) to decide whether it can skip writing a new Definition Message and reuse the currently-active one. Because equals() ignores order, it answers "yes, same" for two messages whose fields are actually in a different order — so the Encoder skips emitting a new Definition Message, while still writing the new message's data bytes in its own (different) order. The decoder is left using the stale Definition Message's field order to interpret data bytes that were actually written in a different order — every field after the first divergence point gets misinterpreted.
Minimal reproduction:
from garmin_fit_sdk import Encoder, Decoder, Stream
import datetime
encoder = Encoder()
encoder.write_mesg({
'mesg_num': 0, 'type': 'workout', 'manufacturer': 'development',
'product': 1, 'product_name': 'x', 'serial_number': 1,
'time_created': datetime.datetime.now(datetime.timezone.utc),
})
encoder.write_mesg({'mesg_num': 26, 'wkt_name': 'repro', 'sport': 'cycling', 'num_valid_steps': 2})
# Same 5 field NAMES as the second message below, different insertion order
encoder.write_mesg({
'mesg_num': 27, 'message_index': 0,
'intensity': 'active', 'duration_type': 'time', 'duration_value': 300000,
'target_type': 'power', 'target_value': 2,
})
encoder.write_mesg({
'mesg_num': 27, 'message_index': 1,
'duration_type': 'repeat_until_steps_cmplt', 'duration_value': 1,
'target_value': 3, 'intensity': 'active', 'target_type': 'open',
})
data = encoder.close()
messages, errors = Decoder(Stream.from_byte_array(data)).read()
print('errors:', errors) # [] <-- no error reported at all
for step in messages['workout_step_mesgs']:
print(step)
Expected: the second workout_step message decodes with duration_type='repeat_until_steps_cmplt', duration_value=1, target_value=3.
Actual: it decodes as complete nonsense (e.g. duration_type='distance', target_type='speed', intensity='other', with garbage large integer values) — silently, with errors == [].
Suggested fix:
In _MesgDefinition.__init__, sort field_definitions into a canonical order (by FIT field number, which is already available on each entry as fp['num']) right after they're built, before the existing equals() check ever runs:
# in _MesgDefinition.__init__, right after the "for field_name in mesg:" loop
# that builds self.field_definitions, before the developer-fields section:
self.field_definitions.sort(key=lambda fd: fd['num'])
This makes the already-existing (order-insensitive) equals() check safe: since every definition is now built in the same canonical order for a given field set, "same fields present" and "same order" become equivalent, so no divergence between what equals() considers identical and what's actually written to the wire is possible anymore. It also has a nice side benefit over the alternative fix (making equals() order-sensitive instead): callers that produce the same field set via different code paths/insertion orders now correctly collapse into the same reused Definition Message, rather than each needing its own — smaller output, not just correct output.
Alternative fix
Instead of canonicalizing field order, equals() could be made order-sensitive — comparing fields positionally rather than via any(...) membership:
def equals(self, other):
if (self.global_message_number != other.global_message_number
or len(self.field_definitions) != len(other.field_definitions)
or len(self.developer_field_definitions) != len(other.developer_field_definitions)):
return False
for lhs, rhs in zip(self.field_definitions, other.field_definitions):
if lhs['num'] != rhs['num'] or lhs['size'] != rhs['size'] or lhs['base_type'] != rhs['base_type']:
return False
# ... developer field comparison unchanged
return True
This also fixes the corruption — two definitions with the same fields in a different order would now correctly be treated as different, so the Encoder would emit a fresh Definition Message before the differently-ordered data, and decoding would work correctly again either way.
The trade-off versus the canonical-ordering fix: it's correct but not optimal. Two calls to write_mesg() that happen to produce the same field set via different insertion orders (exactly the case in the reproduction above) would now always be treated as distinct definitions, each needing its own Definition Message written to the stream — larger output than necessary, since the data itself is equivalent either way. Canonicalizing field order fixes the same bug while also letting those calls collapse into one reused definition, producing smaller files as well as correct ones.
