<whitequark>
I have three suggestions for this RFC:
<whitequark>
1. separating CRC parameters (and associated methods for e.g. generating the matrix) into their own class, so that we don't have to pass them around as dictionaries that are **kwsplatted to the CRC constructor if we also have SoftwareCRC, and can be converted to e.g. a software CRC instance from Amaranth stdlib or another library
<whitequark>
2. renaming the parameters to something that's less... 1990s "we will run out of identifier characters" style
<whitequark>
3. enabling some form of software CRC
<d1b2>
<Olivier Galibert> Why 3?
<adamgreig[m]>
For 1, will you end up with a parameters class that just takes the same arguments the current class does, which you have to construct and then pass into the hdl module?
<whitequark>
Sarayan: huh?
<adamgreig[m]>
I do like the idea especially because it also enables providing a library of named commonly used CRCs
<whitequark>
Sarayan: are you sure you're getting all the messages?
<whitequark>
adamgreig: yes, exactly
<adamgreig[m]>
(with a dict of name to arg-class instances)
<whitequark>
and we can potentially e.g. rename arguments later
<adamgreig[m]>
But I wonder if that same argument class just has a method to generate a module and a method to run software crc, rather than being something you pass to another class
<whitequark>
yes, I've thought about this
<adamgreig[m]>
(whether it's then itself an Elaboratable idk)
<whitequark>
adamgreig: I think it shouldn't be itself Elaboratable because the parameters are reusable, but the Signals an elaboratable exports are single-use
<adamgreig[m]>
We already do it to construct the hardware and it's then very convenient to have it to hand with the same arguments
<adamgreig[m]>
Would you have it have a method that returns an Elaboratable then maybe?
<whitequark>
Sarayan: well, it's common to have a CRC checker in hardware for something and then you need to generate those CRCs in host software
<whitequark>
so providing a bridge between the two is very handy. Glasgow needs this all the time for protocol handling for example
<whitequark>
adamgreig: we need to export the name of the returned elaboratable class, so that it can be documented properly
<d1b2>
<Olivier Galibert> So a CRCConfiguration class that does the magic, then a CRC Elaboratable and a compute_crc function that take a configuration object as a parameter?
<whitequark>
but I think we can very well have a method on the parameters class that gives you that elaboratable, for consistency with it having another method to make a software CRC implementation
<d1b2>
<Olivier Galibert> that's the other way around
<whitequark>
what is?
<adamgreig[m]>
Sounds fine to me. And the software implementation is another class so that it can hold state?
<d1b2>
<Olivier Galibert> either you have a config class that does the common computation, and classes or functions to generate an elaboratable or do a computation, or you have a big class that does everything
<whitequark>
Sarayan: I'm saying we should probably have it "both ways" because the class needs to be documented anyway, at which point you might as well document the constructor
<adamgreig[m]>
Perhaps with a convenience method on the args class to just do a one off computation and return result
<whitequark>
Sarayan: I think we're talking past each other
<d1b2>
<Olivier Galibert> possibly so
<whitequark>
I'm saying we need two or more separate classes: one to hold the parameters, one that's Elaboratable, and some way to ask the parameters class to let you compute a CRC in software
<d1b2>
<Olivier Galibert> Ok, we agree there
<whitequark>
adamgreig: I'm wondering what's a good way to do software CRC
<whitequark>
I think the crc library should be used opportunistically if it's available
<adamgreig[m]>
For a first pass we can just use a marginally cleverer version of the existing algorithm
<whitequark>
there's no real need to pull in another dependency to do a CRC of like 10 bytes when we already have and use the software CRC implementation
<d1b2>
<Olivier Galibert> I thought you wanted the class-that-holds-the-parameters to also have methods for generating an Elaboratable or computing a crc
<adamgreig[m]>
I can remove the parts that turn bytes into bit strings and reverse the strings
<whitequark>
adamgreig: oh, actually, hold on
<adamgreig[m]>
It was expedient and fine for computing the matrices but it's just a bit more maths to do it as pure integers in python
<whitequark>
your implementation is strictly more powerful than the crc library, right?
<adamgreig[m]>
Yes
<adamgreig[m]>
In terms of acceptable word lengths and crc lengths
<adamgreig[m]>
Oliver: sure, but they are just one liners to construct the relevant class and pass self in as the argument, just for convenience
<d1b2>
<Olivier Galibert> ahhhh ok
<adamgreig[m]>
For a first pass, how about I improve the software implementation it already uses to just avoid going through strings, and I suspect that turns out to be plenty fast, and in the future an accelerated option could be added with the same API?
<whitequark>
yep, let's not integrate with anything external for now
<adamgreig[m]>
We could also have an optional method to turn the arguments class into a configured instance from the crc library that errors if crc library's not installed or args aren't compatible, but it seems marginal benefit
<Chips4MakersakaS>
For software crc would the array be passed in fully at once or would the values also be pushed one by one or both ?
<FL4SHK>
so, today I'm starting my CPU's implementation
<adamgreig[m]>
I'd expect to support either
<d1b2>
<Olivier Galibert> I'd recommend naming (with strings) the known crcs
<d1b2>
<Olivier Galibert> strings or field names
<adamgreig[m]>
Yea, can do. I'm not sure what the copyright status of crc parameters is though
<whitequark>
we should definitely have a one-by-one API
<adamgreig[m]>
Or initial_crc... I think init is probably fine,
<adamgreig[m]>
?*
<whitequark>
or initial_crc, I think that fits better with all the other names which have no shortening
<whitequark>
let's go with initial_crc :)
<d1b2>
<Olivier Galibert> So we push for using EnableInserter/ResetInserter on the domain, or is that for when domains are a little more usable in that area?
<adamgreig[m]>
Sure, initial_crc
<adamgreig[m]>
I think the plan was to keep explicit reset and valid signals on it for now
<whitequark>
yes, for the CRC module it's clearly fine
<adamgreig[m]>
Since you'll need them every time you use it it seems very roundabout to require wrapping it twice
<whitequark>
the valid signal needs to be explicit so that it's easily converted into a stream, anyway
<whitequark>
it's only about the reset signal
<whitequark>
actually, now that I'm thinking about it, isn't reset conceptually a part of the input stream?
<adamgreig[m]>
Yea
<whitequark>
you reset the CRC generator on the first word
<whitequark>
so you'd have it as first, valid, data
<adamgreig[m]>
It's a start of message delimiter really
<whitequark>
yes, it's not really a reset signal
<whitequark>
hm
<adamgreig[m]>
Reset still seems like the most obvious name for it to me when it's not actually plugged in to a stream but hm
<Chips4MakersakaS>
start ?
<jfng[m]>
first implies that it must be asserted at the same time as valid, for the first word of data ?
<whitequark>
I think reset and start have different semantics
<jfng[m]>
how about clear ?
<whitequark>
I like start
<adamgreig[m]>
I think reset is still accurate in terms of what it does and when you use it
<whitequark>
hmm
<whitequark>
when you assert reset, on that cycle, the data is ignored
<whitequark>
but if you assert start/first, the data is taken into account
<whitequark>
that's the difference in semantics
<d1b2>
<zyp> start sounds like a trigger
<whitequark>
well it kind of is?
<d1b2>
<zyp> I mean, a trigger to start something…
<whitequark>
it starts the process of CRC computation
<d1b2>
<zyp> otherwise I agree, reset is good if it's an independent signal, first is good if it arrives together with the first word
<whitequark>
adamgreig: basically, reset is a mux on the output of the algorithm and before the crc register, start is a mux on the input of the algorithm
<adamgreig[m]>
In the current implementation, reset loads the register with the initial value, ignoring anything on data
<whitequark>
yeah
<whitequark>
is that the most useful semantics we can provide?
<adamgreig[m]>
So yea, you couldn't just call it first and not change the impl
<whitequark>
it's trivial for the user to replicate the reset semantics using ResetInserter, but not trivial to replicate start semantics
<whitequark>
and if we're pushing for streams to be used, I think start/first semantics are super handy to have
<adamgreig[m]>
I don't see why we couldn't have a first-semantics instead where first+valid is meaningful
<adamgreig[m]>
And first without valid would be the same as currently
<whitequark>
oh yeah this sounds great to me
<whitequark>
modulo naming, I think calling it first if it's OK to use it without valid is kind of weird
<Chips4MakersakaS>
OK for me too.
<adamgreig[m]>
A lot of streams will have end instead of first I guess?
<WilliamDJones[m]>
There's no first signal in AXI stream
<jfng[m]>
so if we use first+valid, crc_reg could be reset_less in your implementation ?
<WilliamDJones[m]>
it's implied by either "first xaction after reset" or "first xaction after last"
<adamgreig[m]>
If you make it reset_less then the user can't wrap in a ResetInserter and get the right result
<whitequark>
^ yeah I don't think it should be reset_less
<whitequark>
in fact very few things should be reset_less, only things like synchrnizers
<whitequark>
it's a very visible feature that's also very niche
<adamgreig[m]>
Yea, in axi you'd probably want to reset on/after last, and that's probably true for most stream designs?
<d1b2>
<zyp> litex streams have both first and last
<whitequark>
our streams do not have to strictly follow AXI design
<WilliamDJones[m]>
I'm not saying don't include first, just that you'll need to do extra work to connect it to an AXI stream
<Chips4MakersakaS>
Allowing reset_less gives area advantages for ASICs
<whitequark>
Staf: it also gives area advantages for FPGAs in certain cases
<whitequark>
but if you're doing fine grained optimizations like that I don't think you should be doing it in the source HDL
<whitequark>
write a toolchain script (yosys or tcl or whatever) to remove reset from the signals you want, if its not enough to simply disconnect the reset
<adamgreig[m]>
Yea, just add a ResetInserter and leave it disconnected :p
<whitequark>
and yeah you can always make the domain reset_less
<whitequark>
adamgreig: ResetInserter ORs with all other resets
<Chips4MakersakaS>
I don't see how you can do that if all signal depends on not being reset_less ?
<whitequark>
I don't understand, but it's also not relevant to the topic
<whitequark>
we can discuss this after the meeting
<whitequark>
adamgreig: so let's provisionally name that "first" and leave the discussion for the next time, and I'll give some thought to streams design (I should be done with interfaces RFC by the end of this week)
<whitequark>
there are still naming questions here we should go through
<adamgreig[m]>
Ok
Guest25 has joined #amaranth-lang
<whitequark>
we have three classes: parameter class, hardware impl, software impl, and two methods that instantiate hardware and software impls
<whitequark>
since in the other places I'm avoiding the proliferation of lib.foo.FooSomething
<adamgreig[m]>
Hmm but lib.fifo.Async? :P
<whitequark>
legacy from migen
<whitequark>
like many other things from migen it has multiple issues
<whitequark>
oh, wait, I misread
<whitequark>
hm, yes, fifo.Async would be weird
<adamgreig[m]>
My usual instinct is to keep Crc in the name because I'd be importing the class and then using its name directly
<adamgreig[m]>
But if you're generally avoiding that then your suggestions seem fine
<whitequark>
so the lib.data module is designed so that you do from amaranth.lib import data
<whitequark>
and then do data.StructLayout, data.View, and so on
<whitequark>
it makes the people who heavily namespace everything happy, it reads fine, it makes the implementation reads a little nicer too
<adamgreig[m]>
I'd probably add Parameters.compute() that just takes a whole input and returns the output directly
<whitequark>
I'm planning to do the same with interfaces library (names tbd)
<whitequark>
and we have the same in amaranth-soc
<whitequark>
where every type of interface is just class Interface and your'e expected to import the whole module
<whitequark>
honestly, importing the whole module is good just because otherwise you have people insisting on importing every name individually and you have a from x import A, Million, Things, Over, Multiple, Lines in the beginning of file, which is aggravating
<adamgreig[m]>
Yea, that's fair, happy to keep Crc out of the names
<whitequark>
if your IDE cannot handle import *, which is trivial, the problem is your IDE
<whitequark>
(the objection that it's hard for people to handle import * at least has factual basis under it, the objection that IDEs can't handle it has no factual basis)
<adamgreig[m]>
Tell that to flake8 sadly
<adamgreig[m]>
Anyway
<whitequark>
one of the many reasons I don't use flake8
<WilliamDJones[m]>
--ignore=E503
<whitequark>
yeah
<whitequark>
Parameters.compute sounds good to me
<WilliamDJones[m]>
* --ignore=F405
<whitequark>
takes an iterable with data_width-wide integers
<whitequark>
returns an integr
<whitequark>
* returns an integre
<whitequark>
* returns an integer
<whitequark>
anyone else has any input re RFC #6?
<d1b2>
<dave berkeley> Can I just say I'm using the CRC module in a packet framer and it works very well. Interesting discussion today. Glad it is going in.
<whitequark>
oh, adamgreig : can you put the class signatures in the RFC, please?
<whitequark>
I'm going to preface this by saying that the example from the "Motivation" section actually works now, and I wanted to vote against my own RFC even back when it didn't work
<whitequark>
however, in case someone is very deeply convinced this is the right way to go for Amaranth, here's your chance
<jfng[m]>
that example was the main reason why i cared about #7
<WilliamDJones[m]>
Nope, disposition to close for me. I've run into the `TypeError("Attempted to convert Amaranth value to Python boolean")` many times before (with most recent commit). But the Drawbacks seem too great. I'd rather have a better error message for those cases (which I should deal w/ on case-by-case).
<whitequark>
I also want to make clear that while I originally intended for the entire Cat(Funct.ADD, Op.REG) to be preserved, IIRC this wouldn't work even with #7 because of the way enums work
<whitequark>
so the current implementation, which hard-casts to int, is probably the best possible
<whitequark>
yes, I think the drawbacks are very significant
<jfng[m]>
Catherine: the fact that the reset_less signal is valid iff a strobe is high would then be part of the interface "contract"
<whitequark>
oh yeah, it is, there's no disagreement there
<whitequark>
it's just that "your design's state is fully known after a reset" is a property useful for verification regardless of whether the code uses the contract
<whitequark>
imagine you have a testbench that checks some sequence of inputs. if you have all signals reset_less=False, you know that after a reset and running this exact sequence of inputs, your design will be in the state it checks
<whitequark>
if you have some of them reset_less=True, you no longer know that
<d1b2>
<Olivier Galibert> Except for POR, right?
<jfng[m]>
yeah, except for POR
<jfng[m]>
Catherine: the assumption i make is that the input signals that alter the design state are not reset_less
<FL4SHK>
anyone have any suggestions for whether I should simulate within Amaranth itself, or instead with Verilator?
<FL4SHK>
I know that Verilator is much faster
<FL4SHK>
but I might not need Verilator directly
<FL4SHK>
wait, there was another simulator using yosys, right?
<FL4SHK>
that might be ideal
<jfng[m]>
i agree you lose the knowledge of the full design state out of reset, but that would only be the case for some registers whose value are don't care unless their strobe is asserted
<whitequark>
FL4SHK: there's CXXRTL (via Yosys) but its support is not yet merged in Amaranth
<whitequark>
jfng: there are two perspectives on this: small-scale and large-scale
<whitequark>
small-scale is "there are these strobes, they have a contract, if you're adhering to the contract everything is good"
<whitequark>
large-scale is "there is this pile of code written by who knows where and when and my job is to make it work reliably, even at the cost of resources"
<whitequark>
whether that is verification resources at design time or silicon resources afterwards
<FL4SHK>
whitequark: is it still possible to use CXXRTL?
<FL4SHK>
looks like you wrote cxxrtl.h
<FL4SHK>
neat
<FL4SHK>
perhaps for right now I actually could just use formal and simulating Python-side
<whitequark>
'still' possible? I don't understand what you mean
<FL4SHK>
no I just mean
<whitequark>
ohhh yeah you can check out the branch
<FL4SHK>
is it possible for me to use CXXRTL with the Verilog output of Amaranth
<FL4SHK>
oh
<whitequark>
oh, yes, of course
<FL4SHK>
oh, is there a branch of Amaranth that supports CXXRTL?
<FL4SHK>
because that sounds nice
<whitequark>
yes, it's called cxxsim
cr1901 has quit [Read error: Connection reset by peer]
<whitequark>
in the main repo
<FL4SHK>
gotcha
<FL4SHK>
oh
<whitequark>
there are still issues and reporting them is very appreciated
<FL4SHK>
I would be happy to report them when I find them
<whitequark>
perfect
cr1901 has joined #amaranth-lang
<whitequark>
if I forget to rebase it on top of main please remind me, ideally during work hours
<FL4SHK>
when are work hours?
<FL4SHK>
I am in CST/CDT, so GMT-6 sometimes, and I think currently GMT-5
<whitequark>
UTC+1
<FL4SHK>
okay
<FL4SHK>
I suppose that would be either morning for me, and considering I'm about to transfer to fully working from home, I should have access to IRC more often soon
<FL4SHK>
...btw I am really looking forward to that transfer
<whitequark>
good luck
<FL4SHK>
thank
<FL4SHK>
does that branch let you use Views?
<FL4SHK>
I would guess not
<FL4SHK>
since the last commit was in 2021
<whitequark>
yes
<whitequark>
needs rebase
<whitequark>
let me see if it can be rebased cleanly
<FL4SHK>
sounds good
<cr1901>
(testing IRC again) So add_process in the simulator is equivalent to and "always begin ... end" process, and add_sync_process is equivalent to "always @(posedge clk) begin ... end" process?
<cr1901>
If so, does Amaranth simulator have a way to do "always @(sensitivity_list) begin ... end" (no posedge/negedge)?
<whitequark>
nope but you can add the same process twice
<cr1901>
how does that help?
<whitequark>
with different domains, I mean
<cr1901>
I don't want a process to be executed in response to an edge though; I want a process to be executed in response to a signal level change.
<cr1901>
If you give me a moment, I can paste some code
<whitequark>
a level can only change 0->1 or 1->0, right? so handling both of those is equivalent to handling a level change
<whitequark>
but I think you might be asking for something else
<whitequark>
which is replacing some comb statement with a function (as opposed to replacing some sync expression with a function, like add_sync_process does)
<WilliamDJones[m]>
Yes, that is what I want. add_process isn't sufficient unless I manually add yield or yield Delay
<WilliamDJones[m]>
(Oops, this is what I get for having both clients open at once)
<whitequark>
there will be an add_comb_process, yes
<whitequark>
probably together with the new simulator interface
<WilliamDJones[m]>
This infinitely loops because, well add_process always runs, and this process doesn't consume any time (also I forgot Settle)
<whitequark>
you can emulate it right now with yield Settle
<WilliamDJones[m]>
Where should I put Settle to make this not infinitely loop?
<whitequark>
oh, wait. you cannot
<whitequark>
yeah sorry
<WilliamDJones[m]>
Yea, my impression was that Settle doesn't take time
<whitequark>
yeah
<whitequark>
you need a simulator command that doesn't exist yet
<whitequark>
and probably won't, considering that I think the whole yield story is bad design
<whitequark>
stay tuned for a new simulator design (using async functions) RFC
<WilliamDJones[m]>
Is what I want at a high level reasonable at least :P?
<whitequark>
it's required for my job at ChipFlow so it'll be done pretty soon
<whitequark>
yeah
<whitequark>
it's explicitly in scope
<WilliamDJones[m]>
Okay that's fine, I'll keep the nonoptimal sync testbench around until the RFC, and will update immediately to test the RFC out when ready
<WilliamDJones[m]>
(Btw, what's the difference between yield Tick(), yield Delay() and yield? Second one's obvious enough, but I didn't see docs for them
<whitequark>
bare yield is translated into yield Tick(domain) in add_sync_process
<WilliamDJones[m]>
Bare `yield` inside an `add_process`, AFAICT, is equivalent to "pass time until the next event". Assuming that that's meant to work at all :).
<WilliamDJones[m]>
* the next scheduled event, * event, do that process, and then go back to this process". Assuming
<whitequark>
IIRC bare yield in add_process is an error
<whitequark>
FL4SHK: rebased, let me know if everything works
<WilliamDJones[m]>
I might've done yield Tick() (sic). I don't remember exactly. So don't put too much stock into my recollection
<WilliamDJones[m]>
* I might've done yield Tick() (sic) in add_process. I don't remember exactly. So don't put too much stock into my recollection
<FL4SHK>
whitequark: sounds good
<WilliamDJones[m]>
If the plan is to replace the simulator soon, I don't think I should get too deeply acquainted w/ the parts of the current API I don't know.
<WilliamDJones[m]>
* adamgreig: https://github.com/adamgreig/amaranth-examples/blob/master/amaranth\_examples/comb\_test.py#L35-L50 I looked at this for a comb testbench example. To be clear, this does _not_ pass any simulation time, and therefore can't generate a (meaningful) VCD?
<_whitenotifier-f>
[amaranth-lang/amaranth-lang.github.io] whitequark 19518ef - Deploying to main from @ amaranth-lang/amaranth@8af90620c0f044b6a252da56957870544b57d7db 🚀