Add FitFileEncoder for writing FIT files #58

xmedeko · 2018-03-09T15:02:22Z

Fix #8

I have started with FitFileEncoder, see the test_encoder.py. I will rebase this PR while fixing the FitFileEncoder and edit this initial message.

The messages are created by DataMessageCreator just to keep quite a lot of code outside of the core DataMessage.

Cannot write:

compound fields,
accumulated fields,
developer fields.

But these fields are not necessary for an application generated FIT files. The compound and accumulated fields are designed to save a few bits of the resulting FIT file, which is not necessary for a server generated files and, furthermore, I need to use simple FIT features only to make some weak FIT parsers also working.

@dtcooper @pR0Ps Please review the code.
I would appreciate to move some code changes out of this PR and merge it to master to make this PR smaller and simpler to review:

fileish_open, see RFCT utils.fileish_open with a write possibility #54
RFTC Crc, see the commit "RFCT move crc computation to records.Crc, add test". Also speeds up the FitFile when CRC is ignored.
Move apply_scale_offset to the records.py, what about to merge it to the field.render code?
Solve the optimisations Remove RecordBase to speedup processing #57
Rename FitFileDataProcessor.process_xxx methods to parse_xxx
Add BaseTyp.invalid_value, so as the parse does not need to be defined in most of times.

Thank you

pR0Ps · 2018-04-05T11:32:53Z

fitparse/records.py

+    0x07: BaseType(name='string', identifier=0x07, fmt='s', parse=parse_string, unparse=unparse_string, in_range=lambda x: x),
+    0x88: BaseType(name='float32', identifier=0x88, fmt='f', invalid_value=_FLOAT32_INVALID_VALUE,
+                   parse=lambda x: None if math.isnan(x) else x,
+                   in_range=lambda x: x if -3.4028235e+38 < x < 3.4028235e+38 else _FLOAT32_INVALID_VALUE),


Would prefer if this constant was named for readability along with _FLOAT32_INVALID_VALUE:

_FLOAT32_RANGE = (-3.4028235e+38, 3.4028235e+38) in_range=lambda x: x if _FLOAT32_RANGE[0] < x < _FLOAT32_RANGE[1] else _FLOAT32_INVALID_VALUE

Maybe should polish this part, maybe subclass BaseType and use merge in_range into unparse.

pR0Ps · 2018-04-05T11:40:03Z

fitparse/utils.py

+    :rtype bool"""
+    if isinstance(obj, (str, bytes)):
+        return False
+    try:


Can simplify by using the Iterable type from the collections module:

from collections import Iterable def is_iterable(obj): return not isinstance(obj, (str, bytes)) and isinstance(obj, Iterable)

See https://stackoverflow.com/questions/1952464
isinstance(obj, Iterable) does not cover all use cases. However, for most of situations, isinstance is OK. Maybe should do it to speed up the code a bit.

Huh, I actually had no idea that defining __getitem__ on a class would cause it to become iterable. Seems like a legacy feature that was kept in for backwards compatibility after PEP234 was implemented. I did some tests in 2.7/3.6 and it seems that the basic types all implement __iter__ now so as long as custom classes do the same it should be fine.

pR0Ps · 2018-04-05T11:45:47Z

scripts/fitdump

@@ -98,7 +98,7 @@ def main(args=None):

    fitfile = fitparse.FitFile(
        options.infile,
-        data_processor=fitparse.StandardUnitsDataProcessor(),


The fitdump script is user-facing and should therefore convert units to SI by default. Getting the raw values from the file could be useful in some cases, but should be put behind a flag (--raw-values?) if it's going to be added.

This change is a mistake, thanks.

pR0Ps · 2018-04-05T11:57:23Z

fitparse/base.py

-# Python 2 compat
-try:
-    num_types = (int, float, long)
-    str = basestring


str is still used as a typecheck in get_messages in the following code:

names = set([ int(n) if (isinstance(n, str) and n.isdigit()) else n for n in names ])

However, looking at it now, this code can probably be refactored to:

def try_int(obj): try: return int(obj) except ValueError: return obj names = set(try_int(n) for n in names)

This would remove the typecheck so the shim wouldn't be needed.

IMO this code should be removed - the caller should make sure a message number is int, not str if wants to use the number.

@pR0Ps Should I remove this from the FitFile?

pR0Ps · 2018-04-05T11:59:56Z

fitparse/encoder.py

+        :param profile_version: profile version.
+        :param data_processor: custom data processor.
+        """
+        self.protocol_version = 1.0 if protocol_version is None else float(protocol_version)


Any reason to not put these defaults in the function definition? Would be more self-documenting.

Not any reason, just a mistake, thanks.

pR0Ps · 2018-04-05T12:07:47Z

fitparse/encoder.py

+        self._crc.update(data)
+
+    def _write_struct(self, data, fmt, endian='<'):
+        if fmt.startswith('<') or fmt.startswith('>'):


In general, allowing multiple ways to specify a parameter can get confusing (how do they interact?). In this case I would either raise an error if the endianness is specified in both parameters or just not allow it in the fmt at all since (as far as I can tell) it's only used for the CRC.

IMO it's a design flaw in the fitparse base, already. E.g. see test.py func generate_fitfile() - there is copy&paste of the struct.pack spec for the FIT header and Crc. I've tried to fix that for Crc at least. But I can copy&paste the endianness for the Crc, too.

pR0Ps · 2018-04-05T12:32:01Z

fitparse/encoder.py

+from .utils import fileish_open, FitParseError
+
+
+class FitFileEncoder(object):


It doesn't look like this class allows for creating chained FIT files (basically multiple FIT files concatenated together, see the commit that added support for parsing them). Not saying it has to, just letting you know that it's a possibility since it's a bit of an obscure part of the spec.

