Skip to content
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

adding RawModule documentation in module.md section #24

Merged
merged 11 commits into from
Sep 11, 2019
Merged

adding RawModule documentation in module.md section #24

merged 11 commits into from
Sep 11, 2019

Conversation

Martoni
Copy link
Contributor

@Martoni Martoni commented Sep 10, 2019

documentation proposal for RawModule refering to this Chisel3 issue.

Closes chipsalliance/chisel#678

@johnsbrew
Copy link

Hi, it's great to get some pieces of documentation for these features =)
Just so you know, they are in the process of getting out of chisel3.experimental to go into the main chisel3:
chipsalliance/chisel#1162

Which would lead to some minor modifications to the doc you have just written.

@Martoni
Copy link
Contributor Author

Martoni commented Sep 10, 2019

Thanks johnsbrew, I deleted lines with «experimental» remarks.

@seldridge
Copy link
Collaborator

seldridge commented Sep 10, 2019

I'll give this a real review, but the experimental vs. non-experimental is tricky. The website is using submodules (pointing at the latest tagged-version) of Chisel for it's dependencies. Because of that, anything in a tut shed (which is going to be run in a REPL) needs to work. Consequently, stuff like: [...] or use of bleeding edge features will cause the website build to fail.

@johnsbrew brings up an interesting point related to what version should the website documentation be documenting?

Currently, it's documenting the most recent tagged version (3.1.8). My reasoning here is that I don't want to document features that require the use of 3.2-SNAPSHOT. I figure if people are comfortable with that, then they are comfortable with only using the published snapshot Scaladoc and are beyond the need for the gentle documentation. People wanting to use newer features can be fixed with a faster release cadence 🤞.

The current approach of documenting the most recent version implies that it's a (necessary) pain to prepare the website for a new release, e.g., 3.2, as all the documentation needs to get updated. This isn't insurmountable, though.

I'm open to other approaches on how to handle this.

@seldridge seldridge self-requested a review September 10, 2019 16:44
@johnsbrew
Copy link

I guess the original "experimental" remark was a good approach then, maybe just add a tag "must be imported from experimental prior X.Y"

However IMHO the website build should be based on the latest snapshot of chisel, so latest features can be advertised very quickly to users. Devs should also come with proper documentation and basic intended use-cases along with their code changes when publishing new features.

As @chick mentioned it in the original issue chipsalliance/chisel#678 the idea of this 'gentle documentation' is to highlight cool features of chisel. I was personally unaware of chisel's RawModule and MultiIOModule until very recently which I think is a shame.

@seldridge
Copy link
Collaborator

Interesting perspective @johnsbrew. I think there's a fair point in getting features out to users. (Discoverability is a huge issue.)

I think you're implying this, but do you think purely new features (e.g., those that do not exist in 3.1.8) should be advertised on the website?

@Martoni
Copy link
Contributor Author

Martoni commented Sep 10, 2019

I wrote this document because I needed it for a design and the only documentation about RawModule I'd found is my own blog article (in french).
In my opinion this feature is not a gadget, it's mandatory for FPGA design I think.

Copy link
Collaborator

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for diving into documenting Chisel! We need more contributions like this!

Two main points:

  • If you are so inclined, this PR would be really complete with documentation of MultiIOModule. I won't hold up this PR if you want to do that in a subsequent request or leave it for someone else.
  • I think the example you provide (which is a great start) could be simplified a bit and show usage of RawModule to rename all ports.

val sclk = Input(Bool())
val csn = Input(Bool())
})
[...]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you put this code in a tut shed, it's executing on the Scala REPL. So, anything here needs to be legal Scala and type safe.

Suggested change
[...]
// ...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'm deleting tut shed replacing it with ```scala

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if you can keep the tut sheds I think that would be better. There's then a guarantee that a user can copy-paste that code block and it will work (with a specific version of the scala compiler).

Going forward, I view tut or mdoc as the approach for keeping the documentation in sync (and guaranteed to work): #4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

docs/src/main/tut/chisel3/modules.md Show resolved Hide resolved

For this, a specific module named *RawModule* should be used. With RawModule
there is no implicit signals. All signals like clock and reset should be
given in IO argument.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would tighten these two paragraphs up. Consider something like:

A `RawModule` is a module that allows you to define as much `IO` as needed (like `MultiIOModule`) but **does not provide an implicit clock and reset.** 
This can be useful when interfacing a Chisel module with a design that expects a specific naming convention for clock or reset.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Schuyler and would condense these two paragraphs into one. For example, we probably don't need to repeat something like *Module* are used to connect components inside a design

A `RawModule` is a module that allows you to define as much `IO` as needed with fully-custom naming (like `MultiIOModule`) but **does not provide an implicit clock and reset.** 
This can be useful when interfacing a Chisel module with a design that expects a specific naming convention for clock or reset.

there is no implicit signals. All signals like clock and reset should be
given in IO argument.

Then we can use it in place of *Module* usage :
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this example is a great start, but may be a little verbose. Maybe something abstract and with fewer signals.

I've hit the issue where companies expect _i and _o suffixes on ports. Maybe something like the following which shows naming problems with all io:

class Foo extends Module {
  val io = IO(new Bundle{
    val a = Input(Bool())
    val b = Output(Bool())
  }
  io.b := !io.a
}
class FooWrapper extends RawModule {
  val a_i  = IO(Input(Bool()))
  val b_o  = IO(Output(Bool()))
  val clk  = Input(Clock())
  val rstn = Input(Bool())

  val foo = withClockAndReset(clk, !rstn){ Module(new Foo) }

  foo.io.a := a_i
  b_o := foo.io.b
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I discovered the syntax :

  val foo = withClockAndReset(clk, !rstn){ Module(new Foo) }

I usally use this syntax :

withClockAndReset(clk, !rstn){ 
 val foo = Module(new Foo)
}

I still have a lot to learn ;)

}
```

In the example above, the RawModule is used to change the reset polarity
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Scala/Chisel things, the rest of the documentation uses RawModule over RawModule or RawModule.

conjuction with BlackBox to connect a differential clock input for example.

With multi-lines IO() declaration we also rename signal in emitted verilog.
Instead of *io_* named signals we will have a same names has val written:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this documentation should likely be moved into the documentation for MultiIOModule if we introduce Module first, MultiIOModule second, and RawModule third.

@Martoni
Copy link
Contributor Author

Martoni commented Sep 11, 2019

Thanks for all the reviewing. I can do the reviewed modifications myself if you want. But maybe it's easier to let you do it no ?
I wanted to document MultiIOModule but I could not find documentation about it ;). And Scala is not so easy to understand quickly.

@seldridge
Copy link
Collaborator

Probably easier, but that would defeat the point! Why don't you make modifications just to raw module and we can leave the MultiIOModule documentation for future work.

@Martoni
Copy link
Contributor Author

Martoni commented Sep 11, 2019

I rewritten text according to the above review.

Copy link
Collaborator

@seldridge seldridge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. Final round of review. Should be good after this.

Note: once you add in the tut:silent (or other tut options), you can test that all your code examples work (or, in this case, that my example isn't busted) with: sbt ++2.12.6 docs/makeMicrosite.

docs/src/main/tut/chisel3/modules.md Show resolved Hide resolved
docs/src/main/tut/chisel3/modules.md Outdated Show resolved Hide resolved
docs/src/main/tut/chisel3/modules.md Show resolved Hide resolved
docs/src/main/tut/chisel3/modules.md Outdated Show resolved Hide resolved
docs/src/main/tut/chisel3/modules.md Outdated Show resolved Hide resolved
docs/src/main/tut/chisel3/modules.md Outdated Show resolved Hide resolved
@Martoni
Copy link
Contributor Author

Martoni commented Sep 11, 2019

Ok, I used the github feature to 'approuve' all your proposal but it flooded a little bit the commit log ;)

@seldridge
Copy link
Collaborator

Awesome. Thanks for this! Once this gets through regressions, I'll squash and merge (to get rid of the spurious and broken commits).

@seldridge seldridge merged commit 72e5f84 into freechipsproject:master Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Documentation Needed for RawModule and MultiIOModule
4 participants