-
Notifications
You must be signed in to change notification settings - Fork 97
rust: implement sbp2json, json2sbp, json2json [INFRA-125] #790
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of changes here, so it's difficult to review everything in depth. Looks good to me. I did have suggestions for some minor changes.
I'd like to come to a consensus on the Parser object as well, but that's probably less urgent until we start deploying releases to crates.io
| /// | ||
| /// ``` | ||
| /// | ||
| pub fn sbp2json_read_loop( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be handling the same kind of logic that I meant the Parser struct to in the main library. I don't think the Parser is particularly ergonomic as it current stands, and the fact that you had to implement something similar seems to confirm that.
Do you think we should reconsider the Parser object design? I think this is as good a time as any, and the longer we leave it as is makes it more difficult to change it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are definitely things that could be more ergonomic, but the buffer logic here is a pretty verbatim copy of sbp2json in Python: https://github.com/swift-nav/libsbp/blob/master/python/sbp/sbp2json.py#L187 -- are you envisioning that most of this logic would live inside the parser object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, there's already some buffering logic in the parser object but it could be changed to mirror the python logic and the interface could probably made to be more rusty.
Also fix the template for the addition of the test deps.
benjaminaltieri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Built and ran on my machine super easy. Enjoyed a chance to look over the code and see how things are organized. Overall great stuff, can't say I had much to comment on in terms of design choices but it's an extensive solution with a lot of great tooling. Thanks for the hard work @silverjam!
Formalize the Rust variant of the
sbp2json,json2sbp, andjson2jsontools, once merged we'll be able to install these tools with cargo viacargo install --git https://github.com/swift-nav/libsbp.git --bins.The tools are tested with various "round trip" tests that ensure that data converted into and out of the JSON representation of the tool is bit-for-bit identical.
Introduce a benchmark framework for the Haskell, Python and Rust versions of these tools using hyperfine.
Adds builds on Mac OS X and Windows so that we can provide more platform specific builds of the
sbp2json,json2sbp, andjson2jsontools.Behavior difference with Haskell: the float formatting of Haskell's
sbp2jsonuses GHC'sshowFloatbehavior for formatting float values, which is explained in Python sbp2json tool source -- the Pythonsbp2jsontool matches this formatting exactly, the Rust tool does not. The Rust tool attempts to provide some compatibility with a--float-compatswitch but falls short of the Python variant. This behavior difference does not impact actual float values stored in SBP binary (for example when converting Rust SBP JSON output back to SBP binary withjson2sbp).Behavior difference with Haskell: the
sbp2jsontool in Haskell would pass through "messages" that failed CRC validation. It's unclear of this choice was intentional but it makes the Haskell tool not "bit-for-bit" compatible with the Rust tool if the data stream includes "junk". The Rust tool will drop the invalid data. The Haskell tool will pass through the junk data, but as a JSON blob with just common fields such as "message type", "payload" and "crc"-- even if these fields represent junk data.