WIP: Expand and document lib.cdc #47
Comments
|
Codecov Report
@@ Coverage Diff @@
## master #40 +/- ##
==========================================
- Coverage 82.3% 81.34% -0.96%
==========================================
Files 34 32 -2
Lines 5565 5158 -407
Branches 1190 1105 -85
==========================================
- Hits 4580 4196 -384
+ Misses 852 829 -23
Partials 133 133
Continue to review full report at Codecov.
|
|
Last commit is just to fix a small coverage blackspot in the existing code. I was having far too much fun with the graphs on Codecov |
|
BusSynchronizer is as per Migen except:
Again it needs formal checks, and tests are devalued by running them in one clock domain. Edit: updated the commit. Previously there was a clock-enabled odomain register driven by a clock-enabled idomain register. This is ok for CDC purposes, because the CDC-safe handshake controls the clock enables, but I've also put in a (shortened) Edit: squashed in another small change: making sure that sync_stages gets passed on to the MultiReg for the width=1 special case. |
|
Add Edit: also this elastic buffer is unusual, usually there is some provision for slipping the pointers to account for long-term unbounded phase change. This would be more light-weight than an Force push due to finding small issues in earlier commits, e.g. gearbox increment didn't work for non-pow-2 chunk counts (which can't be tested in one clock domain, I just spotted it while reading the code). I've tried to keep it so that each commit can be separately checked out, tested and reviewed. If you'd rather I just push fixes, let me know :) Also, I've copied the |
|
I'll try to fix DomainRenamer to work on Modules as soon as possible!
I use the second style, with some exceptions like MultiReg. The reason is that the signals aren't always completely independent, for example you might want to pass a width and have a set of signals with that width, double that width, etc created for you. Also, constructors with a large number of arguments are unwieldy and bug-prone, especially if you change the constructor later. |
|
Ok, no sweat, I'll rebase + fix as and when :) |
|
Rebased on latest master and fixed up docstrings:
I also stared at the code for a while, only one minor fix which was something that really oughtn't be retimed, but didn't have the Domains are passed as strings, will fix this after the |
|
@Wren6991 Done! |
|
Thanks :) I've fixed up PulseSynchronizer and Gearbox so far. Unfortunately I'm away on a trip at the moment and don't have all the necessary tools set up on my work laptop, so it'll take me a while to get the fixes tested and pushed. For consistency I'm using "read" and "write" for default domain names (matching AsyncFIFO), but there is a not-totally-frivolous argument to use something like "i" and "o" instead:
Was just something that occurred to me when editing the files, it's not up to me. |
|
Now that I think more about it, I think it doesn't make sense to use There's a good argument that WDYT? |
|
So a flow could be:
If so, that feels cleaner, yeah! |
|
Yep!
Incredible. |
|
Awesome. I guess the next step for this PR is a lot more testing then. I did note #28, which I guess I could start to take a look at at some point, as this bumps against it. It would be good to learn about that part. Formal would be good too. For CDC it would also be cool to do some soak testing on real FPGAs, to catch genuine CDC bugs rather than just state machine bugs (albeit they're difficult to debug once found). Not sure if this is something you already had ideas on. |
|
The CDC primitives in oMigen are extremely well tested. I think we mainly need to make sure the nMigen ones don't stray from them, like it happened with AsyncFIFO... |
|
Ok that's a good point. Will read through the oMigen library again later today. For the most part it's pretty faithful but mistakes creep in! There are things in the old library that seemed odd though, e.g. for Gearbox with I'll definitely make sure the logic is all ok, and then maybe raise questions about things like the above. |
|
Sounds good! |
|
I did a side-by-side of oMigen vs current state of my code
BusSynchronizer datapath I drew a rough box diagram of oMigen
My problem with this is that there is a 3-flop delay for both the request and the data. (1 flop in I've fixed the race by removing one stage from the datapath |
|
@Wren6991 Sorry, I completely missed your update, GitHub decided to not send me an email for it for some reason. |
|
Do you agree on my diagram of oMigen BusSynchronizer? I might have misunderstood e.g. the PulseSynchronizer logic. Haven't looked at it for a while now. |
|
I'll need to think more about it as I wasn't the one who wrote it. @sbourdeauducq ? |
|
But then, how do you preserve the MultiReg constraints between the anti-metastabilty registers? Sounds easier to add one register of delay into the request path. |
|
@sbourdeauducq You're 100% right -- it's possible to just falsepath/MCP the connection between write data buffer and the MultiReg, but it's cleaner if it can just rely on constraints on paths internal to the MultiReg. Added a patch which restores the data path to its original length, and lengthens the request path to compensate. |
|
Fixed merge conflict and made classes derive from Elaboratable (sorry for force-push) |
|
Thanks, I'll take another look soon. |
|
That's fine, I have a lot of other work here, so you're not really blocking anything. |
|
I saw this in #113
And have also noted #87. Wanted to mention that falsepath is not necessarily safe for all of these primitives: in particular, BusSynchronizer is unsafe if there is too much skew between the request path and the datapath, and false paths can accumulate huge routing delays when FPGA utilisation is high. Since we want the widest possible toolchain support, one option is a maxdelay constraint of capture period minus setup. This isn't perfectly safe (clock skew), and does make closure harder, but might be worth considering. |
|
That seems very troublesome, and certainly something we want to loudly warn about. Would you be able to prepare a detailed evaluation of all of our CDC primitives with this in mind?
Are there any ways to work around this?
This seems like a dramatic increase in complexity of platform code. It's an option, but maybe we can avoid it completely... |
|
|
|
That is an interesting handshake, but seems to overlap quite a lot with BusSynchronizer. I have had the occasional shower thought that BusSynchronizer should have an optional valid/ready handshake in the launching domain (and perhaps a valid pulse in the capturing domain), which would give you something similar to this BlindTransfer. I'm also happy to just port BlindTransfer wholesale! |
I think we actually have maxdelay constraint now on Vivado platforms. |
PulseSynchronizer ported from issue amaranth-lang#47, which was ported from oMigen.
|
Adding more functionality to the standard library today would require going through our RFC process, so I'm closing this PR as obsolete. Thank you for your effort, @Wren6991! |
Monday Mar 04, 2019 at 21:06 GMT
Originally opened as m-labs/nmigen#40
Very much a work in progress, but I wanted to open early to get feedback on the approach/direction. I'm porting/rewriting some of the missing modules from migen.genlib.cdc.
Currently in this patch set:
Known issues:
I definitely still intend to port:
And we probably ought to do something about GrayCounter at some point, but I think it's less important than the others.
As a general style question, there seem to be two ways of doing module wiring in
nmigen.libat the moment. One is to pass external signals into__init__, which gives you more of a Verilog-style instantiation. The other is to create "port" signals in__init__, which the parent then wires up during elaboration. I've used the second style because it seems a bit cleaner, and doesn't require creating extraneous signals like in Verilog, but not sure if this is the right thing to do?Wren6991 included the following code: https://github.com/m-labs/nmigen/pull/40/commits
The text was updated successfully, but these errors were encountered: