<whitequark[cis]>
oh that's a fascinating approach
<whitequark[cis]>
custom action
<cr1901>
I recall that "r/w registers sharing the same address" was in scope, for e.g. an 16550 peripheral. So I went ahead and made my own until it's decided how to add that Action.
<whitequark[cis]>
oh I see
<tpw_rules>
i had thought about that type of action for the RFC, but figured that a person who desired such an action would want to put logic into it
<whitequark[cis]>
I think that action is basically supposed to be custom defined by the peripheral
<whitequark[cis]>
does that fit onto hx1k?
<tpw_rules>
so just routing the bus signals back out out was a bit brutish and inelegant for the action to do, the user should think a bit more cleverly :)
<cr1901>
>does that fit onto hx1k? No, the CSR version exceeds by like 70 LCs (running to check now).
<cr1901>
Which is not bad at all
<cr1901>
1354 LCs
<cr1901>
The same exact Rust firmware runs on both the CSR and WB versions despite peripherals being at different addresses; I deliberately code the WB peripherals to trigger a masked IRQ on reset, which the startup code checks to determine which set of IO addrs to use.
<cr1901>
So it's not _that_ bad (if a bit cursed) to support both versions for now
<whitequark[cis]>
amazing
<cr1901>
Thank you for your effort writing Amaranth, especially the past 6 months. I started Sentinel in late 2020, with the same goal as now (RISCV with Machine Mode, in HX1K). It became software I thought worth maintaining once lib.wiring got merged.
<cr1901>
Still some things to sort out, but I am at least past a wall of updating for now
<tpw_rules>
yay, first RFC from me! cr1901 you wanted this one too
<tpw_rules>
would be cool to discuss this week's SoC meeting
<tpw_rules>
yeah, lib.wiring is really exciting and i'm happy to get back into amaranth again
<cr1901>
tpw_rules: I did, and I still do want this workaround. MSP430 does the same thing- a register w/ a single field has the field name take the name of the register. And while I can still do the csr.Element workaround for the gateware, it's not as desirable w/ the current commits -soc
<cr1901>
(yea I did it "the right way" for the sake of good practices in the current commit)
<tpw_rules>
i don't think there's anything particularly wrong with using a csr.Element
<tpw_rules>
(actually the builder may barf)
<cr1901>
Last job for tonight is to find all the deprecation warnings from the Amaranth side and quash them. Then maybe I'll start writing the CHANGELOG entries to cut an(other) alpha release tomorrow.
<tpw_rules>
i need to do that for sim
<tpw_rules>
idk why i feel so allergic to test and sim in general, it's not a good habit
<whitequark[cis]>
<tpw_rules> "yeah, lib.wiring is really..." <- tpw_rules: nice!
<whitequark[cis]>
tpw_rules: possibly because we have no docs on simulation methodology and the (current) interface is kind of painful to use
<tpw_rules>
i mean i am bad at testing my regular software too, i just run through a set of cases in my head before cutting a release. and fix it if it breaks
<tpw_rules>
but also i feel like i end up writing a lot of really bizarre software
<tpw_rules>
like that work around insane hardware devices or have really bespoke GUIs
<whitequark[cis]>
ah
<tpw_rules>
if you have some pointers on using unittest or something that would be cool
<tpw_rules>
that's another scary thing, there's 500 libraries for regular python
<cr1901>
I use a pytest fixture that sets up the Python simulator. It has been very useful, but it's also very opinionated.
<tpw_rules>
i did that for an old project but it felt very application-specific to
<tpw_rules>
it was really awesome to use, i think i need to learn how to be test-brained or something
<whitequark[cis]>
ok so, new rules for memory and ports are: you add Memory. you do not add ports, at all
<whitequark[cis]>
as submodules that is
<whitequark[cis]>
oh, I think I know what's going on
<cr1901>
In my case, it was explained to me as a pytest quirk, I'll see if I can find the logs. But am curious of your explanation
<whitequark[cis]>
first off, yeah, you should probably add Memory as a submodule and not add ports. I don't think there's any harm in it but that's the new rules
* tpw_rules
makes mental note
<whitequark[cis]>
(it's for compatibility with lib.memory.Memory, whenever you migrate)
<cr1901>
>whenever you migrate tomorrow, probably
<whitequark[cis]>
second, the idea behind elaboration is that once you're done, you've created a snapshot of the design, be it a netlist or a simulator
<whitequark[cis]>
you can now use simulation objects like Signal to refer to innards of that snapshot (or other snapshots, as you can have concurrent simulations just fine)
<cr1901>
Okay, then yea, I was doing something "bad" that caused certain tests to just completely die
<whitequark[cis]>
as far as I know, the memory init file is just copied into the simulator on startup
<whitequark[cis]>
and you can only modify it afterwards using simulator commands
<whitequark[cis]>
tbh, I think we still haven't fully solved the problem of accessing memory in simulation
<cr1901>
For now, I will go with the current advice of "put memory init in __init__, don't add ports". I don't pervasively use memories, so it'll be a single small commit and hopefully nothing breaks :)
<cr1901>
But that is a problem for tomorrow-me. Bedtime for me.
<galibert[m]>
Wow, I would have failed at "Shitpost or not?" on sd express
<_whitenotifier-7>
[amaranth-lang/amaranth-lang.github.io] github-merge-queue[bot] a3a5f44 - Deploying to main from @ amaranth-lang/amaranth-soc@bf8dbd342befec0d9e9ddb898b644b623afb2a81 🚀
<cr1901>
>available as the init read-write attribute https://amaranth-lang.org/rfcs/0045-lib-memory.html Oh thank God it's read-write. This would break my pytest fixtures if init is immutable, as I just found out by making memory init as part of the SoC constructor
<cr1901>
"AttributeError: property 'init' of 'Memory' object has no setter" Or not. Huh...
<cr1901>
self.mem.init.clear()
<cr1901>
self.mem.init.extend(mem)
<cr1901>
Deleting items from Memory.init is not allowed
<cr1901>
(clear() because I want setting the memory contents to be idempotent)
<Wanda[cis]>
it's sort of mutable
<Wanda[cis]>
you're not allowed to change the length of the init object, or replace it wholesale
<cr1901>
This is exactly what I need, unfortunately :(
<Wanda[cis]>
I think the second could be made to work, though
<cr1901>
Just to avoid an XY problem 1/2
<Wanda[cis]>
you can do something like self.mem.init[:] = new_data, as long as new_data has the exact right length
<Wanda[cis]>
or self.mem.init[:len(new_data)] = new_data
<cr1901>
"self.mem.init[:len(new_data)] = new_data" does what I want
<cr1901>
So nevermind the XY problem (working around some pytest stuff)
<Wanda[cis]>
I do believe there is still an XY problem here though (Cat mentioned the details to me)
<Wanda[cis]>
we're considering refactoring all this to make Memory mutation less of a problem, by not requiring you to create it in the constructor
<Wanda[cis]>
which will probably involve exposing the MemoryIdentity object instead
<Wanda[cis]>
that should be less of a problem
<cr1901>
The RFC hints that init will become optional
<Wanda[cis]>
cr1901: it might, but I don't think that solves your original problem?
<cr1901>
For now, I was able to move instances of Memory into the constructors of all my Elaboratables, _while_ still setting init later, without my tests breaking for that reason
<cr1901>
(right now debugging TypeError: Received default command from process)
<Wanda[cis]>
we can also just make setting init allowed, as long as you provide something that could also have been a constructor argument (and it'll likewise be converted to the special Memory.Init object)
<Wanda[cis]>
I don't see any arguments against it, given that init is already effectively mutable; this just adds a nicer interface for it
<cr1901>
Ack
<cr1901>
"did you mean to add this process with add_sync_process() instead". No, I didn't.
<cr1901>
Because the migration guide says to "convert to add_process()"
<cr1901>
Yea I think I'm doing it wrong (tm)
<Wanda[cis]>
... so there are ways to do what you're doing without creating memories in the constructor, but they require a little private API abuse
<Wanda[cis]>
(create MemoryIdentity in the constructor, set mem._identity to it when you construct the Memory, then in the simulator use yield MemorySimRead(identity, addr) or yield MemorySimWrite(identity, addr, data))
<Wanda[cis]>
that's also kiiiinda how it'd work in the future in the version we'd like to propose
<cr1901>
I think the "Command None is not allowed in testbenches" msg should be modified to "you used bare yield. Don't do that."
<cr1901>
(Something less flippant of course)
<Wanda[cis]>
yes, that too
<cr1901>
(Also: Oh God, I used bare yield pervasively. Good thing not many tests to change.)