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

writer: encoding of FIT files #1

Closed
tormoder opened this issue Sep 29, 2015 · 8 comments · Fixed by #34
Closed

writer: encoding of FIT files #1

tormoder opened this issue Sep 29, 2015 · 8 comments · Fixed by #34

Comments

@tormoder
Copy link
Owner

No description provided.

@tormoder tormoder added this to the gofit release 0.4.0 milestone Sep 29, 2015
@tormoder tormoder modified the milestones: fit release 0.4.0, Unplanned Apr 3, 2017
@tormoder tormoder changed the title Encoding of FIT files writer: encoding of FIT files Apr 3, 2017
@torgil
Copy link

torgil commented Jan 23, 2018

I'm interested in this. Any thoughts/ initial design/ branches/ code here?
My first (urgent) need is to just modify existing fields so I'm looking for a data stored as-is / modify data-structures / save - approach.

@tormoder
Copy link
Owner Author

Hi. Thanks for your interest in the project.

I think this is an issue that will require quite a lot of work, and I currently don't have time to work on this. But I'm happy to take any contributions.

I'm not sure what would be the best way to implement this. It would require the package to know the FIT base type for struct fields (e.g. a Go uint32 could be a FIT uint32 or uint32z). This information would have to be somewhere. I have thought about using struct tags, but I don't know if the would be a solution.

@torgil
Copy link

torgil commented Jan 27, 2018

Thanks. I'm new to go so I'll try to take different approaches and see what fits best. I'm in the process of migrating away from Python so I prefer to smoke out as many corners as possible.
My initial thoughts here is that following the code auto-generation path looks pretty straight forward for quick results.

@usedbytes
Copy link
Contributor

I've been working on this for a little while. Initial stab is here: https://github.com/usedbytes/fit/tree/writer-dev

It's slightly limited (arrays and local times not supported, compressed timestamps not supported), but it's a starting point.

I wanted to see how likely it is to get merged before I spend loads more time polishing it.

@tormoder tormoder removed this from the Unplanned milestone Apr 28, 2019
@tormoder
Copy link
Owner Author

tormoder commented Apr 28, 2019

Hi!

I would be very happy to receive contributions regarding encoding. I skimmed your writer branch and it looks very nice. I also have no problems with an initial version as long we document what is missing.

Please note that I have very limited with time for this project right now. But, I will try to review any PRs, and additionally try to do a little bit of "housekeeping", e.g. converting the project to use Go modules.

@usedbytes
Copy link
Contributor

Great, I've got a fair bit of tidying and testing to do, but I'll send in pull requests when it's ready. I'm going to focus on the encoder stuff right now, but I'd be happy to help with housekeeping stuff too.

I'll fix arrays, and I know how to do compressed timestamps. The thing I can't figure out is local times. I saw some of the testdata has them, so I guess I'll just try and do the opposite of the decoder; but any pointers are gladly received.

@usedbytes
Copy link
Contributor

I fixed the missing support for arrays and local times, rebased onto your recent changes and submitted a pull here: #34

It's tested by decoding the testdata files, encoding them and decoding them again. There's a couple of exceptions where it doesn't match - tagged in the reader_files_test.go list. There's a decision to be made about how to handle encoding of files whose messages contain different valid fields (so they can't all be described by the same definition message), but I see that as an enhancement rather than a must-have.

IMO it would be good to merge this now if you're happy with it - not least because the rebases are really painful! :-)

@tormoder
Copy link
Owner Author

tormoder commented May 7, 2019

Thanks! I'll have a look on Thursday.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants