Rewrite BlackBox IO contract, replace _clock|_reset

The old blackbox behavior still emitted extmodules that have a
clk, reset pin and prepended all io's with io_ (ultimately). Most
verilog modules do not follow this distinction (or use a slightly
different name for clock and so on).

Thus, instead BlackBox has been rewritten to not assume a clk or
reset pin. Instead, the io Bundle specified is flattened directly
into the Module.ports declaration. The tests have been rewritten
to compensate for this. Also, added a test that uses the clock pin.

As a secondary change, the _clock and _reset module parameters were
bad for two reasons. One, they used null as a default, which is a
scala best practices violation. Two, they were just not good names.

Instead the primary constructor has been rewritten to take an
Option[Clock] called override_clock and an Option[Bool] called
override_reset, which default to None. (Note how the getOrElse call
down below is much more natural now.)

However, users may not want to specify the Some(their_clock) so I
also added secondary constructors that take parameters named clock
and reset and wrap them into Some calls into the primary constructor.
This is a better UX because now you can just stipulate clock=blah in
instantiation of that module in symmetry with using the clock in the
definition of the module by invoking clock.

PS: We could also back out of allowing any overrides via the Module
constructor and just require the instantiating Module to do
submodule.clock := newclock, etc.

5 files changed, 137 insertions, 29 deletions

diff --git a/src/main/scala/Chisel/BlackBox.scala b/src/main/scala/Chisel/BlackBox.scala
index ae0c59ba..48887271 100644
--- a/src/main/scala/Chisel/BlackBox.scala
+++ b/src/main/scala/Chisel/BlackBox.scala
@@ -2,6 +2,9 @@
 
 package Chisel
 
+import internal.Builder.pushCommand
+import internal.firrtl.{ModuleIO, DefInvalid}
+
 /** Defines a black box, which is a module that can be referenced from within
   * Chisel, but is not defined in the emitted Verilog. Useful for connecting
   * to RTL modules defined outside Chisel.
@@ -12,10 +15,37 @@ package Chisel
   * }}}
   */
 // REVIEW TODO: make Verilog parameters part of the constructor interface?
-abstract class BlackBox(_clock: Clock = null, _reset: Bool = null)
-    extends Module(_clock = _clock, _reset = _reset) {
+abstract class BlackBox extends Module {
+  // Don't bother taking override_clock|reset, clock/reset locked out anyway
   // TODO: actually implement this.
   def setVerilogParameters(s: String): Unit = {}
 
   // The body of a BlackBox is empty, the real logic happens in firrtl/Emitter.scala
+  // Bypass standard clock, reset, io port declaration by flattening io
+  // TODO(twigg): ? Really, overrides are bad, should extend BaseModule....
+  override private[Chisel] def ports = io.elements.toSeq
+
+  // Do not do reflective naming of internal signals, just name io
+  override private[Chisel] def setRefs(): this.type = {
+    for ((name, port) <- ports) {
+      port.setRef(ModuleIO(this, _namespace.name(name)))
+    }
+    io.setRef("") // don't io parts prepended with io_
+    this
+  }
+
+  // Don't setup clock, reset
+  // Cann't invalide io in one bunch, must invalidate each part separately
+  override private[Chisel] def setupInParent(): this.type = _parent match {
+    case Some(p) => {
+      // Just init instance inputs
+      for((_,port) <- ports) pushCommand(DefInvalid(port.ref))
+      this
+    }
+    case None => this
+  }
+
+  // Using null is horrible but these signals SHOULD NEVER be used:
+  override val clock = null
+  override val reset = null
 }
diff --git a/src/main/scala/Chisel/Module.scala b/src/main/scala/Chisel/Module.scala
index cfcf5a48..738863e3 100644
--- a/src/main/scala/Chisel/Module.scala
+++ b/src/main/scala/Chisel/Module.scala
@@ -25,8 +25,7 @@ object Module {
     val ports = m.computePorts
     Builder.components += Component(m, m.name, ports, m._commands)
     pushCommand(DefInstance(m, ports))
-    pushCommand(DefInvalid(m.io.ref)) // init instance inputs
-    m.connectImplicitIOs()
+    m.setupInParent()
   }
 }
 
