-
Notifications
You must be signed in to change notification settings - Fork 128
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
Initial multi channel trace file format specification #841
base: master
Are you sure you want to change the base?
Conversation
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]>
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). |
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]>
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"? |
Co-authored-by: Thomas Sedlmayer <[email protected]> Signed-off-by: Pierre R. Mai <[email protected]>
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" ...
* Make zero_time and creation_time recommended Signed-off-by: Carlo van Driesten <[email protected]>
Signed-off-by: Pierre R. Mai <[email protected]>
Based on the initial specification work in #833 with refactoring for better integration into OSI and with clearer separation of concerns.