-
Notifications
You must be signed in to change notification settings - Fork 131
Initial multi channel trace file format specification #841
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
base: master
Are you sure you want to change the base?
Conversation
Note that this leaves out for now specifics of how to structure additional non-OSI meta-data, which should likely be handled in layered specifications based on the rdns prefixes, and any larger changes to the naming convention. The later is only a recommendation in any case, and if further changes are wanted they should apply to the whole convention and be handled in a separate PR (e.g. one could think about making the timestamp optional, or dropping the protobuf version, but that would make sense - or not - for all formats and hence is not related to the multi trace file format). |
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.
Review comments from discussion with @jdsika and @thomassedlmayer
I don't think any further review from @TimmRuppert or me is necessary here. Feel free to merge your solution whenever you are ready. |
@pmai is it possible to commit the discussed changes until 1pm and I have a final review and put the PR into ready state with the tag "readyForCCB"? |
The following rules apply to OSI multi channel trace files: | ||
|
||
- The file extension to be used is `.mcap`. | ||
- The file must be a valid MCAP file according to the https://mcap.dev/spec[MCAP format specification] version `0x30`. |
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.
@TimmRuppert I actually cannot find a version number for the MCAP specification but only different ones for the associated libraries and CLI which is then different for Python, C++, etc .... I am a bit confused. Is this the specification version 0?
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.
The spec goes about it in a very round-about way:
Magic
An MCAP file must begin and end with the following magic bytes:
0x89, M, C, A, P, 0x30, \r, \n
The byte following "MCAP" is the major version byte. 0x30 is the ASCII character 0. Any changes to this specification document (i.e. adding fields to records, introducing new records) will be binary backward-compatible within the major version.
(Emphasis by me)
So it is stated quite unclearly, but I would treat this as the major version of the specification, given the statements.
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.
That is what confuse me. "include the major version like this" but not "this spec IS major version 0" ...
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 just two points on naming convention and time definitions that were discussed during our meeting. No other objections to merging.
from internal: I have some questions about the PR:
|
Ultimately, I think it's up to the user to decide. For example, a sensor model that also outputs some non-OSI debug data could still be considered a related data stream. On the other hand, a list of my latest cookie recipes probably wouldn't be ;)
The answer is basically question 3.
Well, you don’t. It seems the documentation might leave room for misinterpretation. Perhaps it should be made more explicit - for example, by stating clearly at the beginning of the section 'The following rules apply to OSI multi-channel trace files' that non-top-level messages may only be stored as sub-elements within the channel of a top-level message. |
I think it is the responsibility of the creator of a file to ensure that related data streams are written/described in a way so that a reader of the file knows that it is indeed related and also how it is related. For the top-level OSI messages we thought about that and therefore defined rules for timestamps etc. But for data streams/channels that are not specified in this PR this is not the responsibility of the OSI specification to define. What is the goal of putting unrelated data in there?
You can still put non-top-level messages as "non-official top-level messages" into an mcap container if you desire to do so. This does not meet the OSI-mcap specification in the sense that there is a high probability that standard-conform tools are able to work with it. Though, the specification does not keep you from putting arbitrary data into your mcap containers. That's why it specifically allows to have "other related data streams" in the same mcap container. And in my opinion, if it is technically possible to pack non-top-level OSI protobuf-encoded messages into a separate mcap channel, you're free to do so, even if the relation to the rest of the OSI data might not be clear anymore (because of missing timestamp etc.). |
Based on the initial specification work with refactoring for better integration into OSI and with clearer separation of concerns. Signed-off-by: Pierre R. Mai <[email protected]>
Co-authored-by: Clemens Linnhoff <[email protected]> Signed-off-by: Pierre R. Mai <[email protected]>
Signed-off-by: Pierre R. Mai <[email protected]>
Signed-off-by: Pierre R. Mai <[email protected]>
Co-authored-by: Thomas Sedlmayer <[email protected]> Signed-off-by: Pierre R. Mai <[email protected]>
* Make zero_time and creation_time recommended Signed-off-by: Carlo van Driesten <[email protected]>
Signed-off-by: Pierre R. Mai <[email protected]>
Signed-off-by: Pierre R. Mai <[email protected]>
Based on review feedback Signed-off-by: Pierre R. Mai <[email protected]>
1bc0627
to
1d96e47
Compare
As discussed with @TimmRuppert all issues have been resolved.
Based on the initial specification work in #833 with refactoring for better integration into OSI and with clearer separation of concerns.