Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
828 MCAP specification for osi tracefiles #833
base: master
Are you sure you want to change the base?
828 MCAP specification for osi tracefiles #833
Changes from 8 commits
38249ce
8261b41
beb6d47
f78eb7c
6c8a35f
7cd3ad4
ccb3bb4
f8c648f
ae05885
9603aad
2c063e9
12a3241
6c94f05
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
This file was deleted.
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.
I don't exactly understand what's meant by "unique global time". There could be multiple scenarios with the same time which means it can not be unique. Did you mean defined/specified global time or that all contained messages must be in the same time frame?
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.
In our last meeting we agreed that an mcap must only contain one scenario (while technically it could contain multiple). We decided to ditch the technical possibility to store multiple independent scenarios to avoid extreme confusion with interesting files and usage that it not intended: One could come up with the idea to store all possible NCAP scenarios at once in a huge file.
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.
I remember that we talked about a common/unified time frame but not about that an MCAP file should only be allowed to contain a single scenario.
Also, I thought that the term "unique global time" is maybe a bit confusing as well. I'd rather write something like common time frame.
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 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.
Still, I don't agree that an mcap file must contain only a single scenario. I think the word "scenario" could be misleading. You can put an arbitrary number of scenarios in one trace file. IMO the only relevant information here is that all the messages in the file must have a common time frame.
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.
I agree that the term scenario is a bit misleading. You could have multiple scenarios one after another in one simulation. The important thing is, that all channels have the same time line.
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 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.
Is it defined somewhere what "full message type name" means exactly?
Does it make sense to specify that the channels must be named "osi3.MessageType"?
Especially, because you used "OSI3::SensorData" in line 60 (not in the same context but anyways).
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.
I understand the protobuf documentation as if this is defined. But to be more precise I will change it to
fully-qualified name of the protobuf message type
There is a mix usage of "must" and leaving it out. Should be changed.
There I meant the osi3::SensorData Struct. But it makes sense to simply write "A channel containing OSI
SensorData
messages" and circumvent this. I will change line 60There 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.
Shouldn't this be removed here?
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.
Can you please elaborate further?I see the issue.
log_time
is defined twiceThere 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.
I think there should be some convention or at least one sentence talking about the used separator character for the prefix.
I see that you used "-" as a separator in the examples. I feel like this is quite confusing (especially when reading this document) because to me it isn't exactly clear, what's part of the prefix name and what's the actual key when there are multiple "-" in the key or (e.g. "GAIA-X4PLC-AAD-hdmap-actual-key"). Something like a "." would probably make it more obvious what the actual prefix is.
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.
I do not have any opinion about this. @jdsika had the idea to add prefixes, so lets see what he suggests.
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.
I would propose using unambiguous words for mcap-related stuff: Use "metadata record" instead of "category" or "metadata" or "file metadata" when referring to a metadata record; use "metadata record name" instead of "metadata name" or "category name"; use "channel metadata" for channel metadata.
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.
Specification of timezone is missing.
Why not use nanoseconds (unix epoch)?
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.
I would personally also favor the unix epoch in ms or nanoseconds. The proposal is using ISO 8601 as it seems like that was used for trace file names etc. before.
That would also make more sense to be compliant with https://opensimulationinterface.github.io/osi-antora-generator/asamosi/latest/gen/structosi3_1_1EnvironmentalConditions.html#a636bb78627046f34208f42f586ab2086?
Lets wait an see if anyone disagrees.
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.
Should there be two "asam_osi" metadata records (see two lines above) or should all the metadata fields be in one metadata record?
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.
While I don't know if it is technically possible to have two metadata records with the same name (I assume it might be), it would make more sense to have all of that in one record. Considering your proposal to always speak of "a metadata record" in this context (see other comment) it should be clearer once this has been added.
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.
Maybe we should state that the timestamp can only exceptionally be omitted when there really is no reference to a global time in the file?
I think if you have a real-world capture or any other trace file that has any meaningful relation to a global time frame, it should be visible in the filename.
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.
In my opinion the meaningful relation to a global time is the exception here, since simulation should be the "normal" use-case rather than measurements.
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 actual timestamp format (including timezone information) should be specified. Probably the same as in OSI file naming convention.
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.
To my limited understanding of the ISO 8601, the .osi and .txth spec is not stating a format. I just provides the example
20210818T150542Z
and refers to ISO 8601Something like
YYYYMMDDThhmmssZ
would be a valid format with respect to ISO 8601 butYYYY-MM-DD-HH
as well right right?I would like to add the format
YYYYMMDDThhmmssZ
and the mention that it must be in UTC (not local, due to the Z) to the .osi/.txth trace file naming convention but this assumes that generalizing the example is not considered a breaking change.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 naming convention even states that the recommended format is YYYMMDDThhmmssZ (even though with exemplary numbers). I would just add the format specification and maybe even consider decimal places. The recommended format remains the same as far as I'm concerned.
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 information of this line got lost.