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

RFC #66: Simulation time #66

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

zyp
Copy link
Contributor

@zyp zyp commented Apr 16, 2024

zyp added a commit to zyp/amaranth-rfcs that referenced this pull request Apr 16, 2024
@whitequark whitequark added the area:core RFC affecting APIs in amaranth-lang/amaranth label Apr 16, 2024
Copy link

@lekcyjna123 lekcyjna123 left a comment

Choose a reason for hiding this comment

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

For me it looks good. I left some minor bikeshedding comments.

- `.GHz(frequency: numbers.Real)`
- The argument will be scaled according to the SI prefix on the method and the `duration` or reciprocal of `frequency` then used to calculate the closest integer femtosecond representation.

To convert it back to a number, the following properties are available:

Choose a reason for hiding this comment

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

Can we provide a convenience wrappers over time getters, to request the period rounded to int? If we specify time, then usually we know which unit is needed to get the exact int value. In such cases there will be always needed explicit cast which will produce boilerplate.

So I would like to propose to add:

  • .iseconds -> int
  • .imiliseconds -> int
  • and so on

(of course the above function names are some example, which should probably by changed for something better)

Copy link
Member

Choose a reason for hiding this comment

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

I don't see that as valuable due to how rare reading simulation time is expected to be.

[guide-level-explanation]: #guide-level-explanation

A new type `amaranth.sim.Period` is introduced.
It is immutable, has no public constructor and is instead constructed through the following classmethods:

Choose a reason for hiding this comment

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

I am not sure if classmethod constructors should have such short names and the time getters the longer one. Usually the construction is more seldom than accessing. So maybe we can have constructors something like .make_s(duration), .make_ms(duration)... and the access functions as .get_s(), .get_ms(),...

Copy link
Member

Choose a reason for hiding this comment

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

Not in this case though. You will almost never use accessors. You should keep that in mind when evaluating the API design.

@whitequark
Copy link
Member

Something that I realized is that this class would also be really convenient for specifying frequencies of clocks.

amaranth.time?

## Prior art
[prior-art]: #prior-art

None.
Copy link
Member

Choose a reason for hiding this comment

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

I think CXXRTL using time as an 128-bit number of femtoseconds is prior art.

@whitequark whitequark added the meta:nominated Nominated for discussion on the next relevant meeting label May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:core RFC affecting APIs in amaranth-lang/amaranth meta:nominated Nominated for discussion on the next relevant meeting
3 participants