Add field grouping ScalaDoc for ArbiterIO #2208

Burnleydev1 · 2021-10-25T20:51:53Z

Contributor Checklist

Did you add Scaladoc to every public function/method?
Did you add at least one test demonstrating the PR?
Did you delete any extraneous printlns/debugging code?
Did you specify the type of improvement?
Did you add appropriate documentation in docs/src?
Did you state the API impact?
Did you specify the code generation impact?
Did you request a desired merge strategy?
Did you add text to be included in the Release Notes for this change?

Type of Improvement

Documentation

API Impact

Unknown Impact

Backend Code Generation Impact

No impact

Desired Merge Strategy

Squash

Release Notes

Improved documentation for ArbiterIO

Reviewer Checklist (only modified by reviewer)

Did you add the appropriate labels?
Did you mark the proper milestone (Bug fix: 3.3.x, [small] API extension: 3.4.x, API modification or big change: 3.5.0)?
Did you review?
Did you check whether all relevant Contributor checkboxes have been checked?
Did you mark as Please Merge?

mwachs5 · 2021-10-25T21:49:46Z

src/main/scala/chisel3/util/Arbiter.scala

@@ -13,6 +13,7 @@ import chisel3.internal.naming.chiselName  // can't use chisel3_ version because
  *
  * @param gen data type
  * @param n number of inputs
+  * @group Define IO bundle definition for an Arbiter


Suggested change

* @group Define IO bundle definition for an Arbiter

* @group Define IO bundle definition for an Arbiter

I think you want @groupdesc here, and I think the @groupdesc should go into Aggregate.scala... if that is a thing that works? So that every subclass of Bundle can by convention refer to the same group?

Add field grouping ScalaDoc for ArbiterIO

Okay @jackkoenig thank you.

I think you want @groupdesc here, and I think the @groupdesc should go into Aggregate.scala... if that is a thing that works? So that every subclass of Bundle can by convention refer to the same group?

Yes that's right. So I will send it into aggregate.scala, but I wish ask if that means all subclasses of Bundle will have the same description?

I also think the groupdesc could describe just fields with the same function.

So I will send it into aggregate.scala, but I wish ask if that means all subclasses of Bundle will have the same description?

Right, in that case the description would need to be something very generic like "The actual hardware fields of the Bundle". Keep in mind this is still opt-in for the user, they would need to manually add the @group Signals to each-and-every one of their val in their own Bundle subclass... which you can see is very verbose in the ArbiterIO example. I do not know a better way :/

Okay thank you @mwachs5, I will do just that.

Hello @mwachs5 please I keep seeing Some test were not successful, is there a way I can solve it?

one of the errors I'm seeing is

[error] ^ [error] /home/runner/work/chisel3/chisel3/src/main/scala/chisel3/util/Arbiter.scala:34:3: expected class or object definition [error] val out = Decoupled(gen) [error] ^ [error] /home/runner/work/chisel3/chisel3/src/main/scala/chisel3/util/Arbiter.scala:35:3: expected class or object definition [error] val chosen = Output(UInt(log2Ceil(n).W)) [error] ^

Should I put def there instead of val?

I think I should have removed

val in = Flipped(Vec(n, Decoupled(gen))) val out = Decoupled(gen) val chosen = Output(UInt(log2Ceil(n).W))

after adding

