Skip to content
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

Create a unified timestamp format #265

Merged
merged 5 commits into from
Feb 24, 2023
Merged

Conversation

chykon
Copy link
Contributor

@chykon chykon commented Feb 11, 2023

Description & Motivation

SystemVerilog and VCD will now use ISO 8601 UTC time when generating. Previously, local time was used, which could leak time zone information.


RENAMED: Use UTC and ISO 8601 for output -> Create a unified timestamp format

UPDATE: During the discussions, @mkorbel1 pointed out the need to preserve the human-readability of the format. The decision was made to leave the display of local time and add a UTC offset to it, which will correctly process timestamps from data coming from different time zones. Suggested timestamp format to use when generating SystemVerilog and VCD: YYYY-MM-DD hh:mm:ss.sss [+/-]hh:mm.

Related Issue(s)

No.

Testing

Ran tool/run_checks.sh, waited for successful completion. Ran example/example.dart, checked the output.


UPDATE: Added timestamp format test.

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

No.


UPDATE: Potentially yes. If someone was tied to the old time format, then he may suffer from a format change.

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

No.


UPDATE: Yes. Documentation for the new utility class Timestamper. Included in PR.

@mkorbel1
Copy link
Contributor

Previously, local time was used, which could leak time zone information.

When you say "leak", do you mean from a privacy/security perspective that sharing a file which was generated using ROHD could expose information about the person who generated it?

From a debuggability perspective, it seems convenient to not have to do timezone math to determine when you generated a file.

Is this a typical practice to always use UTC for generated timestamps? Please excuse my ignorance on the topic.

@chykon
Copy link
Contributor Author

chykon commented Feb 13, 2023

When you say "leak", do you mean from a privacy/security perspective that sharing a file which was generated using ROHD could expose information about the person who generated it?

Yes... although this may sound overly dramatic in the context of ROHD.

From a debuggability perspective, it seems convenient to not have to do timezone math to determine when you generated a file.

Yes, I mentioned the leak, but did not point out the advantages of a single point of reference for processing files. Oops.

Is this a typical practice to always use UTC for generated timestamps? Please excuse my ignorance on the topic.

It's hard to tell if this is a typical practice, but current timestamps use local time and don't preserve the time zone. If the data crosses time zones, then problems can arise in certain use cases. For example, someone sorts vcds received from all over the world by the $date field, which makes the sorting, with a high probability, actually wrong. Why he does this is another question.

@chykon
Copy link
Contributor Author

chykon commented Feb 14, 2023

It is possible to try to maintain both human convenience and processing accuracy. How about converting timestamps to the form local_time+utc_offset? For example, 1997-07-16T19:20:30.45+01:00.

@mkorbel1
Copy link
Contributor

How about converting timestamps to the form local_time+utc_offset?

I think that's a great compromise, good idea!

@chykon chykon changed the title Use UTC and ISO 8601 for output Create a unified timestamp format Feb 18, 2023
@chykon
Copy link
Contributor Author

chykon commented Feb 18, 2023

Example:

  • vcd
$date
  2023-02-18 17:19:12.260 +03:00
$end
  • sv
/**
 * Generated by ROHD - www.github.com/intel/rohd
 * Generation time: 2023-02-18 17:19:12.150 +03:00
 * ROHD Version: 0.4.2
 */

Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you!

@mkorbel1 mkorbel1 merged commit 2e96f28 into intel:main Feb 24, 2023
@chykon chykon deleted the use-utc-for-output branch February 24, 2023 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants