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

Update documentation for clock examples #42

Merged
merged 14 commits into from
Jun 16, 2020

Conversation

daniellimws
Copy link
Collaborator

This adds many examples from the tests/clocks directory to the documentation, including the verilog code with a svg generated from it, and the contents of their respective model.xml.

@mithro
Copy link
Collaborator

mithro commented Apr 15, 2020

@daniellimws This looks pretty good!

@mithro
Copy link
Collaborator

mithro commented Apr 15, 2020

@daniellimws - If you rebase and force push this branch there should be a new read-the-docs build that generates a preview of your new output!

@daniellimws
Copy link
Collaborator Author

I'm currently following the naming that was given earlier in the docs. In this example, the title is "D-Flipflop with combinational logic". Doesn't a D-flipflop only have input D and output Q? Should we rename this to just flipflop instead of D-flipflop?

@daniellimws
Copy link
Collaborator Author

There are many black box modules in the examples. As a result, netlistsvg renders them as ports not connected to any module (for example this).

Should we use the designs generated by symbolator for such cases (like this)? Might have some inconsistency in the designs? Any ideas?

@mithro
Copy link
Collaborator

mithro commented Apr 16, 2020

@daniellimws - Regarding the weird rendering from the netlistsvg output I logged #48

@mithro
Copy link
Collaborator

mithro commented Apr 16, 2020

I also logged - #50 -- Come up with a good "template" for documentation of the tests

I think all the tests should have both symbolator diagram of the verilog and netlistsvg rendered output.

docs/conf.py Outdated
@@ -45,6 +45,7 @@
'sphinx_markdown_tables',
'symbolator_sphinx',
'sphinxcontrib_verilog_diagrams',
'm2r'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mithro I'm using this extension to convert markdown into rst so that we can directly include README.md into the home page index.rst. However, there is a bug with the current version of m2r installed from pip/conda. I've applied the fix suggested here to my own fork. May I transfer it to SymbiFlow for sustainability purposes?

Also, this causes the readthedocs build to fail for now because of these lines https://github.com/SymbiFlow/python-symbiflow-v2x/pull/42/files#diff-85987f48f1258d9ee486e3191495582dR57-R59 causing there to be two markdown source parsers (when there should only be one).

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 we need to figure out if we want to use m2r or recommonmark modules in all our documentation. This probably is a discussion for IRC.

Another option is to convert the README.md to a README.rst at the top level. Both PyPi and GitHub can reader rst files.

@daniellimws daniellimws force-pushed the docs-clock branch 3 times, most recently from 4cec9de to 0adcea7 Compare April 17, 2020 13:22
README.md Outdated
@@ -1,9 +1,106 @@
# python-symbiflow-v2x
# SymbiFlow Verilog to XML
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 we want to be clear it is "Verilog to Routing XML", however "SymbiFlow Verilog to Verilog to Routing XML" is super weird looking with multiple "Verilog to" in it....

Maybe the following? Open to alternatives;

Suggested change
# SymbiFlow Verilog to XML
# Verilog to Routing XML file generation from Verilog (`python-symbiflow-v2x`)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Poke.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Alright changed. I thought I changed this, maybe lost it when rebasing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Still doesn't seem to have updated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh! The issue is you have a README.rst and a README.md file now!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I forgot about it. I changed to use README.rst in the other pull request.

@daniellimws daniellimws force-pushed the docs-clock branch 2 times, most recently from 4c95c47 to de0d892 Compare April 19, 2020 09:12
@daniellimws
Copy link
Collaborator Author

Addressed #47, #49, (hopefully) #50, and additionally #54. #50 too(?) if we are fine with omitting the netlistsvg diagram, showing only the symbolator diagram for blackbox modules.

@mithro
Copy link
Collaborator

mithro commented Apr 20, 2020

@daniellimws Could you move Use README.rst instead of README.md into it's own pull request?

@mithro
Copy link
Collaborator

mithro commented Apr 20, 2020

Please rebase onto master. I think it LGTM otherwise.

@daniellimws daniellimws force-pushed the docs-clock branch 2 times, most recently from 0735423 to 2960694 Compare April 21, 2020 01:09
@daniellimws
Copy link
Collaborator Author

