Instantiating constants of type DspComplex[DspReal]

[harrisonliew]

I believe there is an issue when constructing DspComplex constants with type DspReal versus FixedPoint. There is an inconsistency in the type checking.

If I declare a constant:
val constant = DspComplex(Complex(1.0, 0.0))

Using this constant with DspComplex[FixedPoint], but with DspComplex[DspReal], compilation errors out:
chisel3.core.Binding$ExpectedHardwareException: data to be connected 'chisel3.core.UInt@b88a65d' must be hardware, not a bare Chisel type. Perhaps you forgot to wrap it in Wire(_) or IO(_)?

So, digging deeper, I can instead do this:

//in a parameter trait, have this:
val proto : DspComplex[T]

And then declare constant as:

val constant = Wire(params.proto)
constant.real = ConvertableTo[T].fromDouble(1.0)
constant.imag = ConvertableTo[T].fromDouble(0.0)

However, this does not work for FixedPoint. I instead have to declare constant as:
val constant = Wire(params.proto.cloneType)

Does this have something to do with the fact that DspReal is a bundle instead of a literal?

[shunshou]

Ok, it's been a while since I looked at literals, and I know I was having problems with this a year ago...

Check out

dsptools/src/main/scala/dsptools/numbers/chisel_concrete/DspComplex.scala

Line 42 in 3b1391f

def apply[T <: Data:Ring:ConvertableTo](c: Complex): DspComplex[T] = {

If you wanted to do

val constant = DspComplex(Complex(1.0, 0.0))

You should be explicitly doing
val constant = DspComplex[DspReal](Complex(1.0, 0.0))

(or more generally, DspComplex[T] if you're passing T in at the module level)

There is an issue with Bundles not supporting Literals (at least with the version of Chisel/dsptools you're likely using), which might mean that you still need to do the wire(chiseldatatype) thing... And yes, this would be an issue both for DspReal and DspComplex.

If you find something that works that lines 42+ don't give you (such as adding a wire wrap inside the apply), you're welcome to submit a PR.

I'd probably change the contents of:

def apply[T <: Data:Ring:ConvertableTo](c: Complex): DspComplex[T] = {
    DspComplex(ConvertableTo[T].fromDouble(c.real), ConvertableTo[T].fromDouble(c.imag))
  }

to something like:

def apply[T <: Data:Ring:ConvertableTo](c: Complex): DspComplex[T] = {
    DspComplex.wire(ConvertableTo[T].fromDouble(c.real), ConvertableTo[T].fromDouble(c.imag))
  }

which essentially creates a DspComplex wire with a clone of the real/imaginary Chisel data types and manually assigns lits to them (as you did).

I believe val constant = Wire(params.proto.cloneType) should work for both DspReal and FixedPoint? And I think you're right on cloneType being needed for DspReal b/c its superclass is Bundle.

If the original version doesn't work. Note that if you use fromDouble in this case, you'll need to properly set the fromDouble default # of fractional bits from DspContext.

Otherwise, I'd use

dsptools/src/main/scala/dsptools/numbers/chisel_concrete/DspComplex.scala

Line 47 in 3b1391f

def proto[T <: Data:Ring:ConvertableTo](c: Complex, t: T): DspComplex[T] = {

instead, if you don't want to use DspContext.

[harrisonliew]

Thanks so much Angie. I realized I made a typo above and I did use
DspComplex[T](Complex(1.0,0.0))

Also, to clarify,
val constant = Wire(params.proto.cloneType) is necessary for FixedPoint but does not work for DspReal, which needs val constant = Wire(params.proto). I already specify my FixedPoint proto's # of fractional bits in a case class that extends the trait.

I will mess around with DspComplex.scala and see what can work for both cases.

[shunshou]

Ok it looks like when we made DspReal extend Bundle (see

dsptools/src/main/scala/dsptools/numbers/chisel_concrete/DspReal.scala

Line 9 in 3b1391f

class DspReal(val lit: Option[BigInt] = None) extends Bundle {

), we forgot to add a cloneType method... (again, I think this is only an issue w/ older versions of Chisel...). Example of cloneType required by a subclass o Bundle:

dsptools/src/main/scala/dsptools/numbers/chisel_concrete/DspComplex.scala

Line 74 in 3b1391f

override def cloneType: this.type = {

override def cloneType: this.type = {
    new DspReal(lit).asInstanceOf[this.type]
}

should do the trick...

Also, might as well ask: which branch of Chisel are you on and which branch of dsptools are you on?

[shunshou]

Just to summarize, if you add cloneType to DspReal:

override def cloneType: this.type = {
    new DspReal(lit).asInstanceOf[this.type]
}

And change the Lit apply in DspComplex:

def apply[T <: Data:Ring:ConvertableTo](c: Complex): DspComplex[T] = {
    DspComplex.wire(ConvertableTo[T].fromDouble(c.real), ConvertableTo[T].fromDouble(c.imag))
  }

You should be able to get a hacky version of DspComplex lit to work properly. I think once dsptools is upstreamed to take advantage of some of the more recent Chisel updates, this "hack" will no longer be necessary--i.e., once you take advantage of Lit-supporting bundles, you can do away with the extra wire creation step...

[harrisonliew]

So I'm using this for the 290C class project, and we have been using release snapshots. We had a 1.2-SNAPSHOT jar of rocket-dsptools from ~September. I just tested using the 1.2-102318-SNAPSHOT ivy package and found that indeed the cloneType is no longer an issue for the DspReal case as I guess Chisel was bumped in the last month or two.

The missing cloneType makes sense for why DspComplexT doesn't work for T of type DspReal. I'll try adding cloneType to DspReal after I clone the sources and point to that instead of the ivy package.

[shunshou]

lol yeah... things will get more stable in the future... hopefully, but it's great that you were able to get to these type nuances and figure stuff out :o definitely submit a PR to master after you test out the changes :)

[grebe]

Ideally, no wires are needed at all, but Bundles can't get literal bindings if they have bundles inside them. I made a PR last night to try to resume bundle literals- see this and this.