Add private Module API and internal DataMirror API for moduleIOs. #4036
Conversation
| * | ||
| * @param target BaseModule to get IOs from | ||
| */ | ||
| def moduleIOs(target: BaseModule): Seq[Data] = target.getIOs |
There was a problem hiding this comment.
Maybe constraint target to be FixedIOModule? I'm afraid in some complex design pattern, this function will execute to early.
There was a problem hiding this comment.
For the use case I had in mind, I don't want to add that constraint. Indeed, this is not generally safe, which I tried to explain in the scaladoc and by putting this into DataMirror.internal object.
There was a problem hiding this comment.
we need this for more than FixedIOModule.
There was a problem hiding this comment.
I will add more "this is unsafe" text to the scaladoc at least.
There was a problem hiding this comment.
I see, this sounds to be an advance unsafe API, wish normal user won't touch it;p
There was a problem hiding this comment.
Maybe we can try to add a hook in builder, if user called this function and new IO is created, log an warning to user saying the IO they got is not enough.
There was a problem hiding this comment.
I think it's fine to start with the new API "hidden" in DataMirror.internal, with a comment explaining the dangers. If users start using this and continue to be surprised by the documented behavior, I guess we could add the extra state to give warnings.
| @@ -245,4 +245,24 @@ class DataMirrorSpec extends ChiselFlatSpec { | |||
| DataMirror.getLayerColor(foo.c) should be(Some(A)) | |||
| } | |||
|
|
|||
| "moduleIOs" should "return an in-progress module's IOs" in { | |||
There was a problem hiding this comment.
super nit you changed the name of this function and maybe we should have changed this string
There was a problem hiding this comment.
thanks, i wish we could push NFC changes to main.
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
The DataMirror API allows users who know what they're doing to access a module's ports before it is closed.
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.