<whitequark[cis]>
from writing a lot of streams based code recently, I can say that it's very important to have some sort of consistent notation, otherwise you only learn that you have a direction issue from wiring.connect failing, and may not learn it at all if you manually expand the connection via m.d.comb (which happens a decent amount)
<whitequark[cis]>
any directional terms (input/output, in case of lib.wiring) are relative: input of what? I would like to suggest that the "origin" of the relative term should always be to the object whose attribute it is. that is, if you have component.i, the "i" always means "input of the component (whatever comes before the dot prior to i)"
<whitequark[cis]>
there is a somewhat annoying edge case here, where io.Buffer.i is an output. when wrapping io.Buffer into a stream based component (which I think should be in amaranth-stdio or even lib.io, but this is a question for another time) I found it that the streams should follow the stream convention even if the buffer-related signals do not
<whitequark[cis]>
I also trialed r/w for queues, and it turns out to be (a) confusing by itself, and (b) even more confusing when you wrap a queue into another component; you see "foo.r", and go "ok, is this an input or an output?" and it's really not clear or easy to tell (especially without editor services)
<whitequark[cis]>
sink and source are, in my experience, a complete failure of terminology because newcomers to litex near-universally report these as very confusing
<zyp[m]>
yeah, I switched to input/output as soon as I moved away from litex
<whitequark[cis]>
so, my proposal: standardize on some variation on "input" and "output", absolutely not do "sink"/"source", ignore "read" and "write" as distractions
<zyp[m]>
I agree 100%
<whitequark[cis]>
I think it is also important to have commonality with lib.wiring; I think the reason read/write are problematic is exactly because there is no such commonality
<whitequark[cis]>
therefore, my complete proposal is: for components that have one primary output and one primary output, call them i and o, corresponding to In(stream.Signature(...)) and Out(stream.Signature(...)). for components with more complex interfaces, call them i_something and o_something where the something is decided on a case by case basis, and the directionality is the same
<whitequark[cis]>
or, rephrasing a bit: whenever you have directionality indicated in the attribute name, it must be indicated using "i" or "o" corresponding to "In" or "Out" direction of the member
<galibert[m]>
that's pretty much what was the convention before wiring too, right?
<whitequark[cis]>
sorta, yeah
<whitequark[cis]>
although that was never formalized
<galibert[m]>
so no surprise there
<galibert[m]>
which is good. Surprises lead to bugs
<whitequark[cis]>
I also suggest saying in the RFC that the direction indicator should be at the beginning
<whitequark[cis]>
so, acceptable names all include: i, i_frames, or requests
<whitequark[cis]>
(personally, I think mandating that every member include its directionality at the beginning is a step too far and sometimes it's clearer to not follow this convention; but when you do add directionality, it must be indicated in this particular way)
<whitequark[cis]>
any comments?
<galibert[m]>
So it's more of a stylistic convention?
<whitequark[cis]>
I would say that anything that relies on matching attribute names is fundamentally a stylistic convention
<zyp[m]>
whitequark[cis]: are you thinking beyond streams now?
<whitequark[cis]>
if we wanted it to be more than stylistic, we would define a base class or Xxxable interface class which would specifically ensure there are the right members present
<whitequark[cis]>
zyp: in general yes, in the context of this discussion I'd say we leave it scoped to streams
<whitequark[cis]>
any decision we make on streams will to some extent influence the rest of the code people write anyway; and I don't want this RFC to end up scope-creeping into a general stylistic convention
<zyp[m]>
agreed
<whitequark[cis]>
so, the proposal I've laid out above will satisfy your desire to have a pipeline connect function
<whitequark[cis]>
* so, the proposal I've laid out above will satisfy your desire to be able to write a pipeline connect function
<whitequark[cis]>
personally, I'm unclear on how useful these are (at the moment--this is by no means a final opinion), but I see value in enabling it, especially if it also makes life easier for everyone else
<zyp[m]>
yes, the only difference from what I've been doing already is shortening input/output to i/o and I'm not opposed to that
<whitequark[cis]>
yes. the reason I propose the shortened form is twofold. first, we already shorten it to i in many places, so this follows precedent. second, saying "input_frames" gets pretty tiresome, especially given that the way you access the insides of a stream is already fairly verbose. being able to go between "i" and "i_frames" when you need it makes refactoring quite a bit nicer
<whitequark[cis]>
all right. any other comments, or is everyone OK with this formulation of the RFC?
<whitequark[cis]>
Wanda? galibert?
<galibert[m]>
I'm ok with that
Wanda[cis] has joined #amaranth-lang
<Wanda[cis]>
sounds good
<whitequark[cis]>
then, disposition on RFC #77: merge with the resolution above for the unresolved questions
<whitequark[cis]>
there is one unresolved question: what to do with DDRBuffer and internal I/O ports?
<whitequark[cis]>
I believe we need to have a glitch-free simulation model for DDRBuffer, else it is very difficult to test any I/O machinery using DDR ports (which is frankly most of them, when you want high-speed IO) and I've hit this problem many times already in Glasgow (which uses a custom simulatable DDR buffer)
<whitequark[cis]>
I don't think there is any reasonable way to provide a generic synthesis model for DDRBuffer so we should make that an error
<galibert[m]>
what is the use-case of components with "explicit" ioports at the interface, it's do make enable easier?
<galibert[m]>
s/do/to/
<zyp[m]>
I figure a synthesis DDR InternalPort would just pass on double-wide signals, like on the gateware side of the buffer
<whitequark[cis]>
galibert: I don't understand the question
<whitequark[cis]>
zyp: oh, hm, that would work actually
<zyp[m]>
just like how a bidir InternalPort still is a in/out/oe triple
<Wanda[cis]>
that wouldn't work
<Wanda[cis]>
ports are buffer type agnostic
<Wanda[cis]>
you could at most make an InternalDDRPort that would work only with DDR buffers
<whitequark[cis]>
yeah, that's what I was thinking of
<galibert[m]>
I'm reading the motivation, and I'm wondering why a component has a ioport in the first place instead of the top component?
<galibert[m]>
or anything that actually names the port if you want to distribute the pin instantiations
<whitequark[cis]>
galibert: because the type of the instantiated buffer is an implementation detail of the I/O component
<whitequark[cis]>
when I create a QSPIController, I give it a ports argument, which is a "port group" (currently an informal name for an object whose attributes contain io.PortLikes). the controller instantiates whichever buffers it wants and connects them to whichever clock domains it likes
<whitequark[cis]>
otherwise, you cannot make usable abstractions around I/O ports
<galibert[m]>
So it's about being able to have buffers in the component?
<whitequark[cis]>
yes
<galibert[m]>
ok, I think I get it now
<whitequark[cis]>
I should be able to upgrade a component from using FFBuffer (maximum throughput 0.5/cycle) to DDRBuffer (max. throughput 1/cycle) to device-specific Instances without changing the callers
<whitequark[cis]>
this was the major flaw of the old platform.request system and one of the big reasons to have lib.io
<whitequark[cis]>
* I should be able to upgrade a component for some hypothetical single data rate synchronous interface from using FFBuffer (maximum throughput 0.5/cycle) to DDRBuffer (max. throughput 1/cycle) to device-specific Instances without changing the callers
<whitequark[cis]>
I've been trialing this methodology in Glasgow and it's been very effective
<whitequark[cis]>
the main issue is that we need to formalize the notion of port groups, but that's not on today's agenda
<whitequark[cis]>
zyp: I haven't seen much need for `InternalDDRPort` yet; I suggest we add `InternalPort` that covers the majority of use cases, while leaving the option to add `InternalDDRPort` later for the cases where you have a need to forward these
<whitequark[cis]>
I feel that InternalDDRPort isn't necessarily very useful because it doesn't capture the clock domains
<whitequark[cis]>
or, perhaps, it should include the clocks, but that makes for an even more complex design that would have to be trialed first
<zyp[m]>
agreed; I'm not sure when you'll want to connect a component with DDR buffers to another gateware component
<Wanda[cis]>
I think InternalDDRPort is a major implementation detail leak; if you need to do something like that, it's likely better to have a component that has a DDRBuffer.signature port
<whitequark[cis]>
yeah I agree
<zyp[m]>
but the DDR simulation case is important, which AIUI can be covered with a regular InternalPort?
<whitequark[cis]>
yes
<whitequark[cis]>
any other comments?
<whitequark[cis]>
alright. should we merge this? zyp galibert
<zyp[m]>
sounds good to me
<galibert[m]>
To me too
<whitequark[cis]>
thanks. we don't have time for another RFC so this concludes the meeting