CRC generator

[adamgreig]

See amaranth-lang/amaranth#681 for implementation.

Rendered.

[whitequark]

Thank you, this is an excellent RFC!

[whitequark]

The only concern I actually have is the naming for the reset signal: rst vs reset. Right now within Amaranth itself we never use reset to name a reset signal.

[adamgreig]

Thanks for looking over it! I'm happy to rename reset to rst in the implementation PR. Shall I wait for the next meeting to address anything else that comes up then?

[whitequark]

Shall I wait for the next meeting to address anything else that comes up then?

Yep, sounds reasonable.

[whitequark]

We've discussed this PR on the 2023-05-15 weekly meeting. We ran out of time and did not arrive on a disposition. However, the following change was agreed on:

• Rename reset parameter to rst.

[whitequark]

We've discussed this RFC on the 2023-05-22 weekly meeting. Several changes to the RFC were proposed, namely:

• splitting off parameters into a separate class
• enabling software RFC computation
• renaming most entities here (classes to crc.Parameters, crc.Processor, crc.SoftwareProcessor, parameter names to data_width, crc_width, initial_crc, polynomial, reflect_input, reflect_output, methods on crc.SoftwareProcessor to update and properties on it to crc, methods on crc.Parameters to create, create_software, and compute)

[adamgreig]

I've updated the RFC text to address the new design in amaranth-lang/amaranth#681. I also expanded the guide section significantly and detailed the full API in the reference section.

Compared to what we discussed in the 2023-05-22 meeting:

• I no longer have a specific software class. I originally wrote one, but it ended up just being a single compute() method that might as well be inlined into Parameters. There's no state stored in the class.
• This does mean it's not possible to perform incremental CRC computations, which was briefly discussed in the meeting. On reflection I'm not sure it's very valuable in Python where it's easy to create the full data buffer and then compute the CRC, but we could add this capability in the future if we wanted as it would require a new API anyway.
• I've added the new Predefined class because the catalog can't know what data_width you want to use in your specific implementation; it's not really part of the CRC algorithm description. We could just use Parameters and expect users to update the data_width if they want something different to 8 bits, instead.
• I've added the new Catalog class with Predefined attributes. I think this leads to a relatively neat API compared to using a dictionary with string keys, but perhaps it's a bit unusual to have a class you never construct and just access class attributes of. I don't think we agreed on a particular way to store the catalogue of predefined values in the meeting, but I'm happy to change this if there's a better way.
• I've made the change to reset so that it functions as first, but this introduces a mux between crc_reg and initial_crc that's not there if you don't need to support having first and valid asserted at the same time. Maybe that's a worthwhile cost. I'm not sure if we can tweak the logic to not require the extra mux.

[whitequark]

Thanks! I'll review this as soon as I have time.

[whitequark]

We have discussed this RFC on the 2023-05-29 weekly meeting. The decision was to merge with the following changes:

• Move the Catalog class contents into an amaranth.lib.crc.catalog module;
• Rename Predefined to Algorithm;
• Accept an algorithm argument in Parameters;
• Drop Algorithm.check and Algorithm.residue, as well as Parameters.check;
• Rename Processor.first to start, while keeping existing semantics.

Once again, thanks @adamgreig for the work on this RFC and the excellent addition to Amaranth it will make!

[adamgreig]

I've updated this RFC text and the corresponding PR implementation to address the latest changes.

The only slightly unanticipated change is that to split Catalog into a new module, I also had to move Algorithm, Parameters, and Processor into a new private lib.crc._crc module and export them from lib.crc to allow lib.crc to also export lib.crc.catalog without creating a circular dependency.

We discussed in the meeting having the parameters to crc.Algorithm all become named, which I have also implemented.

[whitequark]

The only slightly unanticipated change is that to split Catalog into a new module, I also had to move Algorithm, Parameters, and Processor into a new private lib.crc._crc module and export them from lib.crc to allow lib.crc to also export lib.crc.catalog without creating a circular dependency.

I'm fairly sure this isn't necessary? In my original suggestion I did not plan on having lib.crc export catalog, but if you want to, here's how you can do it:
crc.zip

[adamgreig]

Indeed, I was avoiding the bottom-of-module import as it's a bit irregular but it does avoid needing a separate module. I feel like exporting catalog is probably useful but perhaps most users will just import an Algorithm directly from crc.catalog and use that without any other imports.

I'm happy to go any way on this, what's your preference?

[whitequark]

Indeed, I was avoiding the bottom-of-module import as it's a bit irregular but it does avoid needing a separate module.

It's much better than a separate module! I think in a few places I import in a function. It's OK. It's normal Python.

but perhaps most users will just import an Algorithm directly from crc.catalog and use that without any other imports.

That's what I was thinking. I have no opinion on whether amaranth.lib.crc should export catalog as a field in addition to a submodule; feel free to use your preference.

[adamgreig]

OK, I've updated the implementation PR to move the catalog import to the bottom of __init__.py and moved the classes back into __init__.py.

[whitequark]

@adamgreig Thanks! I'm going to merge this after one more lookover.

[whitequark]

Merged! Once more, thank you for all the work that went into CRC and addressing the review comments.