amaranth icon indicating copy to clipboard operation
amaranth copied to clipboard

`data.Const.__repr__` is very unhelpful for complex structures

Open whitequark opened this issue 1 year ago • 0 comments

For example, look at this assertion:

AssertionError: (cycle 0) Const(StructLayout({'port': StructLayout({'io0': StructLayout({'o': 1, 'oe': 1}), 'io1': StructLayout({'o': 1, 'oe': 1}), 'io2': StructLayout({'o': 1, 'oe': 1}), 'io3': StructLayout({'o': 1, 'oe': 1}), 'cs': StructLayout({'o': 1, 'oe': 1})}), 'i_en': 1, 'meta': <enum 'QSPIMode'>}), 1539) != Const(StructLayout({'port': StructLayout({'io0': StructLayout({'o': 1, 'oe': 1}), 'io1': StructLayout({'o': 1, 'oe': 1}), 'io2': StructLayout({'o': 1, 'oe': 1}), 'io3': StructLayout({'o': 1, 'oe': 1}), 'cs': StructLayout({'o': 1, 'oe': 1})}), 'i_en': 1, 'meta': <enum 'QSPIMode'>}), 1795)

It's completely incomprehensible. There are three issues:

  1. Information overload, with excess StructLayout everywhere. This is probably not solvable.
  2. Typing this back into a shell doesn't actually reproduce the value, because data.Const and Const have different argument order, and Const only accepts structured data anyway.
  3. The actual value is just a decimal number.

I think this needs at least two fixes:

  1. In general, change all amaranth.lib.* classes to prepend whatever * is to the repr. This matches our suggested import of from amaranth.lib import data, wiring, <etc> and isn't onerous, although it would make the "information overload" part surely worse.
  2. In this particular case, return hdl.Const and not a data.Const since the former casts a by-field decomposition back into a value, fulfilling both the repr contract, and being useful to read.
  3. Maybe ameliorating the info overload by adding newlines and indents to StructLayout and friends? It's kind of difficult to implement, but probably feasible, at least as an experiment.

whitequark avatar Jun 28 '24 21:06 whitequark