poke to Duplex Bundle IO

[sequencer]

In https://github.com/ucb-bar/chisel-testers2/blob/f25585375c6eb46a6db6f3f486badea259f73140/src/main/scala/chiseltest/package.scala#L17
tester2 poke signal is with the reflection of ActualDirection to avoid poking to a Output,
However if poking to a duplex Bundle, e.g. DecoupledIO, a Bundle literal cannot be used since it contain Input and Output.
So in Bundle behavior of DecoupledIO, I think it could remove this restriction, if ActualDirection is Output, just ignore this poking, return with out any action.

[sequencer]

BTW, I didn't see DataMirror in peek method, so it means I can peek a Input IO? that's sound not reasonable.

[ducky64]

Yes, you can peek an Input IO.

I'm not sure what the right way to resolve this is. I think the larger theme is how to handle partial bundles, related issue #108. This might be a special case of that - where instead of an arbitrarily partial bundle, you have a bundle with all sub-fields of some direction.

In the meantime, you can define your own methods that poke sub-fields. Decoupled uses this pattern and sidesteps the partial and mixed-directionality bundle problem - see DecoupledDriver.scala

[sequencer]

I think the larger theme is how to handle partial bundles

As for partial bundles connection, I think this actually has two issue to be done:

1. Record elements checking. If two Record.elements are not different: what's the correct way to do poke/poke?
   For the most simple way to peek/poke elements we can access is:

      case (x: Record, value: Record) => {
        // Two Record don't need to be equal
        // type equivalence should be located at Element poke
        (x.elements.keys.toSet intersect value.elements.keys.toSet)
          .filter(key => DataMirror.directionOf(x.elements(key)) != ActualDirection.Output)
          .foreach(key => x.elements(key).poke(value.elements(key)))
      }

2. Element type checking
   However when we are able to access sub-elements, we cannot guarantee chisel type.
   In currently implementation, we guarantee:
   2.1. ActualDirection of poke should be Input, and not check Direction of peek.
   2.2. allow poke: Bool <- Bool, Bits <- UInt, SInt <- SInt, FixedPoint <- FixedPoint, Interval <- Interval, EnumType <- EnumType, forbid poke Bool <- Bits, Bits <- SInt,
   But we didn't check the data width:
   for example it allows 100.U -> Bool since Bits <- UInt is allowed,
   I think Data.typeEquivalent should be reused in the peek/poke functions for more safer peek/poke(As it has already mentioned in comments)

[sequencer]

#151 provide a implementation to this. Will that a good approach to this?

[ducky64]

I generally prefer safer (but possibly more verbose semantics) over looser (but possibly faster), since debugging time is really expensive:

• Unless there's a compelling use case for poking Records of different types that can't be done through some other mechanism, I don't like discarding the elements check. I think Chisel's typeEquivalent is similarly strict. If you're merely trying to poke records that are merely structurally similar, you might use the implicit adapter pattern like done with DecoupledDriver to add the appropriate methods onto your Records of interest.
• I'm not a fan of allowing partial Bundle pokes, because it is potentially unsafe and possibly allows subtle state errors to slip through (missing fields aren't poked) and doesn't provide automated and noisy errors if you structurally refactor / modify your Bundle.
• I'm on the fence about partial Bundle pokes that fully define all fields of some directionality, mostly because otherwise poking those are impossible. I'd like to see example use cases for this that aren't better done with a implicit adapter pattern like done with DecoupledDriver.
• I think adding a literal - wire typeEquivalent check would make sense, though I think it needs to be exposed as a public operation in Chisel, eg through DataMirror.
• I think generalizing the Bundle poke to Records is fine. Ideally would include a simple test too.

[sequencer]

Unless there's a compelling use case for poking Records of different types that can't be done through some other mechanism, I don't like discarding the elements check. I think Chisel's typeEquivalent is similarly strict.
I think adding a literal - wire typeEquivalent check would make sense, though I think it needs to be exposed as a public operation in Chisel, eg through DataMirror.

I exposed typeEquivalent to DataMirror in chipsalliance/chisel#1402, and use this API in #151 for strict type checking.

I'm not a fan of allowing partial Bundle pokes, because it is potentially unsafe and possibly allows subtle state errors to slip through (missing fields aren't poked) and doesn't provide automated and noisy errors if you structurally refactor / modify your Bundle.
I'm on the fence about partial Bundle pokes that fully define all fields of some directionality, mostly because otherwise poking those are impossible. I'd like to see example use cases for this that aren't better done with a implicit adapter pattern like done with DecoupledDriver.

I think this API is required for the conflict IO like Diplomatic AutoBundle and TLBundle. Actually, in TLBundle, each time I only need to poke one channel like TLBundle.a, the partial connection will filter all Elements that isLit and the IO direction is Input. Thus, a simpler API can be provided to diplomatictester.

I think generalizing the Bundle poke to Records is fine. Ideally would include a simple test too.

After the scheme is confirmed, I'll add test for my PRs, currently my test is located at
https://github.com/sequencer/diplomatictester/blob/master/tests/src/TopWireSpec.scala#L79-L90
as you can see partial poking is a really helpful feature.

[sequencer]

#151 closed.