[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 12:31:49 BST 2019
--- Comment #4 from Luke Kenneth Casson Leighton <lkcl at lkcl.net> ---
hard dependence on typing is causing readability issues and
will make contributions from experienced python programmers
that much harder. *nobody* uses typing in common-usage python,
because it gets in the way. there is an actual known paradigm
which python conforms to, that the use of typing violates
(i just can't recall what it is, right now).
basically python is based on the Liskov Substitution Principle,
of *semantic* relationships rather than *syntactical* relationships,
and introducing syntactical relationships increases code size,
decreases readability, decreases flexibility and breaks python
can the typing be moved to a .pyi file and mypi deployed *if
the user wants to use it*?
Data, Shape, SignalShape, RecordShape, Empty, EmptyShape
* addition of name extension (prefix) is a good idea. if
it can be achieved in the same way as nmigen, that would
be even better. nmigen uses this code (example):
tracer.get_var_name(depth=2 + src_loc_at, default="$signal")
* restricting data usage using a syntactical hard limit instead
of defining a semantic relationship results in limitations
that a normal python developer will rebel against / reject.
the limitation of *all* data using *forcibly* being required
to go through the "Data" class instance is also a burden that
will result in significant user demands for "additional features"
on the Data paradigm, to add support for unanticipated
* the Data paradigm needs to be much more in line with the
nmigen "Value" object (i.e. the nmigen API, such as
using __iter__ instead of members() (or perhaps ports()),
having an eq (instead of "assign") function etc.
also note that Record is going to have a "connect" function
added (see nmigen-migen Record compat library)
in discussions with whitequark it was agreed that at some
point a "flattening" system would be provided such that
the nmigen IR "Visitor" system could handle arbitrary objects.
therefore it becomes even more important that Data conform
to nmigen's API, to avoid the "Principle of Least Astonishment"
when presented with the Data paradigm:
EntryPort (and ExitPort)
* o_ready and
* i_valid names were selected due to their vertical alignment.
this increases code clarity.
* Signals default to width 1. the addition of the width 1 is not needed.
* the name "EntryPort" begins with the letter E, and so does "ExitPort".
this is problematic from a clarity perspective in later usage
* use of Data.assign for ready_in/out and valid_in/out, they should just
be an eq.
* connect_from_entry is not clear what it does, by way of it being
an EntryPort connecting *to* an EntryPort. PrevControl._connect_in
is clear (by using the prefix underscore as a standard python convention
equivalent to "protected" in c++) that it should not be used outside
of the API, and its docstring warns against such.
with the name connect_from_entry being so similar to connect_from_exit
it is very confusing, particularly as they are the exact same names
similar logic applies to ExitPort
* comment is good. "side-effect-free data path", very clear.
* self.i and self.o (see above about Entry/Exit Port) get very confusing.
the naming "prev" and "next" (p and n for short) give clarity about
what shall be connected to what, particularly in the context of
ready/valid signalling going *both* directions.
* the use of the function create_data *on* an object - rather than being
a function itself - forces a requirement on chaining of segments to
provide an *intermediary* data object from which *both* the input
object *and* the output object are forced to inherit.
by simply having a function, the location of that function is irrelevant.
it could be a globally-imported function from a module. it could be
a lambda function. it could be a member class function. it could be
a static class function.
this is what is meant by *semantic* relationships (LSP) as opposed to
* the use of an object (shape.create_data) *may* make the use of
static classes difficult. needs evaluation and investigation
* the hard requirement for all derivatives to be modules is unnecessary
(particularly in simple cases)
* Data.eq not Data.assign is better, although just "eq" is preferred
(which can be used as "from import library import data" then "data.eq(...)")
* not sure what this class is for. the large amount of data typing
information is interfering with being able to understand it.
* once removed, the typing information should result in such a small
class that it may turn out to be unnecessary.
* data typing is interfering with readability
* first_child can be merged into children in the constructor,
making children_tuple redundant
* use of a tuple and the forced requirement that the tuple be an array
of CombSegs is a design-flaw hard and unnecessary limitation.
simply passing in *children and assigning them to __children will
allow developers to pass in anything that is iterable. generators,
__iter__-objects, tuples, lists, sets, dictionaries (iterating a
dictionary returns its values) and so on.
* in elaborate() the assignment of submodules is a good idea... however
it assumes that the object *has* an elaborate function (i.e. is
a nmigen module), which is a burdensome requirement in simple cases
* the functionality StageChain._specallocate is missing. some use-cases,
if the creation of an intermediary from a module is not done, the
entire module is "flattened" with a warning issued.
* not sure what this is for
* self.entry and self.exit are non-matching lengths, resulting in
loss of vertical alignment when reading and debugging the code
* reducing these to self.e and self.e is clearly not going to work
* this is why "p" and "n" were chosen in ControlBase.
* self.p and self.n are clearly "previous" and "next" yet at the same
time provide a base to put the "input" (i) and "output (o) into.
data is referred to as self.p.i_data and self.n.o_data which make
it really really obvious that input data comes from the *previous*
stage and output data goes to the *next* stage.
* why does Block.wrap exist? the need for Block and BlockLike is not
clear, and is probably down to the (unnecessary) type-restricting
* why does CombBlock exist?
* why is connect_from_entry not being used in elaborate()?
* all the same arguments applies to first_child and children as to
CombChain. can be greatly simplified.
* likewise same arguments apply to elaborate()
* naming convention connect_from_entry / connect_from_exit is causing
the very confusion predicted to occur. what's being connected to
* the exposure of the inner details of Module is unnecessary. an
example of the equivalent code for the singlepipe.py API is:
def elaborate(self, platform):
m = Module()
pipe1 = ExampleBufPipe()
pipe2 = ExampleBufPipe()
m.submodules.pipe1 = pipe1
m.submodules.pipe2 = pipe2
m.d.comb += self.connect([pipe1, pipe2])
in this code it is really clear, the pipes are declared, they're added
with names *chosen by the developer* as submodules, and then connected.
by contrast, in Chain.elaborate(), the developer has no choice about
the names of the modules (they are forcibly called child_0/1/2/3).
(likewise for the names of CombChain segs)
* Signal(1, reset=0), both width 1 and reset=0 are the defaults and may
* super().__init__(shape, shape) is not clear
* shape = Shape.wrap(shape) is not clear
* shape.create_data() again, see explanation about forcing the use
of an object to become part of the API rather than allowing freedom
and flexibility by way of a function
* self.data_out does not vertically line up with self.valid_out.
- self.o_data lines up with
at least at the prefix. code that was sequentially as follows
would also line up, improving readability:
which would make a user consider tidying them up as follows:
* like this class a lot, prefer that it be derived from FIFOInterface,
to make it clear that it conforms to the nmigen FIFO API.
* Queue entries can actually be zero, as long as fwft (first-word
fall-through) is enabled.
* using the name "fwft" as opposed to "allow_comb" makes it clear
that it has the same purpose and effect as fwft in the nmigen FIFO API.
You are receiving this mail because:
You are on the CC list for the bug.
More information about the libre-riscv-dev