-
Notifications
You must be signed in to change notification settings - Fork 641
ChiselEnum convenience functions; support string interaction #5036
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…ne of the values that contains a specific substring
…d in external file instead of hardcoded inside chisel source code
… with their names
def allWithNames: Seq[(Type, String)] = all.zip(allNames) | ||
|
||
/** All Enum values with their names, printed one per line. Compatible with gtkwave filter file */ | ||
override def toString(): String = allWithNames.map(e => s"${e._1.litValue} ${e._2}").mkString("", "\n", "\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, don't add ()
here (that implies mutation in canonical Scala style).
More important discussion--I took a look at how this prints and I don't think this is the right .toString
.
For reference, for the following example:
object EnumExample extends ChiselEnum {
val e0, e1, e2 = Value
val e100 = Value(100.U)
val e101 = Value(101.U)
}
This toString
is:
0 e0
1 e1
2 e2
100 e100
101 e101
Old (which I agree is bad so no issues replacing) is:
chiselTests.EnumExample$@4cd7e993
Note that the .toString
for a specific value (e.g. EnumExample.e100
) is:
EnumExample(100=e100)
(I'd prefer if this order were swapped, e100=100
, I think it's backwards but oh well).
I think the "right" .toString would be the String concatenation of all of the values:
EnumExample(0=e0, 1=e1, 2=e2, 100=e100, 101=e101)
That would be more consistent with normal Scala toStrings (which typically do not include newlines).
I think this functionality of printing the the gtkwave compatible style is good but belongs in a method on DataMirror
. Maybe something like DataMirror.gtkwaveEnumMapping
or something, what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The .toString method now does what you suggested (+ test added). I agree this makes sense.
I moved the previous functionality into a new method called asTable
. I think we should not explicitly have gtkwave in any of the chisel method names. Gtkwave is just one of the waveform viewers out there.
In our environment, the Enum mappings are also parsed by other tools (simulation environment) - not just gtkwave. We chose the gtkwave filter format because we see no need to invent "yet another format".
I do like the asTable method to be a member of ChiselEnum
, not hide it in DataMirror
. I think dumping the ChiselEnum mappings in a machine-readable way is a pretty generic thing.
…nameContains(String)
… for contains, so let's tolerate nameContains too. I don't want scalafmt to make the test code harder-to-read
…rsed by external programs
…and change test code accordingly
Contributor Checklist
docs/src
?Type of Improvement
toString
method onChiselEnum
. I wonder if anybody out there depends on the current behavior. If so, we could rename tolistValues
Desired Merge Strategy
Release Notes
Convenience functions to interact between ChiselEnum and String. Allows RTL readability improvements, especially if Enum values are not yet known at compile time (e.g. decode tables in separate files)
Reviewer Checklist (only modified by reviewer)
3.6.x
,5.x
, or6.x
depending on impact, API modification or big change:7.0
)?Enable auto-merge (squash)
and clean up the commit message.Create a merge commit
.