Add library support to read and write SBP files #780
Conversation
lkloh
force-pushed
the
lkloh_read
branch
2 times, most recently
from
d3ab4f6 to
1d066f7
Compare
| #include <libsbp/cpp/message_handler.h> | ||
|
|
||
| namespace sbp { | ||
|
|
There was a problem hiding this comment.
Following example of state.h have implementation in header file too.
There was a problem hiding this comment.
My original intention was that all of the C++ wrappers would be header only. I think that's fine to continue here, especially since these are nice small classes.
c/include/libsbp/cpp/sbp_stdio.h
Outdated
|
|
||
| class SbpFileReader : public sbp::IReader { | ||
| public: | ||
| SbpFileReader(const std::string &file_path) |
There was a problem hiding this comment.
We should probably take in a const char * here. It's a little more universal, and a std::string can be easily converted into it with .c_str(). I'm not sure why we'd have a situation where we are using std::ifstream but couldn't use a std::string, but it wouldn't hurt to keep our dynamic memory requirements low.
There was a problem hiding this comment.
Is the general policy to use const char * where possible (unless you need to string manipulation functions that std::string offers?)
I was reading Cases where you might prefer char* over std:string here: https://www.geeksforgeeks.org/char-vs-stdstring-vs-char-c/
There was a problem hiding this comment.
For general C++ programming I'd say std::string is almost always preferred. But since we're attempting to keep libsbp code more or less portable const char * is the lowest common denominator. Like I said before, it's difficult to imagine a situation where libsbp would be used where we're using std::fstream without concern but where we wouldn't want to use std::string.
| @@ -0,0 +1,9 @@ | |||
| #!/usr/bin/env bash | |||
There was a problem hiding this comment.
Do we need to be able to regenerate the binary file?
There was a problem hiding this comment.
Yes, it's helpful to have the json version (and use it to generate binary) if we ever want to change/add tests.
|
|
||
|
|
||
| TEST_F(SbpStdioTest, ReadsSbpFiles) { | ||
| EXPECT_EQ(num_entries_in_file("gnss_data.sbp"), 3); |
There was a problem hiding this comment.
It looks like there's already a test_data directory already in the root of the repo. Could we reuse one of the files there to test this?
There was a problem hiding this comment.
So I looked into reusing one of the files there. Unfortunately, both the short.sbp and long.sbp files gave me an error when I read them in. This looks like a data error, not a problem with my code, as sbp2json also gave me this problem:
(py3) laykuanloh@Lays-MBP ~/Desktop$ cat long.sbp | sbp2json
Traceback (most recent call last):
File "/Users/laykuanloh/py3/bin/sbp2json", line 6, in <module>
module_main()
File "/Users/laykuanloh/py3/lib/python3.8/site-packages/sbp/sbp2json.py", line 235, in module_main
sbp_main(args)
File "/Users/laykuanloh/py3/lib/python3.8/site-packages/sbp/sbp2json.py", line 211, in sbp_main
m = msg_nojit.SBP.unpack(b)
File "/Users/laykuanloh/py3/lib/python3.8/site-packages/sbp/msg.py", line 233, in unpack
assert p.preamble == SBP_PREAMBLE, "Invalid preamble 0x%x." % p.preamble
AssertionError: Invalid preamble 0x0.
Thus I need to keep my own data files.
There was a problem hiding this comment.
This is bizarre, the data files there work fine on my machine (and on the CI for sbp2json).
There was a problem hiding this comment.
Talked, there's a bug in Python 3.8 working with sbp2json. Filed as https://swift-nav.atlassian.net/browse/INFRA-114.
Unfortunately the files in test_data still don't work - nothing gets read in at all. At this point I think its better to keep the files I added instead, which have the added benefit of allowing user to easily make changes to the test data if they desire.
No description provided.