PeekPokeTester doesn't work with StrongEnum

[nhynes]

Type of issue: bug report

Impact: API addition (no impact on existing code)

Development Phase: request

Chisel version: 3.2-SNAPSHOT

Error messages:

poke:

[error] StateMachineTest.scala:42:00: overloaded method value poke with alternatives:
[error]   (signal: chisel3.core.Aggregate,value: IndexedSeq[BigInt])Unit <and>
[error]   (signal: chisel3.Bundle,map: Map[String,BigInt])Unit <and>
[error]   (signal: chisel3.Bits,value: Long)Unit <and>
[error]   (signal: chisel3.Bits,value: Int)Unit <and>
[error]   (signal: chisel3.Bits,value: BigInt)Unit <and>
[error]   (path: String,value: Long)Unit <and>
[error]   (path: String,value: Int)Unit <and>
[error]   (path: String,value: BigInt)Unit
[error]  cannot be applied to (MyStrongEnum.Type, MyStrongEnum.Type)
[error]   poke(dut.io.state, MyStrongEnum.Idle)

expect:

[error] StateMachineTest.scala:42:00: overloaded method value expect with alternatives:
[error]   (signal: chisel3.Bundle,expected: Map[String,BigInt])Boolean <and>
[error]   (signal: chisel3.core.Aggregate,expected: IndexedSeq[BigInt])Boolean <and>
[error]   (signal: chisel3.Bits,expected: BigInt,msg: => String)Boolean <and>
[error]   (good: Boolean,msg: => String)Boolean
[error]  cannot be applied to (MyStrongEnum.Type, MyStrongEnum.Type)
[error]   expect(dut.io.state, MyStrongEnum.Idle)

Internally, the StrongEnum Value items are assigned UInts, but there's no way of accessing these from the outside world.

[edwardcwang]

@hngenc @ducky64

[seldridge]

I haven't looked at this, but is converting to UInt for the poke or expect with .asUInt an option / workaround?

[nhynes]

poke using .U or expect with .asUInt

@seldridge That would have been a nice fix, but that produces the following error:

[info]   chisel3.internal.ChiselException: Error: Not in a UserModule.
Likely cause: Missed Module() wrap, bare chisel API call, or attempting to
construct hardware inside a BlackBox.
[info]   at chisel3.internal.throwException$.apply(Error.scala:13)
[info]   at chisel3.internal.Builder$.forcedUserModule(Builder.scala:227)
[info]   at chisel3.internal.Builder$.pushOp(Builder.scala:259)
[info]   at chisel3.core.EnumType.do_asUInt(StrongEnum.scala:73)

Also, using reflection to change the runtime type or get at the internal UInt field also doesn't work.

I guess a workaround workaround would be to use a BasicTester which would allow constructing hardware, but that would make peeking and poking a bit more difficult.

[hngenc]

I think I can get a fix for this out in the next day or two. I'm not sure I understand what you mean by the enum's internal UInt field; enums aren't composed of other Chisel objects, although they do get translated to UInts when going from Chisel to FIRRTL.

[nhynes]

This is what I meant by "interal UInt", but I also don't have a great mental model of what happens during elaboration.

Thanks for the fix!

[hngenc]

Actually, looking at this problem again, I'm unsure which of the possible solutions to implement. None of them seem ideal to me.

The problem is that peek, poke, pokeAt, etc. all expect inputs of type Bits, but enums aren't subclassed from Bits.

1. We can make enums inherit from Bits instead of inheriting directly from Element. I don't like this solution, because Bits has operations like << and toBool that I don't think make sense for enumerations.
2. We can create a new Pokeable typeclass for both Bits and EnumType in chisel-testers. Maybe this is the Scala-y way.
3. We can change peek, poke, etc. to accept arguments of type Element. However, users will then be able to poke Analog, Clock, etc. and I don't know if we want to permit that.
4. We can overload poke, peek, etc. to accept arguments of type EnumType as well. There will be some hacky code duplication.

I'm leaning towards either making enums inherit from Bits, or creating a Pokeable type class. I'm just not sure which.

[nhynes]

Proposal 2 sounds reasonable if Pokeable subsumes conversion to an expectable type. Proposal 4 would also be fine if using a macro to generate the impls for both; this is the Rust/C++-y way. If it were up to me, I'd just use a macro.

Regarding the other approaches, Proposal 1 would be simple to implement, but feels like it defeats the purpose of using a type system. Perhaps an explicit conversion to UInt would be more appropriate? Proposal 3 is neat and would enable a few interesting use cases, but it would require a lot code changes to support testing enums!

[edwardcwang]

Building off Nick's comment - could we add a Pokeable trait that Bits, Element, etc can mix in?

[hngenc]

A conversion from EnumType to UInt would be great, but I can't figure out how to do it outside of a Module. Maybe somebody who's more familiar with the Chisel codebase will know, if its even possible.

@edwardcwang I considered that as well. As I understand it, we'd have to mix in the trait in chisel3, but it wouldn't have any meaning except in chisel-testers which is a different project. That feels like a coding sin to me, but it would be a simple solution.

[hngenc]

@nhynes If you need enum testing support right away, then you can pull this branch: https://github.com/hngenc/chisel-testers/tree/add_enum_support. I haven't tested it extensively, but it seems to work for examples like poke(dut.io.state, MyStrongEnum.Idle). If the reviewers are fine with my changes, then I should be able to upstream this soon.