open-simulation-interface icon indicating copy to clipboard operation
open-simulation-interface copied to clipboard

SensorViewConfiguration missing timestamp

Open caspar-ai opened this issue 5 years ago • 4 comments

Describe the bug

There is a disconnect between the documentation and the implementation with regards Top-Level Messages and SensorViewConfiguration.

Describe how to reproduce the bug

The interface conventions documentation states that all top level messages require an interface as the first field and a timestamp as the second. It also explicitly mentions SensorViewConfiguration as one such example of a top level message.

The definition of a SensorViewConfiguration includes an interface version but does not include a timestamp.

Describe the expected behavior

The expected fix depends on the desired behaviour:

  1. If the SensorViewConfiguration is an exception to the top level rule then I would expect the documentation to be updated, and a comment added inline by the message definition.
  2. If it isn't an exception, a timestamp field should be added.

caspar-ai avatar Jun 17 '20 08:06 caspar-ai

@caspar-ai Hi Casper, while going through the SensorViewConfiguration definition, I could find the simulation start time at line 191. Isn't it what you are expecting?

Krishna626 avatar Aug 19 '20 19:08 Krishna626

@Krishna626 I'm not an expert in this area, so I'm not 100% sure. On first read though it doesn't look like it to me.

In particular, compare to, for example, the ground truth definition which defines the timestamp as the current time. Whereas the field you point to on line 191 defines it as the start time of the simulation, i.e. that field would never change throughout the lifetime of the exchange. At least that's how I read it.

caspar-ai avatar Aug 20 '20 08:08 caspar-ai

As this issue was brought up by @PhRosenberger in #764 I tried to get some insight on the problem.

It seems like the part on mandatory 'version' and 'timestamp' fields in top level messages was removed from the documentation at some point. I could imagine that it isn't too important for SensorViewConfiguration to include a timestamp since it is either sent prior to simulation start (during initialization) or contained in another top level interface message which already includes a timestamp.

I further noticed that FeatureData is listed under the chapter "Top-level interfaces" in the documentation while not containing a timestamp as well. Though, each SensorDetectionHeader already contains some information on the measurement time of the data instead of having a top level timestamp for all messages together.

thomassedlmayer avatar Jan 23 '24 13:01 thomassedlmayer

Issue was discussed in today's Packaging/Performance meeting:

  • Discussion: Could make sense to add the timestamp based on the following use case: If multiple configs are sent after simulation start, a timestamp would help to identify the order in case the messages get mixed up. But no real use case/problem for anyone right now.
  • No further actions for now.

thomassedlmayer avatar Mar 07 '24 10:03 thomassedlmayer