New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
remove clock and cond from probe force/release API #3605
Conversation
@@ -101,14 +101,18 @@ package object probe extends SourceInfoDoc { | |||
} | |||
|
|||
/** Override existing driver of a writable probe. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while we are add it can we scaladoc the probe and value fields? SHould we mention in the scaladoc that the when condition is used...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, I don't think we should mention the when
stuff in the ScalaDoc b/c that's more of a firtool implementation detail. But will def add the other fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't call it an implementation detail. The semantic is that these ops only occur when the when condition is true.
For example, the ScalaDoc for printf says:
Prints a message every cycle. If defined within the scope of a when block, the message will only be printed on cycles that the when condition is true.
Does not fire when in reset (defined as the encapsulating Module's reset). If your definition of reset is not the encapsulating Module's reset, you will need to gate this externally.
I think we need something similar in these APIs, except instead of not firing when in reset, it doesn't fire "until reset has been asserted and then deasserted" through the Disable API (link to Disable).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me! just always lobbying for more scaladoc :)
Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Release Notes
Remove
clock
andcond
from probeforce
andrelease
methods.Reviewer Checklist (only modified by reviewer)
3.5.x
,3.6.x
, or5.x
depending 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
.