-
Notifications
You must be signed in to change notification settings - Fork 1
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
Project structure and c++ trace file reader/writer #2
Conversation
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
…permissions Signed-off-by: Timm Ruppert <[email protected]>
rename "metadata_glossary" to "context". We use a familiar term right away and "we are actually giving context for the definitions" |
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
I would suggest to only support a .txth writer and not a reader due to the discussion of the format being "lossy" |
I already implemented a txth reader which seem to work very reliable by simply assuming the first line in the file will be the separator between messages. If you'd like, I can certainly remove it again! However, my recommendation is to keep it, as it is designed to be quite robust (as good as possible for a format which is not fully specified) and aligns with the de facto standardization caused by Adding a txth writer should be quite easy. |
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
…een required and optional metadata Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
…ests Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
Where can I see which MCAP repo tag/version is pinned for the submodule? |
// Add optional metadata to the net.asam.osi.trace metadata record, as recommended by the OSI specification. | ||
net_asam_osi_trace_metadata.metadata["description"] = "Example mcap trace file created with the ASAM OSI utilities library."; // optional field | ||
net_asam_osi_trace_metadata.metadata["creation_time"] = osi3::MCAPTraceFileWriter::GetCurrentTimeAsString(); // optional field | ||
net_asam_osi_trace_metadata.metadata["authors"] = "Jane Doe, John Doe"; // optional field |
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.
Where is the rest of the mandatory metadata written/defined?
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.
Does the example fail? src/tracefile/writer/MCAPTraceFileWriter.cpp line 119 requires it?
constexpr std::array<const char*, 5> kRequiredFields = {"version", "min_osi_version", "max_osi_version", "min_protobuf_version", "max_protobuf_version"};
for (const auto& field : kRequiredFields) {
if (metadata.metadata.find(field) == metadata.metadata.end()) {
std::cerr << "ERROR: cannot add net.asam.osi.trac meta-data record without a " << field << " field.\n";
return false;
}
}
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.
Here it gets created and here the mandatory and optional metadata are written.
The convenience function osi3::MCAPTraceFileWriter::PrepareRequiredFileMetadata();
provides the required (by the current state of the new OSI spec) metadata. PrepareRequiredFileMetadata()
is obviously not useful for merging mcap files etc. as the min and max values of osi/protobuf version metadata will all get the same value (therefore a convenience function and not part of setting any metadata automatically).
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.
Does the example fail? src/tracefile/writer/MCAPTraceFileWriter.cpp line 119 requires it?
No because it uses osi3::MCAPTraceFileWriter::PrepareRequiredFileMetadata()
$ git rev-parse HEAD
0df13a4432946a5a08f13f386e62d56c5f642e0f
$ git status
On branch 1-add-a-c++-trace-file-readerwriter
Your branch is up to date with 'origin/1-add-a-c++-trace-file-readerwriter'.
nothing to commit, working tree clean
$ ./cmake-build-debug/examples/example_mcap_writer
Starting MCAP Writer example:
Creating trace_file at "/tmp/sv_example.mcap"
Finished MCAP Writer example
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.
ah ok, but then this makes no sense metadata.metadata["version"] = osi_version_string;
because it is the version of the spec (we don't have yet but we would anticipate 3.8.0 which should include the multi tracefile spec.
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 should (after merge and release) be the osi version which is mentioned in the CMAKE
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.
ah ok, but then this makes no sense metadata.metadata["version"] = osi_version_string; because it is the version of the spec (we don't have yet but we would anticipate 3.8.0 which should include the multi tracefile spec.
Ah you mean this should be hard-coded? Yes this makes sense.
ah ok, but then this makes no sense metadata.metadata["version"] = osi_version_string; because it is the version of the spec (we don't have yet but we would anticipate 3.8.0 which should include the multi tracefile spec.
In the cmake you can specify what ever osi you want but that is not related with the spec we are trying to fulfill here right? Updating the osi you build against does not automatically fix potential osi mcap schema issue which need to be fixed in the library itself.
we would anticipate 3.8.0
The assigned milestone is called 3.7.1
This should (after merge and release)
Which merge and release? OSI muli trace file spec or this PR of asam-osi-utilities? I can imagine that asam-osi-utilities releases could be made quite complicated: multiple asam-osi-utilities versions for multiple osi versions. It might make sense to simply target the newest OSI version and always provide independent improvements and fixes - but that is a discussion for another day.
To sum it up: If you do not disagree I would simply hardcode this to 3.7.0-a63f47d7b21bb44edafca176700d9cedf6b12d81
which represents the current values of Pieres last commit (the VERSION
file in that branch + commit).
To my knowledge submodules always point to a commit. If you checkout the tag (like I did) it will be in detached HEAD mode pointing to the commit the tag points to. Locally you can still see the tag $ cd asam-osi-utilities/lib/mcap
asam-osi-utilities/lib/mcap$ git status
HEAD detached at releases/cpp/v1.4.1
nothing to commit, working tree clean
|
Signed-off-by: jdsika <[email protected]>
Signed-off-by: jdsika <[email protected]>
Signed-off-by: jdsika <[email protected]>
Signed-off-by: jdsika <[email protected]>
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.
Just add the last changes we discussed via teams and it is good to merge
Signed-off-by: Timm Ruppert <[email protected]>
…tblick-Suite/asam-osi-utilities into 1-add-a-c++-trace-file-readerwriter
Signed-off-by: Timm Ruppert <[email protected]>
Signed-off-by: Timm Ruppert <[email protected]>
I added more options to the converter as requested --chunk_size <size> Optional: Chunk size in bytes (default: 786432)
--compression <type> Optional: Compression type (none, lz4, zstd) (default: zstd)
--compression_level <type> Optional: Compression level (fastest, fast, default, slow, slowest) (default: default)
I also did some quick tests:
-rw-rw-r-- 1 timm timm 61M Dec 2 15:07 /tmp/test_200000000_lz4_default.mcap
-rw-rw-r-- 1 timm timm 80M Dec 2 15:06 /tmp/test_200000000_lz4_fastest.mcap
-rw-rw-r-- 1 timm timm 61M Dec 2 15:07 /tmp/test_20000000_lz4_default.mcap
-rw-rw-r-- 1 timm timm 80M Dec 2 15:05 /tmp/test_20000000_lz4_fastest.mcap
-rw-rw-r-- 1 timm timm 80M Dec 2 15:05 /tmp/test_2000000_lz4_fastest.mcap
-rw-rw-r-- 1 timm timm 80M Dec 2 15:04 /tmp/test_200000_lz4_fastest.mcap
-rw-rw-r-- 1 timm timm 80M Dec 2 15:04 /tmp/test_20000_lz4_fastest.mcap
-rw-rw-r-- 1 timm timm 80M Dec 2 15:04 /tmp/test_2000_lz4_fastest.mcap
-rw-rw-r-- 1 timm timm 61M Dec 2 15:04 /tmp/test_200_lz4_default.mcap
-rw-rw-r-- 1 timm timm 80M Dec 2 15:04 /tmp/test_200_lz4_fastest.mcap
-rw-rw-r-- 1 timm timm 80M Dec 2 15:03 /tmp/test_20_lz4_fastest.mcap
-rw-rw-r-- 1 timm timm 61M Dec 2 15:20 /tmp/test_2_lz4_default.mcap
-rw-rw-r-- 1 timm timm 80M Dec 2 15:03 /tmp/test_2_lz4_fastest.mcap
So the chunk size does not seem to have an effect 🤷♂️ but it is applied. Check the following 2 vs. 11 vs. 66 chunks. The file had 66 messages. $mcap info /tmp/test_200000000_lz4_fastest.mcap
library: libmcap 1.4.1
profile: osi2mcap
messages: 66
duration: 6.499999999s
start: 0.100000000
end: 6.599999999
compression:
lz4: [2/2 chunks] [209.92 MiB/79.75 MiB (62.01%)] [12.27 MiB/sec]
channels:
(1) ConvertedTrace 66 msgs (10.15 Hz) : osi3.SensorData [protobuf]
channels: 1
attachments: 0
metadata: 1
$ mcap info /tmp/test_20000000_lz4_fastest.mcap
library: libmcap 1.4.1
profile: osi2mcap
messages: 66
duration: 6.499999999s
start: 0.100000000
end: 6.599999999
compression:
lz4: [11/11 chunks] [209.92 MiB/79.75 MiB (62.01%)] [12.27 MiB/sec]
channels:
(1) ConvertedTrace 66 msgs (10.15 Hz) : osi3.SensorData [protobuf]
channels: 1
attachments: 0
metadata: 1
$ mcap info /tmp/test_2000000_lz4_fastest.mcap
library: libmcap 1.4.1
profile: osi2mcap
messages: 66
duration: 6.499999999s
start: 0.100000000
end: 6.599999999
compression:
lz4: [66/66 chunks] [209.92 MiB/79.75 MiB (62.01%)] [12.27 MiB/sec]
channels:
(1) ConvertedTrace 66 msgs (10.15 Hz) : osi3.SensorData [protobuf]
channels: 1
attachments: 0
metadata: 1
|
Description
I left out further documentation/tutorials besides doxygen documentation and multiple example applications. The PR already contains a lot more than "just adding a reader/writer". I recommend that decisions about how to host, design, or set up additional tutorials and documentation be made outside of this PR. This also applies to contribution guide lines etc.
What is missing to get out of Draft
Closes #1