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

Make three reset glitch filter variants #2544

Merged
merged 1 commit into from
Aug 2, 2023
Merged

Conversation

DigitalBrains1
Copy link
Member

@DigitalBrains1 DigitalBrains1 commented Jul 14, 2023

The existing code for resetGlitchFilter was wrong: the error on domains with unknown initial values rendered as an X value in HDL instead of throwing an error.

The new unsafeResetGlitchFilter allows to sidestep the restriction if the user is sure this works for them.

The new resetGlitchFilterWithReset is the better solution for domains with unknown initial values, relying on an additional power-on reset for which assertion glitches don't occur.

Both resetSynchronizer and resetGlitchFilter no longer carry a NOINLINE/OPAQUE annotation. The only reason they had one was for nicer port/signal names, there was no technical reason to put them in a separate HDL file. Furthermore, somehow the names for resetSynchronizer were not nice anyway, it would need -fno-do-lambda-eta-expansion to work correctly.

Still TODO:

@DigitalBrains1
Copy link
Member Author

DigitalBrains1 commented Jul 14, 2023

Review comments on the user documentation are particularly welcome.

There's a circumstance I thought of that I found no good way to put into the documentation. In unsafeResetGlitchFilter, about the period when it is still unasserted. I don't know how realistic it is, but during power-on the clock might not have stabilised. It might violate timing requirements if it is pulsing faster than the stable clock rate, which might mess up the Mealy machine in the reset glitch filter, potentially increasing the number of clock cycles spent unasserted. Then again, once the clock is stable, the amount of cycles will not exceed 2 * glitchlessPeriod. Perhaps it is understood that you don't start counting until the clock is stable. Or I'm just thinking too much about it.

Finally, this PR should probably wait for the discussed changes in #2539. That's the only reason why it is Draft.

@DigitalBrains1 DigitalBrains1 added this to the 1.8 milestone Jul 14, 2023
@DigitalBrains1
Copy link
Member Author

DigitalBrains1 commented Jul 14, 2023

The build failure due to a dependency loop will be solved by adding the constraint from #2539, which gets rid of clashCompileError in resetGlitchFilter, which is responsible for the dependency loop.

[edit]
I temporarily changed the clashCompileError to error so we can look at, for example, the generated Haddock. This term-level treatment will be replaced by a type-level treatment introduced in #2539 anyway.
[/edit]

clash-prelude/src/Clash/Explicit/Reset.hs Outdated Show resolved Hide resolved
clash-prelude/src/Clash/Explicit/Reset.hs Show resolved Hide resolved
clash-prelude/src/Clash/Explicit/Reset.hs Outdated Show resolved Hide resolved
@DigitalBrains1 DigitalBrains1 force-pushed the safeglitch branch 3 times, most recently from bea4e3e to 6ce1254 Compare July 27, 2023 09:55
@DigitalBrains1 DigitalBrains1 marked this pull request as ready for review August 2, 2023 15:19
The existing code for `resetGlitchFilter` was wrong: the `error` on
domains with unknown initial values rendered as an X value in HDL
instead of throwing an error.

The new `unsafeResetGlitchFilter` allows to sidestep the restriction if
the user is sure this works for them.

The new `resetGlitchFilterWithReset` is the better solution for domains
with unknown initial values, relying on an additional power-on reset
for which assertion glitches don't occur.

Both `resetSynchronizer` and `resetGlitchFilter` no longer carry a
`NOINLINE`/`OPAQUE` annotation. The only reason they had one was for
nicer port/signal names, there was no technical reason to put them in a
separate HDL file. Furthermore, somehow the names for
`resetSynchronizer` were not nice anyway, it would need
`-fno-do-lambda-eta-expansion` to work correctly.
@DigitalBrains1 DigitalBrains1 merged commit 3234dac into master Aug 2, 2023
12 checks passed
@DigitalBrains1 DigitalBrains1 deleted the safeglitch branch August 2, 2023 19: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.

3 participants