In fact, if you want to support chained FIT files, it should probably be taken care of by higher-level code that basically just does an itertools.chain(*fileishs) for the fileishs written by this class.

Exactly, the code itself allows to append to a file, too. Just the file has to be opened by the user code. So, chaining is the task for the user code. Should not burden the core code.

I get that it shouldn't be done as part of the normal encoding since it's at a higher level than messing around with the bytes of the file, but that doesn't mean it can't be included in the library.

If it's a common operation, pushing it into user code just means that everyone is going to have to write their own implementation. A better way would be to provide all the functionality the user would need, but let them decide when to use it. For example, one way would be to have a small method chain on the class that does something like (untested):

def chain(self, *fits): if not self.completed: raise Exception("Can't chain more files onto an incomplete FIT file") for fit in fits: if isinstance(fit, FitFile): data = fit._file else: data = fileish_open(fit, 'rb') while True: block = data.read(BLOCK_SIZE) if not block: break self._write(block)

This way the library can check for issues (like appending data to an incomplete fit datastream) as well as handle compatibility with library objects (the FitFile shim), but it doesn't complicate the rest of the encoding process in any way.

You can already chain with this code:

with FitFileEncoder(open(fitname, 'ab') as fit: ... with FitFileEncoder(open(fitname, 'ab') as fit: ...

I have not seen any particular support for chaining in the FIT SDK, I think they support it the same way.

xmedeko · 2018-04-12T20:17:39Z

@pR0Ps I've addressed most of the comments by amend. See the rest.
Do not want to merge it into the master until #57 is resolved.

vlcvboyer · 2019-01-03T10:23:06Z

+1 writing fit files will be useful !
Please see my PR on @xmedeko fork which includes some scripts to fix FIT files or combine them into multi-sport ativities:
[pull request on xmedeko fork] (xmedeko#1)
Unfortunately there are conflicts with upstream.... I've not changed anything yet as I can see some different opinions in this stream...

23chrischen · 2019-02-01T12:00:38Z

+1 would love to see this merged as well!

Kypaz · 2019-06-05T08:11:51Z

+1, would love to have this merged in master. Will have to clone this manually to get access to this new feature for now ! Thanks for this !!

xmedeko · 2019-06-05T09:27:54Z

I am sorry but I do not plan to maintain this PR and resolve conflicts due to the development in the master. Personally, I have decided do abandon FIT writing (and parsing, too) in pure Python since the FIT format is loosely specified and complex. I recommend to make some FIT<-->JSON tool from the original FIT SDK (e.g. C#, Java, C++) and just call this tool from Python.

xmedeko force-pushed the write branch 11 times, most recently from 1073502 to 0cfa1be Compare March 14, 2018 21:47

xmedeko mentioned this pull request Mar 16, 2018

Remove RecordBase to speedup processing #57

Open

xmedeko force-pushed the write branch 2 times, most recently from a66d1e6 to f93d871 Compare March 16, 2018 18:28

xmedeko force-pushed the write branch 2 times, most recently from 4a9b763 to 314569c Compare April 5, 2018 06:53

pR0Ps reviewed Apr 5, 2018

View reviewed changes

xmedeko force-pushed the write branch from 314569c to 044afad Compare April 12, 2018 20:11

Add FitFileEncoder for writing FIT files

66b3399

xmedeko force-pushed the write branch from 044afad to 66b3399 Compare April 12, 2018 20:21

This was referenced Apr 24, 2018

FitParseError invalid field size 227 for type 'sint32' #64

Closed

FitFile.get_messages remove parsing names to int, remove Python 2 str shim #66

Merged

xmedeko mentioned this pull request Oct 15, 2018

Saving fit files? #77

Open

xmedeko mentioned this pull request Jan 3, 2019

several functions to fix FIT files xmedeko/python-fitparse#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FitFileEncoder for writing FIT files #58

Add FitFileEncoder for writing FIT files #58

xmedeko commented Mar 9, 2018 •

edited

pR0Ps Apr 5, 2018

xmedeko Apr 6, 2018

pR0Ps Apr 5, 2018

xmedeko Apr 6, 2018

pR0Ps Apr 9, 2018

pR0Ps Apr 5, 2018

xmedeko Apr 6, 2018

pR0Ps Apr 5, 2018

xmedeko Apr 6, 2018

xmedeko Apr 12, 2018

pR0Ps Apr 5, 2018

xmedeko Apr 6, 2018 •

edited

pR0Ps Apr 5, 2018

xmedeko Apr 6, 2018

pR0Ps Apr 5, 2018

xmedeko Apr 6, 2018

pR0Ps Apr 9, 2018

xmedeko Apr 10, 2018 •

edited

xmedeko commented Apr 12, 2018

vlcvboyer commented Jan 3, 2019

23chrischen commented Feb 1, 2019

Kypaz commented Jun 5, 2019

xmedeko commented Jun 5, 2019

		from .utils import fileish_open, FitParseError


		class FitFileEncoder(object):

Add FitFileEncoder for writing FIT files #58

Are you sure you want to change the base?

Add FitFileEncoder for writing FIT files #58

Conversation

xmedeko commented Mar 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmedeko Apr 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmedeko Apr 10, 2018 • edited

Choose a reason for hiding this comment

xmedeko commented Apr 12, 2018

vlcvboyer commented Jan 3, 2019

23chrischen commented Feb 1, 2019

Kypaz commented Jun 5, 2019

xmedeko commented Jun 5, 2019

xmedeko commented Mar 9, 2018 •

edited

xmedeko Apr 6, 2018 •

edited

xmedeko Apr 10, 2018 •

edited