-
Notifications
You must be signed in to change notification settings - Fork 262
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
Configurable simulation time resolution in logging #1044
Comments
Let's say 123450 ps is to be displayed in ns with three decimals. Is 123.450 ns or 123.45 ns preferred? I.e. is the number of configured decimals the exact number or the maximum number? I'm leaning towards 123.45 ns because if no number of decimals is provided, I'd like the default behavior to be "as many decimals as needed". That aligns with the behavior of It also aligns with Python rounding: >>> round(123.450, 3)
123.45 |
I can't think of any other case where the configured three decimals are the exact number apart from the case where you want to print a lot of timestamps and you would like that all numbers are vertically aligned with the same number of decimals:
But I don't think this is critical and everybody's OCD can live with it 😄 |
@tasgomes This is what working state looks like ("auto unit" mode with three decimals). It wouldn't be difficult to add an extra space to 1.02 ps to get better alignment. |
Another issue is the fact that VHDL time is an integer number,
|
@LarsAsplund It does look weird with extra spacing, therefore I think the fixed number approach would be more suitable for this.
|
I agree. Fixed number of decimals and truncate to that width. If someone comes up with a good reason for why we need to round, we can always reconsider. |
@tasgomes I pushed the code for the new time format configuration. Here is the upcoming documentation. Sounds good? If so, I will merge: |
Hi @LarsAsplund, the documentation sounds sane to me. I took the opportunity to test this quickly. I used commit 48bcaa5. Is this the most updated one? I could not find most of the definitions mentioned in the documentation like It is like the default time unit is |
No, you need to checkout 229a688 |
hum, I guess you were doing some rebasing and that commit 48bcaa5 got lost / not tracked anymore. This somehow broke our Gitlab mirroring of the VUnit repository, that is why I did not find the latest sources like 229a688. I fixed that and now our mirrored Basically, when you use the time unit Do you see the same? If I comment all lines that use |
I only tested with `ps` on Riviera-PRO and then there is no problem. Does
Questa return time as`fs` when calling `time'image` despite a resolution of
`ps`? Or does it complain about that return statement despite not being
executed? The reason I have my own resolution_limit function is to maintain
our VHDL-93 support for logging
…On Fri, 9 Aug 2024, 15:59 Tiago Gomes, ***@***.***> wrote:
hum, I guess you were doing some rebasing and that commit 48bcaa5
<48bcaa5>
got lost / not tracked anymore. This somehow broke our Gitlab mirroring of
the VUnit repository, that is why I did not find the latest sources like
229a688
<229a688>.
I fixed that and now our mirrored sim_time_format branch matches the
Github sim_time_branch.
Basically, when you use the time unit fs in the log_handler_pkg-body.vhd
like here
<https://github.com/VUnit/vunit/blob/sim_time_format/vunit/vhdl/logging/src/log_handler_pkg-body.vhd#L13>,
then Modelsim/Questa will throw an annoying warning at the beginning of the
simulation, if the simulator time resolution is set to ps.
image.png (view on web)
<https://github.com/user-attachments/assets/71447477-c589-4b17-a61f-74af8c21ac19>
Do you see the same?
If I comment all lines that use fs, then the warning disappears.
—
Reply to this email directly, view it on GitHub
<#1044 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABS7IWLEVXXFXDR6APBQCNTZQTDMHAVCNFSM6AAAAABLGWOZZGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENZYGAYTCNRSHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
@tasgomes I tested with Questa and saw the same issue so I pushed a new commit that "hides" the finer time units. See if it works for you as well. |
Thanks @LarsAsplund, latest commit removed the warning. |
Support configuration of the log entry simulation time with respect to:
fs
,ps
,ns
, and so on)ns
, and the number of decimals is set to 2, the logged time should be 123.46 nsIf the unit parameter is of type
time
, we can create negative time constants with special meaning. For example, a constantauto
that uses the unit that keeps the integer part of the time within the 0 - 999 range.The text was updated successfully, but these errors were encountered: