<cr1901>
jfng[m]: jfng: It's been about a month since I last updated my Sentinel SoC example. What current changes do I need to make to the CSR* classes to make this not error out? Reading the test cases is not clear to me because you use a _MockRegister for everything, but csr.Register doesn't take a width (in fact, it takes fields. How do I create registers without fields then, like leds, inout, and oe regs in CSRLeds?)?
<cr1901>
I didn't pay close attention for a month and I'm utterly lost now amid the "Resource must be a wiring.Component, not csr.Element/flipped()" errors
<tpw_rules>
yeah a lot of things must have changed, Multiplexer only takes a memory map now
<cr1901>
Not clear to me when Element became a low-level detail
<tpw_rules>
i think Elements are okay to use still but not necessarily recommended
<tpw_rules>
looks like what happened is the memory map needs to be created and passed to the multiplexer and a csr.Builder is the best way to create that
<tpw_rules>
that code was last touched before i had to fix breakage from upstream on dec 12 2023
<cr1901>
ahhh, so it was
<tpw_rules>
i only used that to grab the widths from the signature because i like the annotations. it looks like you do that the other way around
<tpw_rules>
but i don't ever remember using a Multiplexer, i think i always used a Bridge and a Register.
<tpw_rules>
(nor an Element for that matter)
<cr1901>
Tbh, I adapted my code from the tests at the time. A Multiplexer muxing all the regs for a peripheral at the time seemed intuitive to me.
<tpw_rules>
where a bridge is really just a wrapped multiplexer. not actually sure looking at the code now what the benefit is of the Bridge
<cr1901>
Multiplexer hasn't changed much according to blame- mostly a mix of changes 8 months ago and last month. Maybe I'm just not doing it optimally anymore.
<tpw_rules>
oh, i guess the bridge also hosts the registers/elements as submodules
<tpw_rules>
i mean Multiplexer has no .add anymore
<tpw_rules>
the only thing you can do is construct it with a memory map
<whitequark[cis]>
yeah, the bridge hosts the multiplexer and registers as submodules
<tpw_rules>
that seems slightly subtle and footgun-y
<whitequark[cis]>
explain?
<tpw_rules>
i guess if you expect registers as entities unto themselves rather than just components
<cr1901>
yea, that was the point where I got lost, because the way I'm using csr.Element, I'm adding "anonymous registers without fields (i.e. each bit controls a different LED- there's no need for a deeper interpretation of any subset of bits)".
<tpw_rules>
like a multiplexer is 99% the same as a bridge, it's just that the bridge takes the registers and adds them as submodule stoo
<whitequark[cis]>
well, registers also need to be in the same clock domain as the multiplexer
<whitequark[cis]>
something needs to make sure that happens
<tpw_rules>
so if you're code trawling or something then you could use a multiplexer and forget the registers. hopefully documentation and examples will clear that up in the future
<whitequark[cis]>
you'll get an UnusedElaboratable warning then.
<tpw_rules>
cr1901: i'm 95% sure you can add Elements to the Builder instead of Registers if you like. you just have to have that to construct the memory map and pass it to the Multiplexer now. and i guess also add your Elements as submodules? or switch to a Bridge instead
<tpw_rules>
Elements might also become a bit of a pariah with future doc/BSP generators. the advantage of using Registers and Fields is that you'll get storage and can just comb connect all that instead of having a bunch of connection logic in your regs component
<tpw_rules>
it's not just about structure or interpretation
<tpw_rules>
you could just make an 8 wide "it's just leds" field if you like
<cr1901>
tpw_rules: So the context for me finding that ick is that for Rust msp430 board support packages, registers whose field takes up the whole register take an additional .dot_access for a redundant field
<cr1901>
e.g. gpio_out.gpio_out = val
<whitequark[cis]>
we don't currently have a super great solution for single field registers
<tpw_rules>
i have to use a synchronizer in my code for architecture reasons but those could just be replaced by a comb assignment in yours. inout might need a custom FieldAction though
<whitequark[cis]>
actually
<tpw_rules>
that is a good point
<whitequark[cis]>
I wonder if we do and it's just not clearly documented?
<whitequark[cis]>
I don't remember the API in detail sadly
<tpw_rules>
it was annoying to me to have to specify the register name and the field name
<whitequark[cis]>
can you specify a Field directly instead of a dict of fields to a register constructor?
<tpw_rules>
hm, that could actually be a good idea
<cr1901>
> I wonder if we do and it's just not clearly documented? <-- at the time I last worked on AttoSoC, all my registers were single field, and Element was "a way that works" to get a single field register without me having to care about the field's name (either during elaboration or for hypothetical CSR generation).
<cr1901>
It might've not been intended, but that's on me for invoking Hyrum's Law
<tpw_rules>
Fields are not intrinsically named, so if we just make fields be a single Field, then you can set a flag to remove the extra level from BSP/docs, and quite possibly the .f proxy
<tpw_rules>
(not sure how that would play with paths though)
<tpw_rules>
it does say that is optional but cr1901 i notice you provide it, maybe the multiplexer was unhappy without it?
<cr1901>
tpw_rules: Link the relevant code please b/c I'm at like 2% bandwidth right now?
<whitequark[cis]>
tpw_rules: can you do that *right now*?
<whitequark[cis]>
the path would be just () which is a valid path
<tpw_rules>
whitequark[cis]: i could whip together a prototype? it's not possible to supply a single Field to a Register without a container, it throws an exception as i linked above iiuc
<whitequark[cis]>
oh sorry I missed the link
<tpw_rules>
(i guess also if we are touching that should it accept FieldActionMap/Array?)
<cr1901>
tpw_rules: If I remove the relevant path args, the SoC builds fine. It's most likely a "best practice" thing I did.
<whitequark[cis]>
re: the .f accessor, I guess you could add a field which is an alias of fields and the .f will be an alias to either (they refer to the same container underneath)
<tpw_rules>
cr1901: ok. just curious because you said you didn't want to have to care about names and then you added an optional name
<whitequark[cis]>
there's a slight inconsistency in that you can access a thing with the wrong plurality but it's kinda whatever
<cr1901>
tpw_rules: I thought path was purely a debugging thing, not something BSPs use
<cr1901>
_that_ much I do remember :)
<whitequark[cis]>
BSPs will absolutely use paths
<tpw_rules>
whitequark[cis]: i mean i would think that .f or .fields would just point to the single unnamed Field(Action?) instead of a FieldActionMap/Array and then just deal with that plurality quirk.
<tpw_rules>
i don't think adding .field is worth, especially not conditionalizing it
<cr1901>
Lots of things in that branch should be treated as "I have no idea what I'm doing, I'm reading docs and copying what amaranth-soc tests do and if the SoC firmware works, I commit it"
<tpw_rules>
there could also be special treatment of a 1-length array but that could be footgun-y. don't generally like magic like that
<tpw_rules>
is there a difference between a path of tuple() and of None?
<tpw_rules>
whitequark[cis]: i could proooooobably prepare an RFC for the vaguely proposed single Field in a Register behavior above
<cr1901>
merge merge merge merge merg- *hit with a tranquilizer dart*
<tpw_rules>
shoudln't you be anyway? bedtime?
<tpw_rules>
(specifically a more formal RFC with accompanying implementation)
<cr1901>
Yes, tying up some last-minute loose ends. Then bedtime
<whitequark[cis]>
tpw_rules: I'm thinking we shouldn't conditionalize any of .f, .field, .fields
<whitequark[cis]>
1-length array should not be specially treated I think
<tpw_rules>
so the path of the internal field will just be `__`? i'm looking at the code and considering a` _flattened_fields` property which handles the case where self.fields.flatten needs to be called or just a static list returned, should that be public?
<tpw_rules>
actually i think it would just be ``
<whitequark[cis]>
_anything isn't public API
<tpw_rules>
well yes, should i instead name it `flattened_fields` so it's public
<tpw_rules>
okay, would you like me to write an RFC or are you working on an RFC/patch now?
<whitequark[cis]>
nope, I rarely contribute to -soc (mostly due to lack of time)
<tpw_rules>
ok. that sounds like you would be cool with me writing one then
<whitequark[cis]>
yep
<tpw_rules>
(i.e. you don't have any major objections to the idea)
<tpw_rules>
the RFC page says "In the future you will need to follow this process for the standard I/O library and the System-on-Chip library as well, but at the moment they are not covered.", is that out of date for SoC?
<tpw_rules>
or maybe the bar of necessity for SoC is higher than amaranth core?
<whitequark[cis]>
that's a question for jfng as to whether we require everything to go through the RFC process or not
<whitequark[cis]>
I think we do but I'm not making the policy on that
<whitequark[cis]>
(stdio is in a weird place where I'm not sure who's actually responsible for it, we need to sort that out at some point)
<whitequark[cis]>
zyp: thank you for the updates! I'd say the RFC is almost ready for a vote, with the under-defined nature of trigger objects being the last serious problem
FFY00_ has quit [Read error: Connection reset by peer]
FFY00 has joined #amaranth-lang
balrog has quit [Ping timeout: 268 seconds]
balrog has joined #amaranth-lang
<whitequark[cis]>
so on second thought, RFC 53 and RFC 55 go well together; plus RFC 55 is now in a much more mature state (enough for a vote)
<whitequark[cis]>
I am going to go with agenda for tomorrow of RFC 36 (if zyp updates it) and RFCs 53, 55, 54, 56, in that order
<whitequark[cis]>
note that RFC 54, if accepted, would not be scheduled for implementation until the release after the next one (0.6)
FFY00 has quit [Read error: Connection reset by peer]
<cr1901>
Anyone who uses VSCode... could you paste this into a new Python (must be Python type) file. Does it trigger "Call expression not allowed in type expression" lint in Pylance (for the unsigned(1) line)?
<cr1901>
You tell me "name must be a tuple", so I change it to a tuple, and then "destruct_res" bitches when it was passed a tuple _that I was told to pass into add_resource in the first place_.
<whitequark[cis]>
well it's probably a bug
<cr1901>
Forgive me for being frustrated; I expected this update to take an hour, not a full day. And I'm about to say "fuck it" and make an alpha release of Sentinel without updating amaranth/amaranth-soc, just delaying the pain.
<cr1901>
(While I can't make a release on PyPI until amaranth 0.5 at the very earliest, there is still a ton to do before I even consider Sentinel ready, and the alpha releases allow me to structure a CHANGELOG/docs in pieces without me being overwhelmed)
<whitequark[cis]>
-soc is in many ways (mostly relative lack of use compared to amaranth) entirely alpha quality software itself, even though it's been around for a while
<cr1901>
Wait. I misread the backtrace. This is my code.
<cr1901>
(Apologies to jfng[m] when you read this)
<Wanda[cis]>
I cannot request changes on my own PR? how annoying
<whitequark[cis]>
lol
<Wanda[cis]>
does github expect me to have functioning long-term memory
<whitequark[cis]>
zyp: I'm vaguely uncomfortable with the fact that all of the trigger object combinators are present on both the sim object and on the trigger object
<whitequark[cis]>
ok, not all; except until (this really just makes it a different kind of weird)
<whitequark[cis]>
it's... fine, you can write type annotations for it easily even, it's just bad vibes lol
zyp[m] has joined #amaranth-lang
<zyp[m]>
until is a condition, not a trigger
<whitequark[cis]>
what does until return anyway
<zyp[m]>
I just typed that into the RFC
<zyp[m]>
«The return value is an unspecified awaitable with await as the only defined operation.»
<whitequark[cis]>
sgtm
<whitequark[cis]>
so in practice, it will be basically `Simulation -> Trigger (-> Trigger)* (-> Condition)?`
<zyp[m]>
the PoC just has it as an async def that awaits self
<zyp[m]>
yeah
<whitequark[cis]>
s/->/→/, s/->/→/, s/->/→/
<whitequark[cis]>
putting this faux-grammar into the RFC might actually be quite helpful for understanding the design
<whitequark[cis]>
unrelated, but I'm really of my async for val in sim.changed(x) invention
<whitequark[cis]>
* unrelated, but I'm really proud of my async for val in sim.changed(x) invention
<zyp[m]>
if we wanted to avoid the duplication of trigger methods, we could have e.g. sim.trigger return an «empty» trigger object, e.g. await sim.trigger.posedge(foo).negedge(bar)
<zyp[m]>
yeah, I liked that
<whitequark[cis]>
async for is just perfect for this, the lexical construction is perfect, the scoping fits well, the execution model fits well I think
<whitequark[cis]>
zyp[m]: we could for sure, but I feel like that's quite verbose
<zyp[m]>
yes.
<zyp[m]>
we could shorten it to sim.t, but still
<whitequark[cis]>
I've also mulled over having trigger as a second argument to the function
<whitequark[cis]>
thooough then you have to pass it around
<whitequark[cis]>
even worse
<zyp[m]>
I don't think the method duplication is all that bad, I figure implementation wise we can internally just have them all create an empty trigger object and forward the call