Initial writer support #34
Conversation
|
The first commit is necessary for me, otherwise fitgen segfaults. I'm not sure why I'm seeing it if you aren't, but I think it should be harmless. |
There was a problem hiding this comment.
Absolutely amazing work. Thank you very much!
The round-trip DecodeEncodeDecode test is very nice.
I added some very minor comments.
If you want you can also
- add an edit of the README to state that encoding is now supported,
e.g. "implements decoding and encoding". - Add "Fixes writer: encoding of FIT files #1" to either the PR description or one of the last commits.
It would be nice to "contextualize" the Encoder error returns,
e.g. encoding failed: error writing foo: some i/o error,
but that can be done later.
Some XLSX sheets return "short" rows in a number of places, initalise the slice to be always the length of the header to avoid out-of-bounds accesses. This will presumably waste some memory, but is the least intrusive fix.
To make the test work across gofmt changes, calculate the golden fingerprint at runtime, by running the golden data through go/format.Source() This ensures that the go code is equivalent, but doesn't care if the golden output was generated with a different version of gofmt. Fixes tormoder#32
It's a bit more obvious to emit this from the same place as the opening brace.
This should be somewhat easier to use for creating new files than the reflect.Value-based approach, though some reflection will be needed to utilise these constructors in the reader code. Also update the generator test golden files.
Instead of the array of invalid field values, just use the new constructors. It removes a little duplication. Also update the generator golden files.
Most fields which aren't included in the profile have an empty cell in the EXAMPLE column, however a few of the fields in the Developer field_definition message have a "0". It seems that the intention is for these to be omitted (they aren't in the table in the PDF), so add a new condition for "skip" if EXAMPLE is "0"
For strings and arrays, the size of a field isn't fully defined by its BaseType. Instead, store a length value in the field array, to be used by the writer. For arrays where the size is defined globally (in the "Array" column of the spreadsheet), use that. Otherwise, if Array == "[N]", or the type is a string, take the value from the EXAMPLE column, which defines the array/string length in that particular profile. The decoder can likely safely ignore this, and just use whatever size is defined in the file itself.
We need to get MesgNum in the writer to be able to encode messages
This is the lowest level of the encoder implementation, simply encoding a FIT type into a binary form.
|
Thanks for the review! All the comments make sense, I'll send a respin with them fixed. In the process of implementing your suggestions, though, I've found that the DecodeEncodeDecode test wasn't really doing anything after a last minute change, because I I'm about to go away for a couple of weeks, but I will try to get the update pushed while I'm travelling. |
Uses reflect to iterate through the message struct, and writes based on the field definition. The field defitions are got from the profile.
Uses reflect, and the profile, to go through all the fields in a message and determine if their value is valid, and create an encodeMesgDef which will encode those fields in writeMesg().
Includes a test which decodes the testdata, re-encodes it, then decodes again to compare the result.
|
So it wasn't as bad as I thought. I had to "blacklist" a couple more files in the round-trip test, but there's still 12 files which get run through it. I opened #36 and #37 for the major limitations, and tagged the I added some text to the merge message at the top, I hope that works. I've not done many Github pull requests before 😄 |
|
Great, thank you for writing the down the limitations. I'll merge this and tag a 0.4.0 release soon. Thanks again! |
Implements support for encode, with some known limitations (e.g. #36, #37)
Fixes #1