chipsalliance
stdlib: add UART from ip-contributions
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
- new feature/API
API Impact
add ip-contribution components.
Backend Code Generation Impact
Desired Merge Strategy
- Squash: The PR will be squashed and merged (choose this if you have no preference.
Release Notes
Reviewer Checklist (only modified by reviewer)
- [ ] Did you add the appropriate labels?
- [ ] Did you mark the proper milestone (Bug fix:
3.4.x, [small] API extension:3.5.x, API modification or big change:3.6.0)? - [ ] Did you review?
- [ ] Did you check whether all relevant Contributor checkboxes have been checked?
- [ ] Did you do one of the following when ready to merge:
- [ ] Squash: You/ the contributor
Enable auto-merge (squash), clean up the commit message, and label withPlease Merge. - [ ] Merge: Ensure that contributor has cleaned up their commit history, then merge with
Create a merge commit.
- [ ] Squash: You/ the contributor
Hi Martin, thanks for starting this PR.
Since this is the first full IP addition to the Chisel standard library, we should have an extended discussion about what our standards and procedures are for including IPs in the standard library.
It would be great if you could do a small write-up on what problem you are trying to solve with this IP. Ideally you would list some (open-source) projects that are all implementing their own UART and explain how their implementation could be replaced with a new Chisel standard library implementation. It would also be great if you could list some API design considerations, and example use-cases for your component. I was attempting something similar with my AXI Bundle PR but ran out of time to finish it.
Kevin
Hi Kevin,
On 9 Aug, 2022, at 15:06, Kevin Laeufer @.***> wrote:
Hi Martin, thanks for starting this PR.
Since this is the first full IP addition to the Chisel standard library, we should have an extended discussion about what our standards and procedures are for including IPs in the standard library.
Yes, I completely agree. Library code and API are a very sensible thing. However, it should also be not too hard to add stuff.
We briefly chatted on one Monday meeting that ip-contribution is kind of dead and we want to move it into Chisel std lib. I guess we could take that example and discuss the process next Monday.
It would be great if you could do a small write-up on what problem you are trying to solve with this IP. Ideally you would list some (open-source) projects that are all implementing their own UART and explain how their implementation could be replaced with a new Chisel standard library implementation. It would also be great if you could list some API design considerations, and example use-cases for your component. I was attempting something similar with my AXI Bundle PR https://github.com/chipsalliance/chisel3/pull/2441 but ran out of time to finish it.
There is a little README.md there, but I will improve. Example use cases are part of the Uart.scala. Maybe we should split this? Don’t know.
Martin
What is that on CI?
[error] /home/runner/work/chisel3/chisel3/stdlib/src/main/scala/chisel3/std/uart/Uart.scala:17:34: Symbol 'type firrtl.ir.IntervalType' is missing from the classpath.
[error] This symbol is required by 'class chisel3.internal.firrtl.IntervalRange'.
[error] Make sure that type IntervalType is in your classpath and check for conflicting dependencies with `-Ylog-classpath`.
[error] A full rebuild may help if 'IntervalRange.class' was compiled against an incompatible version of firrtl.ir.
[error] class UartIO extends DecoupledIO(UInt(8.W))
The tests run locally OK.
There is a little README.md there, but I will improve. Example use cases are part of the Uart.scala. Maybe we should split this? Don’t know.
I mostly wanted to have some use-case scenarios so that we can discuss the design of the component. This would be distinct from user-facing documentation.
One thing that I think is very important here is to predict the impact of including this on the Chisel eco-system. We should try to estimate how many users would benefit from this inclusion and how they would take advantage of it.
I did some of my own digging:
Standard UART libraries
These will be helpful to see what kind of features we should provide and maybe we can also find some inspiration for more comprehensive tests.
- uart16550: popular with many Verilog projects, available through fusesoc
- amlib: amaranth (former nmigen) community library, not sure how many uses
- litex: written in migen, probably many FPGA uses
- sifive-blocks: most likely the most taped out UART written in Chisel
UARTs implemented in Chisel (presumable for learning purposes)
This might show us various ways to write a UART in Chisel, but none of them have very many features.
Copies of the UART IP suggested for inclusion
This shows that there is some demand for a UART like this.
I have no idea what is happening here. This is in my opinion a very innocent line of Chisel code.
[error] /home/runner/work/chisel3/chisel3/stdlib/src/main/scala/chisel3/std/uart/Uart.scala:49:20: Symbol 'type firrtl.annotations.ComponentName' is missing from the classpath.
[error] This symbol is required by 'method chisel3.internal.NamedComponent.toNamed'.
[error] Make sure that type ComponentName is in your classpath and check for conflicting dependencies with `-Ylog-classpath`.
[error] A full rebuild may help if 'NamedComponent.class' was compiled against an incompatible version of firrtl.annotations.
[error] bitsReg := 11.U
Is it only me having trouble with CI, or is it generally broken?
I'm generally hesitant to add things like this that are difficult to test thoroughly in the CI that we have set up for Chisel, and for which supporting them require significant domain knowledge outside of basic RTL design. What is our story for support when users have questions about using it in their tapeout? We would need a strong CODEOWNER story before adding more IPs like this to Chisel, and be open to dropping them quickly if the CODEOWNER "retires" from the project. If the argument is "the repo they are in is dead" that isn't a compelling argument to bring them upstream here.
be open to dropping them quickly if the CODEOWNER "retires" from the project
This effectively kills the usefulness of the standard library.
The fact that stdlib components ought to be maintained in perpetuum suggests that they should be maintainable by general Chisel maintainers, and not require domain specific knowledge.
I think we are saying the same thing? Code has to be maintained. People have to maintain it... as someone who currently does maintain this codebase, I personally don't feel comfortable saying anything about this UART (how it works, how to use it, what the tests do and do not cover, what standards it complies with, etc). That's not to say such a person does not exist, and I am suggesting that if we have a code owners story then that person can be identified, and can "train up" someone if they can no longer support it due to other interests/commitments.
If I want to make a change to chisel internals itself, who can I talk to to be comfortable that this code still works if these small set of tests pass?
I don't think this kills the effectiveness of the std library, it is the bare minimum to make a useful standard library or open source project in general.
I am happy to serve as code owner of this UART.
That sayed, we need to discuss how to handle library code in a meeting. If it is so hard to get some almost trivial but useful code in, we might be having a hard time getting useful contributions. BTW, this UART is not new. It already passed a review process some time ago at ip-contributions.
About domain knowledge: there is no domain knowledge; this is just basic RTL. A tick generation and a shift register. This code is even documented in the Chisel book.
Would you say this serves more as an example application of Chisel than a UART IP we are encouraging people to use in their designs and tape outs? I think that is useful as well but has a very different support model than, say, Queue.