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

Jump to bottom

{r,w}_level is broken on AsyncFIFO{Buffered,} #485

Closed
anuejn opened this issue Aug 23, 2020 · 4 comments · Fixed by #512
Closed

{r,w}_level is broken on AsyncFIFO{Buffered,} #485

anuejn opened this issue Aug 23, 2020 · 4 comments · Fixed by #512
Assignees
@anuejn
Labels
bug
Milestone
0.3

Comments

@anuejn
Copy link
Contributor

anuejn commented Aug 23, 2020

It seems like the r_level / w_level on AsyncFIFO / AsyncFIFOBufered are broken in multiple ways:

  • If the FIFO is full the level indicators become 0
  • The semantics of w_level of AsyncFIFOBuffered are quite unintuitive since it cannot reach depth and is quite different from r_level / w_level of AsyncFIFO.

@awygle started to work on a fix for this and I picked up his work and added a simulation-based test case (master...anuejn:fifo_bugfix).
However, it is unclear to me, how one would implement the same behavior of w_level for AsyncFIFOBuffered as for AsyncFIFO without violating CDC rules. Maybe anyone has an idea here? Am I wrong here?
@whitequark whitequark added the bug label Aug 23, 2020
@whitequark whitequark added this to the 0.3 milestone Aug 23, 2020
@awygle
Copy link
Contributor

awygle commented Aug 23, 2020

Yeah I goofed this up pretty badly (we either need a lot more simulation test cases for AsyncFIFO or we need to implement #387...). Thanks for the test case! I'll pick this back up ASAP, just kinda swamped at the moment.

@whitequark
Copy link
Member

However, it is unclear to me, how one would implement the same behavior of w_level for AsyncFIFOBuffered as for AsyncFIFO without violating CDC rules.

Nominating for today's meeting because of this.

@whitequark
Copy link
Member

Per IRC discussion, @anuejn may be able to fix this bug as they already have a partial fix for r_level. The fix for w_level would involve resynchronizing the state of the output buffer (which has the same CDC latency as the Gray counters themselves) and then doing the same thing as for r_level.

@anuejn anuejn mentioned this issue Oct 22, 2020
Merged
@whitequark
Copy link
Member

Partially fixed by #513.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@anuejn anuejn

Labels
bug
Milestone
0.3
Development

Successfully merging a pull request may close this issue.

Fix {r,w}_level in AsyncFIFOBuffered anuejn/nmigen
3 participants
@whitequark @awygle @anuejn