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
AsyncFIFOBuffered {r,w}_level fixes #571
AsyncFIFOBuffered {r,w}_level fixes #571
Conversation
…hronizer AsyncFFsynchronizer only synchronizes one edge
Codecov Report
@@ Coverage Diff @@
## master #571 +/- ##
=======================================
Coverage 81.50% 81.50%
=======================================
Files 49 49
Lines 6461 6461
Branches 1287 1287
=======================================
Hits 5266 5266
Misses 1008 1008
Partials 187 187
Continue to review full report at Codecov.
|
Everything looks good. However, does anyone remember how we ended using |
To me it happens quite often that I end up using AsyncFFSyncronizer by accident because of the naming. My thought process is something like "I need to synchronize a signal that is async to $domain into $domain. Must be AsyncFFSyncronizer". I think it is very likely that it slipped in there similarly? |
@awygle mentioned on IRC that he thinks it's deliberate; let's wait for him to investigate. |
I misread this because of what @whitequark said about it motivating a refactoring. This is a different case than what I was thinking of (the reset |
Thanks everyone! |
This includes a number of fixes for
{r,w}_level
in AsyncFIFOBuffered.AsyncFFSynchronizer
instead ofFFSynchronizer
. This is wrong as we want to synchronize both edgesconsume_w_bin
/w_level
. Two cycles for theFFSynchronizer
from the read to the write domain ofconsume_w_gry
, one for theGrayDecoder
and one for thew_level
calculation.r_en
even ifr_rdy
is low, we get-1
in the intermediate calculation which overflows to1
as we have a unsignedSignal
Let me know if I should split this into multiple PRs or elaborate more on the fixes. The last two include additional testcases that fail without the fix. I did not include a testcase for the
AsyncFFSynchronizer
toFFSynchronizer
change, as this is obviously wrong and would be hard to properly test without proper multi clock support in formal.