chipsalliance / chipsalliance/chisel
[RFC] What does it mean to be a Bundle?
- Dominant language
- Scala
- Stars
- 4.8k
- Forks
- 658
- Avg merge
- 18h 59m
- Merged PRs (30d)
- 14
Description
The standard pattern for using Chisel Data objects as generator parameters in Bundles is to pass them as a constructor argument like so:
```scala
class MyBundle[T <: Data](gen: T) extends Bundle {
val foo = Input(gen)
}
```
In the modern world of autoclonetype; however, you are encouraged to make constructor arguments `vals` so that reflection can find them. If you make `gen` a `val`:
```scala
class MyBundle[T <: Data](val gen: T) extends Bundle {
val foo = Input(gen)
}
```
Whoops, you've accidentally added a new element to your Bundle called `gen`! This is due to the fact that the elements of a Bundle are defined (by implementation) to be the public vals of type `Data` or `Some[Data]`.
Instead, you should use a private val:
```scala
class MyBundle[T <: Data](private val gen: T) extends Bundle {
val foo = Input(gen)
}
```
While a little verbose this allows autoclonetype to automatically generate clonetype so everyone is happy.
This led to a discussion on what the fundamental definition of a Bundle is/should be. As far as I can tell there are two possibilities:
### 1. Bundle Elements are the public vals of the Scala class (the status quo)
As far as I know, this hasn't been much a point of confusion for new users; however, with autoclonetype encouraging people to make their constructor parameters `vals` it most certainly could become one. To make this better, I think there are a couple of things we should do:
1. Write a wiki page on the definition of a Bundle with examples
1. When a `chisel3.core.Binding$MixedDirectionAggregateException` caused by a val constructor parameter of type `Data` or `Some[Data]`, inform the user that it counts as an element and link to the wiki page.
1. Augment autoclonetype to detect elements that are also constructor parameters and implicitly call `cloneType` on them. This is needed since these constructor arguments are not just generator parameters, they belong to the Bundle so we need fresh objects.
* Added bonus of this is it should enable single-line case class Bundles
There is one case where I'm not sure if we'll be able to [easily] provide a decent error message though. For a Bundle with no directions at all, you can end up with a `firrtl.passes.CheckInitialization$RefNotInitializedException` if you have a val constructor parameter of type `Data` that you didn't intend to be an element of the Bundle.
### 2. Bundle Elements should be vals in the *body* of a Bundle
It is a keen observation that there are very few examples of people using val constructor parameters to create fields of Bundles (I only know of [one](https://github.com/ucb-bar/dsptools/blob/master/src/main/scala/dsptools/numbers/chisel_concrete/DspComplex.scala#L59) h/t @grebe). Perhaps it is better to redefine Bundles such that constructor arguments of type `Data` or `Some[Data]` are not elements, rather they are just constructor parameters.
This has the nice property of encouraging separation of generator parameters and elements of the Bundle, and encourages people to express Bundles in a way similar to a C-struct. Misuse (eg. connecting to a val that is a constructor parameter) is also very easy to detect and provide a specific error message. As far as I know there aren't any obvious cases we can't catch like the one above. That being said this kind of a change would require carefully considering its implications on inheritance (especially overriding elements that may be a constructor parameters for a parent class)
I think there are other benefits as well, but I'm blanking so maybe @ducky64 can comment with some thoughts.
In any case, I think we should stuck with 1. but after some discussion in a meeting we thought we ought to ask the community to see what users think!
* **Type of issue**
- [ ] Bug report
- [ ] Feature request
- [x] Other enhancement
* **What is the use case for changing the behavior?**
See 2. above.
* **Impact**
- [ ] no functional change
- [x] API addition (no impact on existing code)
- [x] API modification
- [ ] unknown
* **Development Phase**
- [x] request
- [ ] proposal
Contributor guide
Assessment
This issue has not been assessed yet.