Ok, rebased and squashed some commits.

@daniellimws
Copy link
Collaborator Author

@mithro I just realized I didn't add the linenos option to show the line numbers next to the code. What do you think? Do we want to have that?

@daniellimws
Copy link
Collaborator Author

There are many black box modules in the tests, which have the (* whitebox *) attribute. For example:

/*
 * `input wire a` should be detected as a clock because of the `(* CLOCK *)`
 * attribute.
 */
(* whitebox *)
module BLOCK(a, b, o);
	(* CLOCK *)
	input wire a;
	input wire b;
	output wire o;
endmodule

I think the documentation should give a brief explanation on why there is a (* whitebox *) attribute, what is the reason to put this there. Perhaps this could be done on it's own page so that we don't repeat it everywhere (in a separate issue/PR).

(Actually, I myself don't know why is that attribute included.)

@mithro
Copy link
Collaborator

mithro commented Apr 26, 2020

@daniellimws - I created #61 to track the documentation of the whitebox attributes and friends.

Copy link
Collaborator

@mithro mithro left a comment

Choose a reason for hiding this comment

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

Just the two minor comments.

Signed-off-by: Daniel Lim Wee Soong <[email protected]>
Signed-off-by: Daniel Lim Wee Soong <[email protected]>
- Rename docs/tests/ to docs/examples/ as it fits the content more
closely
- Add examples main page
- Add description for clock examples page

Signed-off-by: Daniel Lim Wee Soong <[email protected]>
Signed-off-by: Daniel Lim Wee Soong <[email protected]>
Signed-off-by: Daniel Lim Wee Soong <[email protected]>
@daniellimws
Copy link
Collaborator Author

Ok removed README.md

All this while it was using `code` which italicizes the code instead
of ``code``.

Signed-off-by: Daniel Lim Wee Soong <[email protected]>
@mithro
Copy link
Collaborator

mithro commented Apr 30, 2020

@daniellimws
Copy link
Collaborator Author

daniellimws commented May 1, 2020

The problem was make links outputs to tests while I have changed the directory name to examples/.

Another problem was that the rtd build for master uses sphinx 2.4.4 while the build for this pull request uses sphinx 3.0.3. The newer version complains that the md files do not have a title and refuses to add them to the toctree.

To fix this I changed the environment.yml file to use sphinx 2.4.4 for now while we are still using the markdown symlinks. Also removed breathe from requirements.txt because we aren't using it and it only works on sphinx version 3.

The build for this PR still doesn't show the DSP examples because it is still using sphinx 3.0.3. I think it would work in master however.

@daniellimws
Copy link
Collaborator Author

@mithro Ok to merge?

@@ -0,0 +1,10 @@
Examples
============
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

.. Clock Example Tests

Clock
==============
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

@@ -0,0 +1,6 @@
Autodetection of clock from flipflop
=====================================
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

@@ -0,0 +1,7 @@
Manually set inputs as clock
==================================
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

@@ -0,0 +1,5 @@
Manually set outputs as clock
==================================
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

@@ -0,0 +1,19 @@
Set input as clock by name (clk)
+++++++++++++++++++++++++++++++++++
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

@@ -0,0 +1,23 @@
Set input as clock by name (regex)
+++++++++++++++++++++++++++++++++++
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

@@ -0,0 +1,19 @@
Set outputs as clock by name (multiple clock outputs)
++++++++++++++++++++++++++++++++++++++++++++++++++++++
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

@@ -0,0 +1,19 @@
Manually set output as clock by setting the CLOCK attribute
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

@@ -0,0 +1,19 @@
Set output as clock by name (clk)
+++++++++++++++++++++++++++++++++++
Copy link
Member

Choose a reason for hiding this comment

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

this underline is too long

@daniellimws
Copy link
Collaborator Author

Ok fixed the underlines

Signed-off-by: Daniel Lim Wee Soong <[email protected]>
Signed-off-by: Daniel Lim Wee Soong <[email protected]>
@daniellimws
Copy link
Collaborator Author

Weird. flake8 is failing on a file I did not modify.

@mithro mithro merged commit 5a8fb0d into chipsalliance:master Jun 16, 2020
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.

3 participants