Skip to content
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.

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

Developer fields #44

Merged
merged 11 commits into from
Oct 14, 2024
Merged

Conversation

Lingepumpe
Copy link
Contributor

@Lingepumpe Lingepumpe commented Aug 13, 2024

This PR is on top of "Apply linter suggestions for cleaner code".

This is a draft PR, I would like some feedback, but functionally it works for me: My fit files with developer fields are now correctly parsed. This addresses issue #41.

What changed:

  • fields are now HashMap<(u8, u8), Value> instead of HashMap<u8, Value>: The first u8 is the dev_data_idx, or 255 if this i not a developer field (but a "normal" field). If I understand the fit protocol document correctly 255 is not a valid dev_data_idx.
  • We keep track of "field definitions" for developer fields, in a HashMap<(u8, u8), DeveloperFieldDescription> called developer_fields. This is owned by the Decoder and when a field definition record is found, a new DeveloperFieldDescription is created from it, and added to the HashMap. The deserializer is passed a read only reference to this map: Inside is the information about the BaseType that is needed by the deserializer in data_message_fields_impl

Open questions:

  • Could/Should we combine FitBaseType and BaseType? I would prefer to only use the auto-generated FitBaseType, and remove BaseType. Currently src/lib.rs:408 looks exceptionally ugly: BaseType::from(FitBaseType::from(fit_base_type_id as &str).as_u8()), this could be nicer if BaseType and FitBaseTyper were the same thing.
  • The BaseType is currently converted to a string by the decoder, and then converted back to a enum entry from string. This is also not the nicest.
  • Error handling: The fit file protocol states that e.g. developer fields must be defined before mentioned, but is it ok to use .expect or panic! if it doesn't follow this? Or should we handle errors more gracefully?

@Lingepumpe
Copy link
Contributor Author

Lingepumpe commented Aug 15, 2024

To address my open questions:

  • I added a commit that removed enum BaseType and replaces it with FitBaseType. It looks cleaner to me, and also less code: 3 files changed, 67 insertions(+), 125 deletions(-). Let me know if you agree with this.
  • Converting back the FitBaseType from String back to FitBaseType (in src/lib.rs:405) shouldn't be too big of a deal, as it occurs only once per developer field, so performance wise it is negligible.
  • Still would like some feedback/checking if my error handling in this PR is ok :)

Unless I get any change requests, this is now good to go from my end - draft status revoked.

fitparser/src/lib.rs Outdated Show resolved Hide resolved
@stadelmanma
Copy link
Owner

stadelmanma commented Aug 16, 2024

Great start on this! I went through an added some comments, for this change I'll need to run it over a separate corpus of test files that I've collected from other FIT file parsing repos.

I also merged your linter work as well.

@Lingepumpe
Copy link
Contributor Author

All the comments have been addressed from my point of view, please take a look. Also, I have rebased the branch on the current master status (since the linter changes were merged).

@Lingepumpe Lingepumpe changed the title Draft: Developer fields Developer fields Aug 17, 2024
@Lingepumpe
Copy link
Contributor Author

If there is anything further I should do, please let me know. :)

@stadelmanma
Copy link
Owner

Thank you for all your work on this and for fielding the in-depth discussion on how best to get this implemented!

@stadelmanma stadelmanma merged commit 1a1ebae into stadelmanma:master Oct 14, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants