-
Notifications
You must be signed in to change notification settings - Fork 572
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
Extend constants in probe.force
and probe.forceInitial
methods.
#3418
Conversation
@@ -83,7 +93,7 @@ package object probe extends SourceInfoDoc { | |||
/** Override existing driver of a writable probe. */ | |||
def force(clock: Clock, cond: Bool, probe: Data, value: Data)(implicit sourceInfo: SourceInfo): Unit = { | |||
requireHasWritableProbeTypeModifier(probe, "Cannot force a non-writable Probe.") | |||
pushCommand(ProbeForce(sourceInfo, clock.ref, cond.ref, probe.ref, value.ref)) | |||
pushCommand(ProbeForce(sourceInfo, clock.ref, cond.ref, probe.ref, padData(value, probe.width.get).ref)) |
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.
What happens if the width of a Probe
itself is unknown?
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.
Repeating from the conversation on slack, firtool errors out if the probe width does not eventually compute to the force value width. So I have decided for now to just error if either of these widths are unknown, though for future purposes this may be too strict.
case Some(w) => | ||
if (w > probeWidth) Builder.error(s"Data width $w is larger than $probeWidth.") |
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.
If probe width is unknown, but data width is known, there's an extra error being generated that is unnecessary. I think you should tweak this like so:
case Some(w) => | |
if (w > probeWidth) Builder.error(s"Data width $w is larger than $probeWidth.") | |
case Some(w) => | |
if (probe.widthOption.exists(w > _)) Builder.error(s"Data width $w is larger than $probeWidth.") |
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 but there is 1 minor issue where 1 error will cause an extra one to be reported and the 2nd one is wrong, please fix then feel free to merge!
Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Release Notes
Properly extend forced values in
probe.force
andprobe.forceInitial
methods. Error out on unknown widths.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
.