RobTaylor[m] has quit [Quit: Idle timeout reached: 172800s]
frgo has quit [Remote host closed the connection]
frgo has joined #amaranth-lang
frgo has quit [Read error: Connection reset by peer]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 246 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 268 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 260 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 256 seconds]
jfng[m] has joined #amaranth-lang
<jfng[m]>
<cr1901> "jfng: Decent chance I won't be..." <- ack
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 260 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 252 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 245 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 252 seconds]
balrog has quit [Quit: Bye]
balrog has joined #amaranth-lang
Wanda[cis] has quit [Quit: Idle timeout reached: 172800s]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 256 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 276 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 245 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 268 seconds]
frgo has joined #amaranth-lang
frgo has quit [Remote host closed the connection]
frgo has joined #amaranth-lang
frgo has quit [Remote host closed the connection]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 260 seconds]
frgo has joined #amaranth-lang
frgo has quit [Ping timeout: 246 seconds]
<tpw_rules>
jfng[m]: is pr #40 fully baked?
<tpw_rules>
i saw it just got updated and i'm not sure if the philosophy changed or it's not completely updated for the latest amaranth changes yet and in a workable state
frgo has joined #amaranth-lang
<jfng[m]>
tpw_rules: it has been updated to solve the design issue that you found last time; i am currently updating the RFC itself (which has severly bitrotten since lib.wiring happened)
<tpw_rules>
okay. it looks like csr.field.RW(32) got mapped to Field(csr.field.RW, 32)
<jfng[m]>
yep!
<tpw_rules>
is that the intention? it seems strange to me that things in csr.field are not Fields
<tpw_rules>
it's not a problem updating, maybe that's a discussion to have once the RFC is refreshed
<whitequark[cis]>
csr.access.RW could be better perhaps
<whitequark[cis]>
or csr.mode.RW
<jfng[m]>
the idea is that Field is a factory for field components
<jfng[m]>
whitequark[cis]: i like `mode`
<tpw_rules>
why doesn't csr.field.RW inherit from Field and have the Component be an implementation detail?
<whitequark[cis]>
tpw_rules: that's more work if you want custom access; now you need to override two closely coupled things instead of one loosely coupled
<tpw_rules>
you could still pass whatever you like to Field if you want custom behaviro but that seems like a common case.
<tpw_rules>
why do you have to override a second thing?
<tpw_rules>
i see the things in csr.field as pre-canned and final
<tpw_rules>
i also don't immediately see how csr.mode.RW would mix with custom stuff, if it were simply renamed from csr.field.RW
frgo has quit [Ping timeout: 260 seconds]
<tpw_rules>
whitequark[cis]: should it be possible to assign arbitrary attributes to a Signature?
<whitequark[cis]>
tpw_rules: it's too annoying to deny
<tpw_rules>
okay
<whitequark[cis]>
so we rely on "don't do that" as a safeguard
<tpw_rules>
okay. i just noticed my code which did that previously wasn't barfing and got curious
<jfng[m]>
i'm still updating the RFC (#16), but the implementation can be tried out
<tpw_rules>
i successfully did and it seems to work nice. i do think there should be rework around what's in csr.field and what those objects mean/do
<tpw_rules>
but it might be hard to discuss without seeing the RFC
<jfng[m]>
tpw_rules: yes, the examples are fairly outdated, and the reference-level explanation is very different now that lib.wiring is a thing
frgo has joined #amaranth-lang
<tpw_rules>
i guess most relevantly, calling it csr.access or csr.mode doesn't sound right to me. is RW1S really a mode?
<jfng[m]>
it is an access policy
<tpw_rules>
does that mean that the object you pass to a Field defines the access policy?
<jfng[m]>
... but also a field implementation, which in this case has its own storage, etc
<tpw_rules>
i should think what you pass to Field's constructor is an implementation
<jfng[m]>
tpw_rules: yes, in a sense; but only for that specific field
<jfng[m]>
users can choose to pass their own field components, with arbitrary policies
<tpw_rules>
for sure
<jfng[m]>
we provide built-in fields for convenience
<tpw_rules>
yes. i just think those things should be an instance of Field rather than just the implementation
<tpw_rules>
if you wanted your own implementations, you can still do that. and then the question of policy is not really answered at this time
<jfng[m]>
what i like about the current design, is that a field can be just about any component with a `port` member in its signature, that is an `In(FieldPort.Signature(...))`
<tpw_rules>
yes, i think that's good too
<jfng[m]>
which gives a lot of freedom to user, if they wish to implement custom fields
<zyp[m]>
I like the sound of that
<tpw_rules>
my only point is that i think the things in csr.field (R, RW, W, RW1C, RW1S as of now) should be instances of Field and have the implementation be internal
<tpw_rules>
rather than types you need to pass to Field yourself
<whitequark[cis]>
if the implementation is internal, how do you override it?
<zyp[m]>
presumably by making a custom child of Field
<tpw_rules>
i don't immediately see a case where you'd want to. you would make your own thing to pass to Field which has your own logic. the stuff in there is only a few lines
frgo has quit [Ping timeout: 256 seconds]
<tpw_rules>
did you have a usecase in mind for overriding?
<jfng[m]>
fields with one-time programmable bits (i.e. fuses)
<jfng[m]>
fields with custom storage, such as shift registers
<tpw_rules>
that would be a very different thing from anything in csr.field. which of those implementations would you subclass?
<jfng[m]>
or storage that is provided externally
<jfng[m]>
tpw_rules: none
<tpw_rules>
to be clear i think Field itself should be exposed for user use. what i meant by "have implementation be internal" is all the things currently in csr.field module
<jfng[m]>
you would implement a component with a signature that has a `FieldPort.Signature` member
<whitequark[cis]>
I think this discussion is unnecessarily confusing because "Field" is both a name and a specific class
<whitequark[cis]>
tpw_rules: so it's not clear whether you are talking about the specific class or the syntax
<tpw_rules>
what do you mean by name?
<whitequark[cis]>
do you want different syntax or different semantics (inheritance, etc) or both
<tpw_rules>
let me try again
<zyp[m]>
it's a bit unclear to me what the benefit of the Field class is
<tpw_rules>
all the classes in the module csr.field (namely R, W, RW, RW1C, RW1S) are currently subclasses of wiring.Component. this means that to use them, i have to construct a Field instance which takes one of those class types. i propose that instead, those classes should be subclasses of the Field class, which passes an internal implementation to the constructor
<zyp[m]>
as in, what is the benefit of being able to write Field(RW1C, …) over e.g. RW1C(…)? lazy creation?
<whitequark[cis]>
Field lets you distinguish that particular annotation within a Register subclass from other annotations, and delays instantiation of the component inside until later so we don't end up with global state unintentionally
<tpw_rules>
exactly. that way, my csr.Register subclass would have coolreg: RW(4) instead of coolreg: Field(RW, 4)
<whitequark[cis]>
tpw_rules: I understand what you are proposing. I do not understand your motivation
<tpw_rules>
what i just said. less typing, more clarity in the code and the module name
<tpw_rules>
if you wanted a shift register field, or an e-fuse field, or something. you could still do coolreg: Field(EFuse, 4)
frgo has joined #amaranth-lang
<whitequark[cis]>
tbf, those are all very obscure examples
<whitequark[cis]>
I found an example just now writing something as basic as a GPIO peripheral
<whitequark[cis]>
which needs to have a set/reset register, which flip bits in the output data register
<whitequark[cis]>
which needs a custom field!
<tpw_rules>
of course
<tpw_rules>
are you saying that you want use of custom field implementations to look the same as built-in field implementations?
<whitequark[cis]>
so what you want is a different syntax
<jfng[m]>
tpw_rules: absolutely
<tpw_rules>
yes, for the built-in field implementations
<jfng[m]>
there is nothing special about our built-in fields
<jfng[m]>
we just identified them as common use-cases
<tpw_rules>
i would call them field implementations, but yes
<jfng[m]>
agreed
<tpw_rules>
then the docs and module name should be updated to call them that
<jfng[m]>
good point
<whitequark[cis]>
what is the proposed module name?
<tpw_rules>
i don't know. i wonder if the Field class should instead be called FieldFactory
<zyp[m]>
that sounds overly verbose for something you'll be typing all the time
<tpw_rules>
it does
<whitequark[cis]>
also we're not writing Java
<tpw_rules>
:P
<galibert[m]>
And way too Java-y
<galibert[m]>
Huhuhu
<tpw_rules>
but for the module, i don't think access or mode are appropriate because
<tpw_rules>
they are more than that
<zyp[m]>
type?
<tpw_rules>
they're not that either
<tpw_rules>
they're an implementation, but we can't call it field_impl without being too reminded of java for comfort either
<tpw_rules>
maybe storage?
<zyp[m]>
yeah, but it's an implementation of a field type, or a field kind or a field mode or a field access policy
<zyp[m]>
storage sounds wrong, given that a field doesn't necessarily have storage at all
<tpw_rules>
Field(csr.storage.RW, 8)
<tpw_rules>
but all of them in that module do, except for W
<jfng[m]>
tpw_rules: not every field has storage
<whitequark[cis]>
I think csr.field_impl.RW would be awfully ugly even without Java
<tpw_rules>
handler??
<whitequark[cis]>
no
<whitequark[cis]>
(do we have nothing better to do than to bikeshed naming?P)
<whitequark[cis]>
s/P//
<tpw_rules>
port? if the distinguishing thing of a field implementation is that it has a port member with FieldPort.Signature
<whitequark[cis]>
the port is a part of the field implementation
<tpw_rules>
i don't want to bikeshed but i find it confusing that the things in the csr.field module are not subclasses of Field and there's objection to making them so
<zyp[m]>
why should they be subclasses of anything when there's nothing to inherit?
<zyp[m]>
the interface is given through the port member of the signature and everything else is implementation specific
<tpw_rules>
well perhaps not subclasses, but i think they should be things where you can put an instance of Field
<tpw_rules>
things which can be put in the same place where you can put an instance of Field*
<zyp[m]>
Field doesn't make sense to substitute something else for, the customizability is in what type you pass to Field
frgo has joined #amaranth-lang
<tpw_rules>
i wouldn't call the type you pass to Field a "field" though. it doesn't become something you can do Field-like things with (e.g. use as an attribute on csr.Register) until you pas it to Field. so i think the type of things in the csr.field module should be the latter, rather than the former
<jfng[m]>
btw, i think `Field.create()` should raise an exception if the instance it created is not a component with a signature with a `FieldPort`
<tpw_rules>
(or your field might just be completely ignored)
<zyp[m]>
jfng[m]: it should, and that's easy to do through `.is_compliant()`
<whitequark[cis]>
tpw_rules: fwiw I stripped out the similar message from `Component` itself
<jfng[m]>
<tpw_rules> "in any case, i have to step..." <- i should maybe improve that error message to suggest that a component was given instead of a Field; but i think that checking for it specifically is overkill
frgo has quit [Ping timeout: 255 seconds]
frgo has joined #amaranth-lang
<zyp[m]>
by the way, if I wanted to put together a somewhat «complete» amaranth-based soc, what library options do I currently have? I'm aware of lambdasoc, what else?
<tpw_rules>
that might be true. i'll try to brainstorm better names
<tpw_rules>
could it just be csr.comp and call them field components?
<whitequark[cis]>
csr.action.RW?
<tpw_rules>
i think that's the best so far, but there might be better
<tpw_rules>
much better than the current
* whitequark[cis]
is thinking of "RW-action" as in "bolt-action"
<whitequark[cis]>
the core of the mechanism
<zyp[m]>
I've heard about revolver registers before, but bolt-action registers is new :)
<tpw_rules>
that almost makes more sense for the like set-action and reset-action RW1S and RW1C
<whitequark[cis]>
yes
<tpw_rules>
(then store-action and no-action?)
notgull has quit [Ping timeout: 264 seconds]
notgull has joined #amaranth-lang
<zyp[m]>
hmm, I'm having some issues with RFC #27, I inadvertently created a race condition
<zyp[m]>
so I've got a stream interface with .send_packet() and .recv_packet() helpers, and I'm testing a module with stream in and out, so I've got a testbench sending and a testbench reading out
<zyp[m]>
and the module has a combinational bypass mode, in which case the two testbenches will race
<zyp[m]>
switching the testbenches to use add_sync_process() instead works, but requires changes in the interface helpers to account for the different signal timing
<zyp[m]>
and the real issue here is that interface helpers don't know/choose which context they're being called under
<whitequark[cis]>
the interface helpers are not possible to implement in general like this
<whitequark[cis]>
it's not just that there are two meaningful contexts for them to work in
<whitequark[cis]>
but rather that you cannot make a helper for something like that with add_sync_process
<whitequark[cis]>
this race is expected and the solution for it needs to lie somewhere outside of what we've been considering so far
<whitequark[cis]>
or maybe having wait_changed would solve this
<zyp[m]>
I think a potential solution for this could be to wrap the racy code in a context manager that disables automatic settling
<whitequark[cis]>
no
<whitequark[cis]>
absolutely no global state like this
<whitequark[cis]>
I'm not even going to consider it
<whitequark[cis]>
and it's not really "automatic settling", it's the wrong mental model for this. the horseless carriage of testbenches
<whitequark[cis]>
I think the crux of the matter is that whenever two testbenches wait on the same trigger and then try to communicate using comb signals they will race
<zyp[m]>
yes
<whitequark[cis]>
we can actually probably detect this case, though that's a decent amount of work and I'm not sure resources are available for it
<whitequark[cis]>
but I think this particular case can be fixed by not waiting on the same trigger
<whitequark[cis]>
ie waiting on ready instead of tick
<zyp[m]>
another potential solution would be something like splitting .set() into comb and sync variants, i.e. one that's committed immediately and one that's not committed before time advances
<whitequark[cis]>
wouldn't you need to propagate that to every helper?
<whitequark[cis]>
and it sounds very difficult to use correctly
<whitequark[cis]>
whatever solution we end up with, we need to consider how we're going to teach that, and this seems only a marginal improvement over the status quo
<zyp[m]>
I mean, it'd be like writing m.d.comb += foo.eq(bar) vs m.d.sync += foo.eq(bar)
<whitequark[cis]>
would it be?
<whitequark[cis]>
I would expect await x.set(1, domain="sync") to commit on the next tick or something
<whitequark[cis]>
rather than just whenever time advances
<whitequark[cis]>
I think detecting a race is easy: for each process that runs in a given delta cycle, we record the sets of sampled and driven signals
<whitequark[cis]>
if they intersect there is a race
<whitequark[cis]>
this means we can show a warning, which is nice
<whitequark[cis]>
actually, no, that doesn't quite work
<zyp[m]>
my goal is not to be able to detect races, I want to be able to write code that avoids races
<whitequark[cis]>
yes, that would be my next point
<whitequark[cis]>
suppose I show you a warning; what next? the warning should tell you what to do next
<whitequark[cis]>
how is my proposal of waiting on the ready signal as the trigger not sufficient here?
<zyp[m]>
can I trust that there won't be «glitches» on it between ticks if it's derived combinationally from other signals?
<zyp[m]>
and what if it's continously high in a burst transfer?
<whitequark[cis]>
zyp[m]: the wait operation would only call you after comb signals are settled, yes?
<whitequark[cis]>
if it's a "spurious wakeup" where the signal no longer satisfies the wait condition by this point, the loop internally repeats
<zyp[m]>
when are the signal considered settled if another testbench are driving the signals ready is combinationally derived from?
<whitequark[cis]>
testbenches waiting on a signal aren't called until all other processes (user and rtl) are done
<zyp[m]>
hmm, true
<whitequark[cis]>
the proposal kind of kicks the can down the road, hoping that if instead of all testbenches waiting on the same clock signal (extremely common) only testbenches waiting on the exact same ready signal (much less common) would race
<whitequark[cis]>
s/if//
<zyp[m]>
<whitequark[cis]> "absolutely no global state..." <- by the way, what do you mean by global state?
<whitequark[cis]>
there is a global bit of state, affecting the entire simulation, changing how yield or whatever behaves
<whitequark[cis]>
this is unacceptable
<whitequark[cis]>
I mean, there is with your proposal. the bit of state set and reset by the context manager
<zyp[m]>
it wouldn't affect the entire simulation, just the particular process
<whitequark[cis]>
I'm using "global" here as a synonym for "non-lexical"
<zyp[m]>
it'd be a «pretend this process was started with add_sync_process for this block»
<whitequark[cis]>
because once meaning of syntax stops being lexical and becomes dynamic it's kind of all lost
<whitequark[cis]>
further, I do not think sync processes and bench processes can or should be in any way interchangeable
<whitequark[cis]>
and any attempt to mingle them together would just result in pain
<whitequark[cis]>
it should probably be forbidden to do await f where f is a user function in a sync process
<whitequark[cis]>
* it should probably be forbidden to do await f where f is a user function affecting simulation state in a sync process
<whitequark[cis]>
though I'm unsure
<zyp[m]>
I think not being able to write race-free helper functions is a huge limitation
<whitequark[cis]>
I think both sync processes and comb processes should have a defined list of inputs and outputs and any effect on the simulation except for changing those should be an error
<whitequark[cis]>
(we don't have comb processes yet but we hopefully will eventually)
<whitequark[cis]>
zyp[m]: it's better to have racy helper functions than no usable helper functions at all
<zyp[m]>
I'd be inclined to find a way to kill the need for add_sync_process to exist as a separate thing
<whitequark[cis]>
remember: add_sync_process is not for testbenches
<whitequark[cis]>
anyway, if you only have add_sync_process you cannot write usable helper functions at all
<whitequark[cis]>
consider that you cannot even make fifo.read() that completes in a single cycle if you only have add_sync_process
<whitequark[cis]>
this is why there currently isn't one: because not being able to read a value from the FIFO every cycle is ridiculous