-
Notifications
You must be signed in to change notification settings - Fork 615
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
Add explanation for Class and Object, with examples. #3880
Conversation
007f5f1
to
e05c0b9
Compare
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.
Thanks so much for putting together this documentation! It's really great to see how some of these new features are intended to be used.
val identifier = IO(Output(Property[String]())) | ||
// An output Property describing the CSR. | ||
val description = IO(Output(Property[String]())) | ||
// An output Property indicating the CSR width. | ||
val width = IO(Output(Property[Int]())) |
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.
Should these be annotated with @public
?
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.
Yeah, maybe they should. Nothing in the example accesses them at Chisel time, but they'd need to be @public
if so.
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.
is it important to show the @INSTANTIABLE and @public in this simple example...? It seems to be mixing concepts. The important thing here is that it is an IO(Output(Property...)), right?
Maybe the @instantiable/@public could be a follow-on example that says this stuff works with D/I?
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.
Yeah, there is a tension here between the simplest example, and making something that is safe. We have safe and unsafe APIs for creating objects. The unsafe API is useful if you don't know the fields a-priori, like if you're generating the classes/objects from some other data structure. The safe API is literally just Instance, and we declare some helper methods on Instance[T <: Class]
using an implicit class. I wanted the first example to show the safe APIs, but that does mean we need @public
on at least the input ports to be able to connect to them on the Instance.
Co-authored-by: Tynan McAuley <tynan@niobiummicrosystems.com>
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.
Thanks for the clear documentation. LGTM, I'll propose a end-to-end demonstration recently.
|
||
To understand the `Object` graph that is constructed, we will consider an | ||
entrypoint to elaboration, and then show a hypothetical JSON representation of | ||
the `Object` graph. The details of how we go from IR to an `Object` graph are |
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.
Currently, we don't have the API to get the hypothetical JSON representation. we can demo it in the panamaom framework.
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.
Yeah that would be great. I will update the wording here slightly, since panamaom isn't an "external tool". Maybe we can add an example in this section of the doc showing a concrete use of panamaom with this IR. It could traverse the descriptions and print something out, so we could show the code snippet and the expected output, or something like that.
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.
Thanks for @mikeurbach's work! Ask @SpriteOvO to copy&paste the examples to scala-cli test, so we can use the binding framework testing it.
(cherry picked from commit 036dd5e)
Contributor Checklist
docs/src
?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x
,5.x
, or6.x
depending 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
.