@@ -36,8 +35,14 @@ object Module {
   *
   * @note Module instantiations must be wrapped in a Module() call.
   */
-abstract class Module(_clock: Clock = null, _reset: Bool = null) extends HasId {
-  private val _namespace = Builder.globalNamespace.child
+abstract class Module(
+  override_clock: Option[Clock]=None, override_reset: Option[Bool]=None)
+extends HasId {
+  def this(clock: Clock) = this(Some(clock), None)
+  def this(reset: Bool)  = this(None, Some(reset))
+  def this(clock: Clock, reset: Bool) = this(Some(clock), Some(reset))
+
+  private[Chisel] val _namespace = Builder.globalNamespace.child
   private[Chisel] val _commands = ArrayBuffer[Command]()
   private[Chisel] val _ids = ArrayBuffer[HasId]()
   dynamicContext.currentModule = Some(this)
@@ -54,27 +59,29 @@ abstract class Module(_clock: Clock = null, _reset: Bool = null) extends HasId {
 
   private[Chisel] def addId(d: HasId) { _ids += d }
 
-  private def ports = (clock, "clk") :: (reset, "reset") :: (io, "io") :: Nil
+  private[Chisel] def ports: Seq[(String,Data)] = Vector(
+    ("clk", clock), ("reset", reset), ("io", io)
+  )
 
-  private[Chisel] def computePorts = ports map { case (port, name) =>
+  private[Chisel] def computePorts = for((name, port) <- ports) yield {
     val bundleDir = if (port.isFlip) INPUT else OUTPUT
     Port(port, if (port.dir == NO_DIR) bundleDir else port.dir)
   }
 
-  private def connectImplicitIOs(): this.type = _parent match {
-    case Some(p) =>
-      clock := (if (_clock eq null) p.clock else _clock)
-      reset := (if (_reset eq null) p.reset else _reset)
+  private[Chisel] def setupInParent(): this.type = _parent match {
+    case Some(p) => {
+      pushCommand(DefInvalid(io.ref)) // init instance inputs
+      clock := override_clock.getOrElse(p.clock)
+      reset := override_reset.getOrElse(p.reset)
       this
+    }
     case None => this
   }
 
-  private def makeImplicitIOs(): Unit = ports map { case (port, name) =>
-  }
-
-  private def setRefs(): this.type = {
-    for ((port, name) <- ports)
+  private[Chisel] def setRefs(): this.type = {
+    for ((name, port) <- ports) {
       port.setRef(ModuleIO(this, _namespace.name(name)))
+    }
 
     // Suggest names to nodes using runtime reflection
     val valNames = HashSet[String](getClass.getDeclaredFields.map(_.getName):_*)
diff --git a/src/main/scala/Chisel/util/Decoupled.scala b/src/main/scala/Chisel/util/Decoupled.scala
index faee2a5f..6c7787f8 100644
--- a/src/main/scala/Chisel/util/Decoupled.scala
+++ b/src/main/scala/Chisel/util/Decoupled.scala
@@ -104,8 +104,11 @@ class QueueIO[T <: Data](gen: T, entries: Int) extends Bundle
 class Queue[T <: Data](gen: T, val entries: Int,
                        pipe: Boolean = false,
                        flow: Boolean = false,
-                       _reset: Bool = null) extends Module(_reset=_reset)
-{
+                       override_reset: Option[Bool] = None)
+extends Module(override_reset=override_reset) {
+  def this(gen: T, entries: Int, pipe: Boolean, flow: Boolean, reset: Bool) =
+    this(gen, entries, pipe, flow, Some(reset))
+  
   val io = new QueueIO(gen, entries)
 
   val ram = Mem(entries, gen)
diff --git a/src/test/resources/BlackBoxTest.v b/src/test/resources/BlackBoxTest.v
index e57d2852..910b09ff 100644
--- a/src/test/resources/BlackBoxTest.v
+++ b/src/test/resources/BlackBoxTest.v
@@ -1,17 +1,34 @@
 module BlackBoxInverter(
-    input  [0:0] clk,
-    input  [0:0] reset,
-    output [0:0] io_in,
-    output [0:0] io_out
+    input  [0:0] in,
+    output [0:0] out
 );
-  assign io_out = !io_in;
+  assign out = !in;
 endmodule
 
 module BlackBoxPassthrough(
-    input  [0:0] clk,
-    input  [0:0] reset,
-    output [0:0] io_in,
-    output [0:0] io_out
+    input  [0:0] in,
+    output [0:0] out
 );
-  assign io_out = io_in;
+  assign out = in;
+endmodule
+
+module BlackBoxRegister(
+    input  [0:0] clock,
+    input  [0:0] in,
+    output [0:0] out
+);
+  reg [0:0] register;
+  always @(posedge clock) begin
+    register <= in;
+  end
+  assign out = register;
+endmodule
+
+module BlackBoxConstant #(
+  parameter int WIDTH=1,
+  parameter int VALUE=1
+) (
+  output [WIDTH-1:0] out
+);
+  assign out = VALUE;
 endmodule
diff --git a/src/test/scala/chiselTests/BlackBox.scala b/src/test/scala/chiselTests/BlackBox.scala
index 574a6335..ca94087c 100644
--- a/src/test/scala/chiselTests/BlackBox.scala
+++ b/src/test/scala/chiselTests/BlackBox.scala
@@ -21,6 +21,14 @@ class BlackBoxPassthrough extends BlackBox {
   }
 }
 
+class BlackBoxRegister extends BlackBox {
+  val io = new Bundle() {
+    val clock = Clock().asInput
+    val in = Bool(INPUT)
+    val out = Bool(OUTPUT)
+  }
+}
+
 class BlackBoxTester extends BasicTester {
   val blackBoxPos = Module(new BlackBoxInverter)
   val blackBoxNeg = Module(new BlackBoxInverter)
@@ -56,6 +64,45 @@ class MultiBlackBoxTester extends BasicTester {
   stop()
 }
 
+class BlackBoxWithClockTester extends BasicTester {
+  val blackBox = Module(new BlackBoxRegister)
+  val model = Reg(Bool())
+
+  val (cycles, end) = Counter(Bool(true), 15)
+
+  val impetus = cycles(0)
+  blackBox.io.clock := clock
+  blackBox.io.in := impetus
+  model := impetus
+
+  when(cycles > UInt(0)) {
+    assert(blackBox.io.out === model)
+  }
+  when(end) { stop() }
+}
+
+/*
+// Must determine how to handle parameterized Verilog
+class BlackBoxConstant(value: Int) extends BlackBox {
+  val io = new Bundle() {
+    val out = UInt(width=log2Up(value)).asOutput
+  }
+  override val name = s"#(WIDTH=${log2Up(value)},VALUE=$value) "
+}
+
+class BlackBoxWithParamsTester extends BasicTester {
+  val blackBoxOne  = Module(new BlackBoxConstant(1))
+  val blackBoxFour = Module(new BlackBoxConstant(4))
+
+  val (cycles, end) = Counter(Bool(true), 4)
+
+  assert(blackBoxOne.io.out  === UInt(1))
+  assert(blackBoxFour.io.out === UInt(4))
+
+  when(end) { stop() }
+}
+*/
+
 class BlackBoxSpec extends ChiselFlatSpec {
   "A BlackBoxed inverter" should "work" in {
     assertTesterPasses({ new BlackBoxTester },
@@ -65,4 +112,8 @@ class BlackBoxSpec extends ChiselFlatSpec {
     assertTesterPasses({ new MultiBlackBoxTester },
         Seq("/BlackBoxTest.v"))
   }
+  "A BlackBoxed register" should "work" in {
+    assertTesterPasses({ new BlackBoxWithClockTester },
+        Seq("/BlackBoxTest.v"))
+  }
 }