/** *... unchanged */ class ArbiterIO[T <: Data](private val gen: T, val n: Int) extends Bundle { // See github.com/freechipsproject/chisel3/issues/765 for why gen is a private val and proposed replacement APIs. /* Input data, one per potential sender * @group Signals */ val in = Flipped(Vec(n, Decoupled(gen))) /* Output data after arbitration * @group Signals */ val out = Decoupled(gen) /* One-Hot vector indicating which output was chosen * @group Signals */ val chosen = Output(UInt(log2Ceil(n).W)) }

So I hope it works now

mwachs5 · 2021-10-25T21:50:08Z

src/main/scala/chisel3/util/Arbiter.scala

@@ -98,6 +99,7 @@ class LockingArbiter[T <: Data](gen: T, n: Int, count: Int, needsLock: Option[T
  * arb.io.in(0) <> producer0.io.out
  * arb.io.in(1) <> producer1.io.out
  * consumer.io.in <> arb.io.out
+  * @group Define


Suggested change

* @group Define

* @group Signals

Thoughts @jackkoenig , @azidar on what this should be called?

actually, this suggestion isn't quite right... let me give a clearer one...

The right way to add grouping to the scaladoc to something that extends Bundle is like this (I think!)

/** *... unchanged */ class ArbiterIO[T <: Data](private val gen: T, val n: Int) extends Bundle { // See github.com/freechipsproject/chisel3/issues/765 for why gen is a private val and proposed replacement APIs. /* Input data, one per potential sender * @group Signals */ val in = Flipped(Vec(n, Decoupled(gen))) /* Output data after arbitration * @group Signals */ val out = Decoupled(gen) /* One-Hot vector indicating which output was chosen * @group Signals */ val chosen = Output(UInt(log2Ceil(n).W)) }

but oof this is quite verbose in the code!

I'd probably call it Ports or IOs

The right way to add grouping to the scaladoc to something that extends Bundle is like this (I think!)

/** *... unchanged */ class ArbiterIO[T <: Data](private val gen: T, val n: Int) extends Bundle { // See github.com/freechipsproject/chisel3/issues/765 for why gen is a private val and proposed replacement APIs. /* Input data, one per potential sender * @group Signals */ val in = Flipped(Vec(n, Decoupled(gen))) /* Output data after arbitration * @group Signals */ val out = Decoupled(gen) /* One-Hot vector indicating which output was chosen * @group Signals */ val chosen = Output(UInt(log2Ceil(n).W)) }

Okay @mwachs5, thank you

that means I have I to remove the other groupings I did.

I think the other groupings just won't do anything, since they aren't specifically on any members (val, def) of the class.

Sorry for the delay, there was a blackout in my area.

src/main/scala/chisel3/util/Arbiter.scala

changed group name Co-authored-by: Megan Wachs <megan@sifive.com>

jackkoenig · 2021-10-25T21:56:46Z

Can you make the title of this PR a little more precise? Maybe something like "Add field grouping ScalaDoc for ArbiterIO".

core/src/main/scala/chisel3/Aggregate.scala

mwachs5 · 2021-10-26T15:57:36Z

So I hope it works now

Were you able to build the API docs (different than the website Cookbooks/Explanation docs) by running sbt doc? If so you can open the HTML files in your web browser with file->open

Co-authored-by: Megan Wachs <megan@sifive.com>

Burnleydev1 · 2021-10-26T16:49:29Z

So I hope it works now

Were you able to build the API docs (different than the website Cookbooks/Explanation docs) by running sbt doc? If so you can open the HTML files in your web browser with file->open

Hello @mwachs5, I'm finding difficulties doing it. Should I type it in terminal, and should I replace file in file->open with the file name?

Burnleydev1 · 2021-10-26T18:53:46Z

So I hope it works now

Were you able to build the API docs (different than the website Cookbooks/Explanation docs) by running sbt doc? If so you can open the HTML files in your web browser with file->open

Hello @mwachs5, I'm finding difficulties doing it. Should I type it in terminal, and should I replace file in file->open with the file name?

Please where are the HTML files located?

Burnleydev1 · 2021-10-26T19:29:22Z

Hello @mwachs5, I opened it on my web browser, I was looking in the wrong place.

Burnleydev1 · 2021-10-26T20:21:34Z

Hello @mwachs5, I opened it on my web browser, I was looking in the wrong place.

But the information here is different than that on the main API website

mwachs5 · 2021-10-26T20:27:09Z

But the information here is different than that on the main API website

The main API website would have the 3.4.4 docs at this time, while if you build from master I would expect some differences. But, do you see any new grouping when you look at ArbiterIO? Can you share a screenshot (or the HTML page just for ArbiterIO)?

Burnleydev1 · 2021-10-26T20:41:51Z

But the information here is different than that on the main API website

The main API website would have the 3.4.4 docs at this time, while if you build from master I would expect some differences. But, do you see any new grouping when you look at ArbiterIO? Can you share a screenshot (or the HTML page just for ArbiterIO)?

Here it is

Burnleydev1 · 2021-10-26T21:11:43Z

And when I click on Bundle it gives me this page:

Burnleydev1 · 2021-10-27T03:24:00Z

It looks like it has generic groupings for all the subclasses
like value members and deprecated value members.

Burnleydev1 · 2021-10-27T04:19:38Z

I don't understand why the groupings we did is not showing on my local api web page.
@mwachs5 Please what is the difference between Arbiter and ArbiterIO, Decoupled and DecoupledIO.

Burnleydev1 · 2021-10-27T04:21:52Z

I changed the group in Arbiter to groupdesc and it showed on the web page under Arbiter but nothing changed on ArbiterIO.

Burnleydev1 · 2021-10-27T08:19:18Z

Hello @mwachs5, @jackkoenig, Please I'm stuck and have tried out several ways to make the grouping show on my local api webpage but its not visible.
But when I click on hide all for any subclass or class like of ArbiterIO, it returns a specific group and in this case it contains all the elements that are under the Signals group we created.

Burnleydev1 · 2021-10-27T08:20:28Z

Hello @mwachs5, @jackkoenig, Please I'm stuck and have tried out several ways to make the grouping show on my local api webpage but its not visible. But when I click on hide all for any subclass or class like of ArbiterIO, it returns a specific group and in this case it contains all the elements that are under the Signals group we created.

It does the same on the main api web site too.

Burnleydev1 · 2021-10-28T11:46:59Z

great now that you've got it working, what do you want to do with this PR... should we see how many things extend Bundle src/main/scala (which is basically the utils directory?) Or do we want to merge just this one?

I guess it might be good to be sure we like the name Signals before you go and put it on ALL the aggregate subclasses, and the general aesthetics of this solution. Thoughts @ekiwi , @jackkoenig , @chick ?

Hello @mwachs5 I wish to do groupings for subclasses of Bundle first which are: ArbiterIO, DecoupledIO, IrrevocableIO, PipeIO, QueueIO, ReadyValidIO, Valid, PRNGIO
and we did that of ArbiterIO and DecoupledIO(which is a subclass of ReadyValidIO), so what is left is: IrrevocableIO, PipeIO, QueueIO, Valid, PRNGIO.

Burnleydev1 · 2021-10-28T12:20:38Z

In the place of

  val enq = Flipped(EnqIO(gen))
  /** I/O to dequeue data (client is consumer and Queue object is producer), is [[Chisel.DecoupledIO]]*/
  val deq = Flipped(DeqIO(gen))
  /** The current amount of data in the queue */
  val count = Output(UInt(log2Ceil(entries + 1).W))

Its rather showing:

val enq = Flipped(EnqIO(gen))
  /** I/O to dequeue data (client is consumer and Queue object is producer), is [[Chisel.DecoupledIO]]*/
  val deq = Flipped(DeqIO(gen))
  /** The current amount of data in the queue */
  val count = Output(UInt(log2Ceil(entries + 1).W))
  /** When asserted, reset the enqueue and dequeue pointers, effectively flushing the queue (Optional IO for a flushable Queue)*/ 
  val flush = if (hasFlush) Some(Input(Bool())) else None

on the master branch for QueuIO.scala

Burnleydev1 · 2021-10-28T12:38:02Z

I don't know if its the master branch or the 3.4.x that is outdated?
because the information on the api website is from the 3.4.3 branch and it is different from the master branch, so if these changes are to go live then I think it will be conflicting.
So I wish to ask if I should clone the 3.4.x branch and work based on it?

mwachs5 · 2021-10-28T15:05:57Z

I don't know if its the master branch or the 3.4.x that is outdated?

Master branch is newer and you should always work from the master branch. What happens is that once this PR is merged, depending on what milestone we pick (we can pick 3.4), an automatic tool called Mergify will automatically create backport PRs to the 3.4.x branch. If there is an issue mergify tool will flag it and we will clean up the backport PR.

We could also just decide not to backport and have this be a feature when we release 3.5, then we will mark with the 3.5.0 milesone instead of 3.4

In the particular example of th Queue, someone recently added the new flush functionality but we did not want to backport it to 3.4, which is why it is showing on Master. We should just do the docs for what is on master and we can deal with any issues with 3.4 after this PR is merged

Burnleydev1 · 2021-10-28T17:09:56Z

Okay @mwachs5, thank you for the clarification.

src/main/scala/chisel3/util/Arbiter.scala

core/src/main/scala/chisel3/Aggregate.scala

src/main/scala/chisel3/util/Arbiter.scala

Co-authored-by: Megan Wachs <megan@sifive.com>

Burnleydev1 · 2021-10-29T18:11:27Z

Hello @mwachs5, Thanks for the review and corrections I committed the suggestions.

Burnleydev1 · 2021-11-02T09:16:05Z

Hello @mwachs5, please I wish to what the way forward with these pull requests is: #2208 and #2214 is

src/main/scala/chisel3/util/Arbiter.scala

Co-authored-by: Megan Wachs <megan@sifive.com>

Burnleydev1 · 2021-11-02T17:24:47Z

Hi @mwachs5, I went through it to check of syntax or ScalaDoc errors, and I think including your commit suggestions is no longer different from that on the master branch.

@param

* Update Arbiter.scala * Update src/main/scala/chisel3/util/Arbiter.scala changed group name Co-authored-by: Megan Wachs <megan@sifive.com> * minor changes on grouping ArbiterIO * removed unmatched closing brace * Remove groupdesc from Arbiter.scala * Added groupdesc to Aggregate.scala * Update Arbiter.scala * Update core/src/main/scala/chisel3/Aggregate.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update Arbiter.scala * Update src/main/scala/chisel3/util/Arbiter.scala Added suugestions. Co-authored-by: Megan Wachs <megan@sifive.com> * added suggestions from review * added suggestions from review * Resolved conflicts * update Arbiter.scala * Update core/src/main/scala/chisel3/Aggregate.scala deleted groudesc for ArbiterIO Co-authored-by: Megan Wachs <megan@sifive.com> * Update Scaladoc syntax * removed some lines * Better documentation * Removed @param and @gen * Update core/src/main/scala/chisel3/Aggregate.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update src/main/scala/chisel3/util/Arbiter.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Added groupdesc to ArbiterIO * Update src/main/scala/chisel3/util/Arbiter.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update core/src/main/scala/chisel3/Aggregate.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update Arbiter.scala * Update src/main/scala/chisel3/util/Arbiter.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update Arbiter.scala Co-authored-by: Megan Wachs <megan@sifive.com> (cherry picked from commit 6145512)

@param

* Update Arbiter.scala * Update src/main/scala/chisel3/util/Arbiter.scala changed group name Co-authored-by: Megan Wachs <megan@sifive.com> * minor changes on grouping ArbiterIO * removed unmatched closing brace * Remove groupdesc from Arbiter.scala * Added groupdesc to Aggregate.scala * Update Arbiter.scala * Update core/src/main/scala/chisel3/Aggregate.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update Arbiter.scala * Update src/main/scala/chisel3/util/Arbiter.scala Added suugestions. Co-authored-by: Megan Wachs <megan@sifive.com> * added suggestions from review * added suggestions from review * Resolved conflicts * update Arbiter.scala * Update core/src/main/scala/chisel3/Aggregate.scala deleted groudesc for ArbiterIO Co-authored-by: Megan Wachs <megan@sifive.com> * Update Scaladoc syntax * removed some lines * Better documentation * Removed @param and @gen * Update core/src/main/scala/chisel3/Aggregate.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update src/main/scala/chisel3/util/Arbiter.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Added groupdesc to ArbiterIO * Update src/main/scala/chisel3/util/Arbiter.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update core/src/main/scala/chisel3/Aggregate.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update Arbiter.scala * Update src/main/scala/chisel3/util/Arbiter.scala Co-authored-by: Megan Wachs <megan@sifive.com> * Update Arbiter.scala Co-authored-by: Megan Wachs <megan@sifive.com> (cherry picked from commit 6145512) Co-authored-by: Abongwa Bonalais <81782853+Burnleydev1@users.noreply.github.com>

Update Arbiter.scala

2f05f27

Burnleydev1 mentioned this pull request Oct 25, 2021

Improve documentation for subclasses of Bundle #2068

Closed

mwachs5 reviewed Oct 25, 2021

View reviewed changes

src/main/scala/chisel3/util/Arbiter.scala Outdated Show resolved Hide resolved

Update src/main/scala/chisel3/util/Arbiter.scala

a130b7f

changed group name Co-authored-by: Megan Wachs <megan@sifive.com>

Burnleydev1 changed the title ~~Update Arbiter.scala~~ Add field grouping ScalaDoc for ArbiterIO Oct 25, 2021

Burnleydev1 added 7 commits October 25, 2021 23:00

Merge branch 'chipsalliance:master' into apidocs

9579537

minor changes on grouping ArbiterIO

84022f4

removed unmatched closing brace

8cd10c3

Merge branch 'chipsalliance:master' into apidocs

2e83881

Remove groupdesc from Arbiter.scala

ed93cb6

Added groupdesc to Aggregate.scala

93eb9ad

Update Arbiter.scala

89ec347

mwachs5 reviewed Oct 26, 2021

View reviewed changes

core/src/main/scala/chisel3/Aggregate.scala Outdated Show resolved Hide resolved

Update core/src/main/scala/chisel3/Aggregate.scala

e5ea6cf

Co-authored-by: Megan Wachs <megan@sifive.com>

Burnleydev1 added 2 commits October 28, 2021 18:42

Added groupdesc to ArbiterIO

7049026

Merge branch 'master' into apidocs

f9c15d4

mwachs5 reviewed Oct 29, 2021

View reviewed changes

src/main/scala/chisel3/util/Arbiter.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Oct 29, 2021

View reviewed changes

core/src/main/scala/chisel3/Aggregate.scala Outdated Show resolved Hide resolved

mwachs5 reviewed Oct 29, 2021

View reviewed changes

src/main/scala/chisel3/util/Arbiter.scala Outdated Show resolved Hide resolved

Burnleydev1 and others added 4 commits October 29, 2021 18:54

Update src/main/scala/chisel3/util/Arbiter.scala

1422d28

Co-authored-by: Megan Wachs <megan@sifive.com>

Update core/src/main/scala/chisel3/Aggregate.scala

63fb3ea

Co-authored-by: Megan Wachs <megan@sifive.com>

Update Arbiter.scala

3a34421

Merge branch 'chipsalliance:master' into apidocs

d42eb5f

Merge branch 'master' into apidocs

c02405f

mwachs5 reviewed Nov 2, 2021

View reviewed changes

src/main/scala/chisel3/util/Arbiter.scala Outdated Show resolved Hide resolved

Burnleydev1 and others added 2 commits November 2, 2021 17:21

Update src/main/scala/chisel3/util/Arbiter.scala

3cb0872

Co-authored-by: Megan Wachs <megan@sifive.com>

Update Arbiter.scala

fa3f70e

Merge branch 'master' into apidocs

0234ce3

mwachs5 approved these changes Nov 3, 2021

View reviewed changes

mwachs5 added this to the 3.4.x milestone Nov 3, 2021

mwachs5 added the Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. label Nov 3, 2021

mergify bot merged commit 6145512 into chipsalliance:master Nov 3, 2021

mergify bot mentioned this pull request Nov 3, 2021

Add field grouping ScalaDoc for ArbiterIO (backport #2208) #2232

Merged

mergify bot added the Backported This PR has been backported label Nov 3, 2021

	* @group Define IO bundle definition for an Arbiter
	* @group Define IO bundle definition for an Arbiter

Add field grouping ScalaDoc for ArbiterIO #2208

Add field grouping ScalaDoc for ArbiterIO #2208

Conversation

Burnleydev1 commented Oct 25, 2021 • edited

Contributor Checklist

Type of Improvement

API Impact

Backend Code Generation Impact

Desired Merge Strategy

Release Notes

Reviewer Checklist (only modified by reviewer)

Choose a reason for hiding this comment

Burnleydev1 Oct 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwachs5 Oct 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwachs5 Oct 25, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackkoenig commented Oct 25, 2021

mwachs5 commented Oct 26, 2021

Burnleydev1 commented Oct 26, 2021 • edited

Burnleydev1 commented Oct 26, 2021

Burnleydev1 commented Oct 26, 2021

Burnleydev1 commented Oct 26, 2021

mwachs5 commented Oct 26, 2021

Burnleydev1 commented Oct 26, 2021

Burnleydev1 commented Oct 26, 2021

Burnleydev1 commented Oct 27, 2021 • edited

Burnleydev1 commented Oct 27, 2021

Burnleydev1 commented Oct 27, 2021

Burnleydev1 commented Oct 27, 2021

Burnleydev1 commented Oct 27, 2021

Burnleydev1 commented Oct 28, 2021

Burnleydev1 commented Oct 28, 2021

Burnleydev1 commented Oct 28, 2021

mwachs5 commented Oct 28, 2021 • edited

Burnleydev1 commented Oct 28, 2021

Burnleydev1 commented Oct 29, 2021

Burnleydev1 commented Nov 2, 2021

Burnleydev1 commented Nov 2, 2021

Burnleydev1 commented Oct 25, 2021 •

edited

Burnleydev1 Oct 25, 2021 •

edited

mwachs5 Oct 25, 2021 •

edited

mwachs5 Oct 25, 2021 •

edited

Burnleydev1 commented Oct 26, 2021 •

edited

Burnleydev1 commented Oct 27, 2021 •

edited

mwachs5 commented Oct 28, 2021 •

edited