Provide ImplicitClock and ImplicitReset #3714
Conversation
There was a problem hiding this comment.
Would the following work (not sure why i would want to override the clock and reset when there is already a .clock and .reset, that seems like an anti-pattern?)
class MyRawModule extends RawModule with OverrideClock {
val myClock = IO(Input(Clock))
val myReset = IO(Input(Reset))
internalClock := myClock
withClockAndReset(Module.clock, myReset) {
???
}
}
In above example I'm trying to show a) it works for a RawModule and b) now even a RawModule has a Module.clock
|
I agree with making this apply to RawModule, that requires a bit more generalization but I think it's a good improvement to the codebase and API. It will make it possible for Module to not be "special" in that a user could create their own thing just like Module except with a different name for clock and reset, or including clock and reset but also with an automatic clock gate on the clock. |
jackkoenig
force-pushed
the
jackkoenig/override-clock-and-reset
branch
from
8704b92 to
39c7297
Compare
jackkoenig
changed the title
jackkoenig
force-pushed
the
jackkoenig/override-clock-and-reset
branch
from
d64f37b to
88212bf
Compare
| @@ -11,6 +11,7 @@ import chisel3.internal.Builder.Prefix | |||
| import scala.util.Try | |||
| import scala.annotation.implicitNotFound | |||
| import scala.collection.mutable | |||
| import chisel3.ChiselException | |||
| val out = IO(Output(UInt(8.W))) | ||
| override protected val implicitClock = (!foo).asClock | ||
|
|
||
| val r = Reg(UInt(8.W)) |
There was a problem hiding this comment.
can we do a test that withClock(Module.clock) {...} still gives me the expected implicitClock
There was a problem hiding this comment.
I meant that comment to apply for a Module as well
There was a problem hiding this comment.
Are you basically asking that Module.clock returns implicitClock? I should even be able to just assert that
| /** Provides an implicit Reset for use _within_ the [[RawModule]] | ||
| * | ||
| * Be careful to define the Reset value before trying to use it. | ||
| * Due to Scala initialization order, the actual val defining the Reset must occur before any |
There was a problem hiding this comment.
nit: You say "the actual val" but then below you use a def. I can certainly imagine someone doing override protected def implicitClock = InstantiateAClockGate(...) and being very sad by all the clock gates getting put down. Should we use a val in the example body, or explicilty say "val or def defining the reset" in the code above?
There was a problem hiding this comment.
The point I'm trying to illustrate is that the actual reset object needs to be a value, a val, but implicitReset can be a def. I could simplify the example a little bit by glossing over this subtly, but I was trying to show this point. What do you think I should do here? I could make this example very simple and put the more subtle example in a cookbook example.
There was a problem hiding this comment.
I could also just add more text explaining this fact.
There was a problem hiding this comment.
I've decided to keep the def but add a clarifying comment
They were private and [deprecated] protected methods used by an already removed API for setting the clock and reset of a Module from the outside.
Mixing in these traits enables overriding the implicit clock or reset from within a Module.
Module's logic that creates the implicit clock and implicit reset within the Module's body is now implemented in new traits that users are free to mix in to RawModule. There are also now virtual methods implicitClock and implicitReset that the user can override to change what clock or reset is used.
jackkoenig
force-pushed
the
jackkoenig/override-clock-and-reset
branch
from
88212bf to
62d0be0
Compare
Module's logic that creates the implicit clock and implicit reset within the Module's body is now implemented in new traits that users are free to mix in to RawModule. There are also now virtual methods implicitClock and implicitReset that the user can override to change what clock or reset is used. (cherry picked from commit abc96e2)
Module's logic that creates the implicit clock and implicit reset within the Module's body is now implemented in new traits that users are free to mix in to RawModule. There are also now virtual methods implicitClock and implicitReset that the user can override to change what clock or reset is used. (cherry picked from commit abc96e2) Co-authored-by: Jack Koenig <[email protected]>
This also deleted a long-deprecated protected method that didn't do anything.
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
These traits implement the functionality formerly only implemented in Module such that they can now be used by RawModules. They also define new protected virtual methods
implicitClockandimplicitResetthat can be overridden withinModuleto change what values are used as the implicit clock and implicit reset respectively.Reviewer Checklist (only modified by reviewer)
3.5.x,3.6.x, or5.xdepending on impact, API modification or big change:6.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.