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

Updated user guide introduction for nmigen. #22

Closed
wants to merge 18 commits into from

Conversation

sam-falvo
Copy link

The goal is to work through each section, one by one, receiving documentation reviews along the way. The introduction is a good first step. Feedback appreciated!

@codecov
Copy link

codecov bot commented Jan 6, 2019

Codecov Report

Merging #22 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #22   +/-   ##
=======================================
  Coverage   85.87%   85.87%           
=======================================
  Files          28       28           
  Lines        4063     4063           
  Branches      818      818           
=======================================
  Hits         3489     3489           
  Misses        480      480           
  Partials       94       94

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a77122...1fedc1d. Read the comment docs.

#. different design strategies for ASICs versus FPGAs
#. and more...

A little-known fact about FPGAs is that many of them have the ability to initialize their registers from the bitstream contents. This can be done in a portable and standard way using an "initial" block in Verilog, and by affecting a value at the signal declaration in VHDL. This renders an explicit reset signal unnecessary in practice in some cases, which opens the way for further design optimization. However, this form of initialization is entirely not synthesizable for ASIC targets, and it is not easy to switch between the two forms of reset using V*HDL.
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no "ASIC mode" in nMigen.

Copy link
Contributor

Choose a reason for hiding this comment

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

As in, nMigen does not offer the option to switch between the two options either. (It may in the future, but doesn't now.) So this sentence should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

How should I resolve this copy then? I interpret this paragraph not as there being specific ASIC modalities or explicit ASIC support, but rather, "This is a problem contributing to why we wrote nmigen."

Alternatively, I can remove this paragraph all together.

doc/introduction.rst Outdated Show resolved Hide resolved
doc/Makefile Outdated Show resolved Hide resolved
doc/introduction.rst Outdated Show resolved Hide resolved
doc/introduction.rst Outdated Show resolved Hide resolved
doc/hdl.rst Outdated Show resolved Hide resolved
doc/hdl.rst Outdated Show resolved Hide resolved
doc/hdl.rst Outdated Show resolved Hide resolved
doc/hdl.rst Outdated Show resolved Hide resolved
doc/hdl.rst Outdated Show resolved Hide resolved
doc/hdl.rst Outdated Show resolved Hide resolved
doc/hdl.rst Outdated Show resolved Hide resolved
doc/hdl.rst Outdated Show resolved Hide resolved
@eilims
Copy link

eilims commented Feb 7, 2019

Howdy!
For the global section on the installation of nmigen it would be good to add that the pip install command (if used) requires pip3 to properly install using setup.py. The current pip command: pip install git+https://github.com/m-labs/nmigen.git references python2 (at least on my machine (Ubuntu)) and fails to install due to the Python 3.6+ requirement.

Following these commands will correctly install the package:

  1. sudo apt-get install python3-pip
    To get the python3 version of pip
  2. pip3 install git+https://github.com/m-labs/nmigen.git
    Install nmigen using python3 without the taint of python2

This ensures that all packages can be found by the installer as pulling the repository and attempting to run the setup.py locally using python3 will result in missing packages (notably Python.h)

I was installing nmigen today and may have pulled out a hair or two and just wanted to make everyone's life a little easier!

@sam-falvo
Copy link
Author

Thank you for the feedback. I'd missed this. I'll probably be able to commit your recommended changes in a day or two, unless by some miracle I find the free time later tonight. :)

@sam-falvo
Copy link
Author

sam-falvo commented Feb 21, 2019

I decided to make the change without mentioning Ubuntu or Debian specifically. Not all Linux distributions use a distinct pip3 executable (I use Void Linux, for instance, and at least within a virtualenv configured to use Python 3, pip and pip3 are identical executables). I'll be pushing the change shortly. Apologies for the long latency; the start-up that I work for has had me well and truly burnt out for a while.

@peteut
Copy link
Contributor

peteut commented Feb 21, 2019

@sam-falvo It should not be possible to install nMigen on unsupported Python versions anyway, should be constrained using python_requires = ~=3.6,~=3.7.

[1] https://packaging.python.org/guides/distributing-packages-using-setuptools/#python-requires

@sam-falvo
Copy link
Author

sam-falvo commented Feb 21, 2019 via email

@peteut
Copy link
Contributor

peteut commented Feb 21, 2019

Wouldn't >=3.6 be preferred?

On Thu, Feb 21, 2019 at 8:45 AM Alain Péteut @.***> wrote: @sam-falvo https://github.com/sam-falvo It should not be possible to install nMigen on unsupported Python versions anyway, should be constrained using python_requires = =3.6,=3.7. [1] https://packaging.python.org/guides/distributing-packages-using-setuptools/#python-requires — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#22 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/ABbP-IMtIFFe6Ng_O8JssDnomiJPCihdks5vPs0igaJpZM4ZylP2 .
-- Samuel A. Falvo II

Indeed, >=3.6. Sorry for that.


A HDL module is a Python object that derives from the ``Module`` class. Module objects have a series of methods, described below, which describes the behavioral characteristics of the module. In addition to these behavioral methods, the HDL module object also possesses attributes which are used to help construct combinatorial and synchronous logic as well.

In essence, a fresh ``Module`` object a blank slate which, by way of various HDL methods described below, is populated with a description of the hardware module you want to synthesize or simulate.
Copy link

Choose a reason for hiding this comment

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

... object is a blank ...

self.s = Signal(max=3)
self.y = Signal()

def get_fragment(self, platform):
Copy link

Choose a reason for hiding this comment

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

indent

@benallard
Copy link

Is there any reason this isn't merged (yet) ? Are there plain wrong statements in there, or is it a good start, and other could step in and improve the doc incrementally ?

@sam-falvo
Copy link
Author

Some time ago, I received the following from Whitequark:

Thank you for your effort. Ultimately I decided to completely rewrite the manual for nMigen, with both different style and end goals. The current (quite unfinished but already useful) manual is available at https://nmigen.info/nmigen/.

Looks like @whitequark forgot about this PR when she closed the other one. I'll close this PR, as the manual I was working on is no longer relevant.

@sam-falvo sam-falvo closed this Aug 23, 2020
@benallard
Copy link

Thanks for the info. It’s pretty unfortunate that this merge-request did show off before other stuff when googling for the possibility to nest if statements. Is the actual doc linked somewhere?

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.

None yet

6 participants