[libre-riscv-dev] [Bug 64] data handling / io control / data routing API needed

bugzilla-daemon at libre-riscv.org bugzilla-daemon at libre-riscv.org
Fri Apr 19 10:16:23 BST 2019


--- Comment #3 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
Review of singlepipe.py

review of the following revision:



not documented.  required due to Record not being able to modify fields.
should be moved to its own issue (#66).


self._o_ready and s_o_ready and use of i_valid_test not documented or
clear.  makes a bit of a mess of the code.

effort is designed to allow data processing blocks to have a degree
of control over when they accept incoming data (even if upstream
is otherwise ready)

really not clear.


self.d_valid not documented or clear.

effort is designed to allow data processing blocks to control when
they *send* outgoing data (even if downstream is otherwise ready)

not clear.


a mess.  needs to be converted to an iterator/generator (yield)
needs __iter__ and __next__ (etc).


would be much simpler when Visitor is converted to a generator.


needs to be converted to an iterator/generator, and for yield
part to be split out to a separate class/function (similar to
what has been done with eq/Eq/Visitor).


name is unclear.  postprocess experiment not documented.  


name is again unclear.


name ("stage") unclear.  purpose (actual need) is also unclear.


name ("stage") unclear.  need for specallocate parameter is not documented


* name "stage" not clear
* ControlBase allocating i_data / o_data not clear
* connect() function could (does?) overwrite i_data / o_data
* role of set_input not documented (or clear)
* recursive ports() function's purpose not clear (or documented)
* _elaborate rather than just elaborate, reason not clear
* "dynamic" ready/valid (involving stage in signalling) not clear
* addition of _postprocess not been discussed (or evaluated)


* known bugs (#57) in interaction with other control-blocks
* *may* be possible to replace with FIFOControl (see below)
* really complex and not obvious what's going on
* name not clear


* based on wishbone / AXI4 signalling however actual characteristics unknown
* involved in bug #57
* name not clear


* based on "RegStage" from proposal, however actual characteristics unknown
* involved in bug #57
* name not clear


* based on "BreakReadyStage" from proposal, actual characteristics unknown
* involved in bug #57
* name not clear


* very useful as a building-block
* use of a single iospecfn however stops any kind of data width processing
  (this is good?)


* name not clear
* based on "BreakReadyStage" however actual characteristics unknown
* is not involved in bug #57


* probably should not be based on UnbufferedPipeline due to known issues #57
* better name needed?


* nice class!
* buffered argument has to go (use new Queue)
* use of flatten() on output causes problems for stage postprocess
* comment about "looking like PrevControl" redundant due to removal of code
* i_data requiring a "shape" function is problematic.

Replacement classes

all replacement classes need a proper audit and review of their own.
these are the classes with the exact same names, UnbufferedPipeline,
PassThroughHandshake, BufferedHandshake, SimpleHandshake.

should they even *be* replaced, due to the additional complexity
introduced through the use of Queue?  the graphviz layouts are a mess,
comparatively speaking.

You are receiving this mail because:
You are on the CC list for the bug.

More information about the libre-riscv-dev mailing list