Generalize period<->frequency conversion, use #2436
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
DigitalBrains1
force-pushed
the
hztoperioderr
branch
from
6bb85e2
to
28bcb9e
Compare
DigitalBrains1
force-pushed
the
hztoperioderr
branch
from
28bcb9e
to
dfccd00
Compare
DigitalBrains1
force-pushed
the
hztoperioderr
branch
from
dfccd00
to
befe5f5
Compare
DigitalBrains1
force-pushed
the
hztoperioderr
branch
2 times, most recently
from
897631f
to
4c45606
Compare
martijnbastiaan
approved these changes
Mar 11, 2023
`periodToHz`, `hzToPeriod`, `fsToHz` and `hzToFs` can be generalized so they can be more easily used in cases where we don't want `Natural` and `Ratio Natural` as the types. However, the common use of `vPeriod=hzToPeriod 33e6` means we want the argument to `hzToPeriod` to be monomorphic. Making it polymorphic means it defaults to the inferior `Double` and warns about this defaulting. So we merely generalize the return types of these functions. These functions used to throw exceptions without call stacks when called with zero. By changing that to `ErrorCall` we can get a stack trace, solving an immense frustration in debugging a Clash design. Currently, the frequency calculation in for the Intel PLLs depends on the unit of the period of a `KnownDomain` (picoseconds). If this unit is to be changed internally in the future (to femtoseconds), this calculation would produce the wrong frequency. By using `periodToHz`, the unit of the period no longer matters and can be changed internally. This use case is what prompted this PR. Harmonized and slightly improved some documentation.
DigitalBrains1
force-pushed
the
hztoperioderr
branch
from
4c45606
to
30d9b19
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
periodToHz,hzToPeriod,fsToHzandhzToFscan be generalized sothey can be more easily used in cases where we don't want
NaturalandRatio Naturalas the types.However, the common use of
vPeriod=hzToPeriod 33e6means we want theargument to
hzToPeriodto be monomorphic. Making it polymorphic meansit defaults to the inferior
Doubleand warns about this defaulting. Sowe merely generalize the return types of these functions.
These functions used to throw exceptions without call stacks when called
with zero. By changing that to
ErrorCallwe can get a stack trace,solving an immense frustration in debugging a Clash design.
Currently, the frequency calculation in for the Intel PLLs depends on
the unit of the period of a
KnownDomain(picoseconds). If this unit isto be changed internally in the future (to femtoseconds), this
calculation would produce the wrong frequency. By using
periodToHz,the unit of the period no longer matters and can be changed internally.
This use case is what prompted this PR.
Harmonized and slightly improved some documentation.
Still TODO:
(part of this commit message pilfered from a message written by Hidde)