Conversation
NOTE: will not pass CI
hiddemoll
left a comment
hiddemoll
left a comment
There was a problem hiding this comment.
I find this PR very hard to review in the current state because circuits are being constructed and deconstructed multile times. I've left some minor comments, but I've also asked @martijnbastiaan to take a look.
| -} | ||
| xilinxElasticBuffer :: | ||
| forall n readDom writeDom a. | ||
| elasticBufferAutoCenter :: |
There was a problem hiding this comment.
Why is this not directly part of elasticBufferControlWb?
There was a problem hiding this comment.
That's something @lmbollen and I discussed, and we ended up deciding to split them. I can't remember the reasoning at the moment - maybe it was that in the diagram the autocentering is split from the control?
| Circuit go | ||
| where | ||
| go (wbFwd, (dcFifoOut, _)) = | ||
| let | ||
| isFirstRising :: Signal readDom Bool -> Signal readDom Bool | ||
| isFirstRising = E.isRising clkRead flagsReset enableGen False . stickyE clkRead flagsReset | ||
|
|
||
| flagsReset :: Reset readDom | ||
| flagsReset = E.orReset rstRead (unsafeFromActiveHigh (clearStatusRegisters .== Just (BusWrite True))) | ||
|
|
||
| localCounterUnderflow <- shutter (isFirstRising underflow) -< Fwd localCounter | ||
| localCounterOverflow <- shutter (isFirstRising overflow1) -< Fwd localCounter | ||
| fn = toSignals goC | ||
| (wbBwd, dcFifoIn) = fn (wbFwd, dcFifoOut) | ||
| in | ||
| (wbBwd, (dcFifoIn, relDataCount)) |
There was a problem hiding this comment.
I'm getting really confused by the mixture of circuit notation and non-circuit notation.
There was a problem hiding this comment.
The problem here is that I need access to the backwards channel of the DcFifoIO, which is the DcFifoOut stuff. I couldn't figure out a way to do that within the circuit notation section, so I wrote a wrapper Circuit that lets me access it and its fields where I need to.
There was a problem hiding this comment.
Perhaps you can get some inspiration from CInputs and COutputs from Bittide.Transceiver. Those 2 are not merged into a single protocol, like you do with DcFifoIO.
rslawson
changed the title
hiddemoll
left a comment
hiddemoll
left a comment
There was a problem hiding this comment.
I think you might have forgotten to push your fixes. You marked 2 comments as resolved, but the code didn't change
| Circuit go | ||
| where | ||
| go (wbFwd, (dcFifoOut, _)) = | ||
| let | ||
| isFirstRising :: Signal readDom Bool -> Signal readDom Bool | ||
| isFirstRising = E.isRising clkRead flagsReset enableGen False . stickyE clkRead flagsReset | ||
|
|
||
| flagsReset :: Reset readDom | ||
| flagsReset = E.orReset rstRead (unsafeFromActiveHigh (clearStatusRegisters .== Just (BusWrite True))) | ||
|
|
||
| localCounterUnderflow <- shutter (isFirstRising underflow) -< Fwd localCounter | ||
| localCounterOverflow <- shutter (isFirstRising overflow1) -< Fwd localCounter | ||
| fn = toSignals goC | ||
| (wbBwd, dcFifoIn) = fn (wbFwd, dcFifoOut) | ||
| in | ||
| (wbBwd, (dcFifoIn, relDataCount)) |
There was a problem hiding this comment.
Perhaps you can get some inspiration from CInputs and COutputs from Bittide.Transceiver. Those 2 are not merged into a single protocol, like you do with DcFifoIO.
| instance Protocol (DcFifoOutput n readDom writeDom a) where | ||
| type Fwd (DcFifoOutput n readDom writeDom a) = DcFifoOutput n readDom writeDom a | ||
| type Bwd (DcFifoOutput _ _ _ _) = () |
There was a problem hiding this comment.
I do, yes. It's part of the right side of the output circuit from joinEbAndControl.
| instance Protocol (DcFifoIO n readDom writeDom a) where | ||
| type Fwd (DcFifoIO n readDom writeDom a) = DcFifoInput readDom writeDom a | ||
| type Bwd (DcFifoIO n readDom writeDom a) = DcFifoOutput n readDom writeDom a |
There was a problem hiding this comment.
Just saw that @hiddemoll also left a comment, but can you use DcFifoIn and DcFifoOut directly in your Circuit's? I think this would side step your Bwd issue.
There was a problem hiding this comment.
I don't think it would, since I would still need the DcFifoOutput to be backpressure from another circuit, which would still make me unable to bind it in circuit notation.
…ng function NOTE: will not pass CI
3 participants
What (what did you do)
The previous
xilinxElasticBufferWbandxilinxElasticBufferfunctions have been replaced by a set of new functions:elasticBufferAutoCenter,elasticBufferControlWb, andjoinEbAndControl, as well as theDcFifoC n readDom writeDom a vendorEbConstraintstype alias.joinEbAndControljoins a control circuit to anyDcFifoCwith the appropriate type parameters.Why (context, issues, etc.)
Issue #1298.
Dear reviewer (anything you'd like the reviewer to pay close attention to?)
I'm not certain of the interface for
joinEbAndControlat the moment. I'd like some thoughts on whether this is an appropriate degree of separation as mentioned in the linked issue, and if so, whether there's any ways that I could possibly improve the interface. At the moment it's not the neatest thing to use, but I haven't come up with a better way to do it yet.AI disclaimer (heads-up for more than inline autocomplete)
None.
TODO
Cross-outany that do not applyWrite (regression) testdocs/