Closed nmigen-issue-migration closed 5 years ago
nmigen-issue-migration
commented
5 years ago 
What thought has been put into the design of nmigen.build? Where are the design notes? What problems with migen.build does it fix? I don't see any of this...
nmigen-issue-migration
commented
5 years ago 
Merging #46 into master will decrease coverage by
0.03%. The diff coverage is60%.
@@ Coverage Diff @@
## master #46 +/- ##
==========================================
- Coverage 85.66% 85.62% -0.04%
==========================================
Files 27 27
Lines 3927 3930 +3
Branches 767 768 +1
==========================================
+ Hits 3364 3365 +1
- Misses 475 476 +1
- Partials 88 89 +1| Impacted Files | Coverage Δ | |
|---|---|---|
| nmigen/back/rtlil.py | 85.03% <60%> (-0.3%) |
:arrow_down: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 0a2a702...c0475fa. Read the comment docs.
nmigen-issue-migration
commented
5 years ago nmigen-issue-migration
commented
5 years ago 
The main difference lies in the use of Edalize to invoke vendor tools instead of doing so ourselves.
I want to invoke Vivado myself from Nix to facilitate things like https://github.com/m-labs/nix-scripts/issues/11 (or at least have an easy option to do so).
What problem does edalize solve? From what I can see it's only sourcing Xilinx's settingsXX.sh and running vivado -mode tcl -batch file.tcl - the xdc etc. have to be generated for it.
nmigen-issue-migration
commented
5 years ago 
@sbourdeauducq Edalize is an abstraction library for EDA tools to make it a bit easier to target different tools. It does not attempt to generate any RTL or constraint files as the purpose it to be a thin layer which allows python libraries (e.g. migen, litex, myhdl, apio) or stand-alone applications to use it while leaving the details up to each of these projects.
For the purpose of nmigen we have identified a couple of things we would like to change to make edalize more in lin with nmigens goals. IIRC the most important of those was to generate .sh/.bat files instead of Makefiles, to avoid the need to have make on the target system. I did a quick PoC for the icestorm backend some time ago (https://github.com/olofk/edalize/commits/sh) but haven't found time to finish this work
nmigen-issue-migration
commented
5 years ago
Why is it better than subprocess.Popen(["vivado", "-mode", ...], ...)?
nmigen-issue-migration
commented
5 years ago
It allows rebuilding the project without having python installed. It's of course a trade-off, but I have found it very handy to export a system that I can send to clients, or to a build server, without any external dependencies.
With that said, I'm not opposed to adding a possibility to launch tools from within the python code. E.g. by adding a a constructor flag to choose between generating sh/bat files, makefiles or launch from python. But I also got the feeling from your previous message that you wanted to launch vivado yourself anyway, so in that case, it would probably be best to just generate the sh file, which you can ignore and launch vivado yourself
nmigen-issue-migration
commented
5 years ago
It allows rebuilding the project without having python installed. It's of course a trade-off, but I have found it very handy to export a system that I can send to clients, or to a build server, without any external dependencies.
The existing solution in Migen also allows this. And Nix can automatically send stuff to one or many build servers...
nmigen-issue-migration
commented
5 years ago
It allows rebuilding the project without having python installed. It's of course a trade-off, but I have found it very handy to export a system that I can send to clients, or to a build server, without any external dependencies.
The existing solution in Migen also allows this. And Nix can automatically send stuff to one or many build servers...
Not sure what this refers to, but if that was a reply to what I wrote about sending to build servers, then good you have a solution for that, because that is outside of edalize's scope. If this refers to rebuilding an EDA project without requiring python, then it seems you had the answer to your question already.
All in all, I don't have any grand plans for edalize other than being a (mostly) tool-agnostic interface to EDA tools that can be reused by other projects to quickly gain support for many EDA tools. If it fits nmigen, great. More users is generally a good thing. If it needs some changes, let's look at those changes. If it's a bad fit for nmigen, then there's no use in shoehorning it in.
nmigen-issue-migration
commented
5 years ago
If this refers to rebuilding an EDA project without requiring python, then it seems you had the answer to your question already.
It refers to building without python. That has been used several times to report problems to Xilinx.
If it fits nmigen, great. More users is generally a good thing.
Well it fits in the sense that it can do the equivalent of a few subprocess.Popen and other calls, but I don't see it solving a difficult problem. And I see the future synthesis flows mostly using Nix to call the EDA programs and not being based on all-in-one Python scripts. The latter are fine for simple projects though.
nmigen-issue-migration
commented
5 years ago
to quickly gain support for many EDA tools
The difficult part here is writing the tcl and xdc files (or their equivalents for other tools)...
nmigen-issue-migration
commented
5 years ago 
Externalizing toolchain invocation to another tool/library could alleviate the need for this code.
So far, the approach for this PoC has been to reuse as much code from Migen as possible, except for toolchain-specific code, which is now limited to constraints file formatting.
As the io/connector handling code remains unchanged, most platforms should hopefully be able to be migrated with little to no modifications.
In the case of Edalize+Vivado, I do not know how to implement hooks such as toolchain.additional_commands, which can be found in platforms/arty_a7.py.
This is a case were we need to add commands to be executed after write_bitstream, and the Edalize template is currently not extensible.
This is also an example of a platform which requires toolchain configuration outside the constraints file.
I am not sure if my current approach is the right one and I am ready to turn backward and add toolchain invocation inside nMigen if need be.
nmigen-issue-migration
commented
5 years ago
Issues with migen.build
- Interfacing with vendor toolchains requires a lot of OS-specific code to format paths,
I don't think that's the worst issue with migen.build. Here are other ones:
IOStandard). Do we need any constraint translation layer anyway or just put everything into something like the current Misc?nmigen-issue-migration
commented
5 years ago
3\. Maintain portability between Linux/macOS/Windows
Except for the open source flow, I don't think anything runs on Mac.
nmigen-issue-migration
commented
5 years ago
Equivalent functionality to migen.build
Easy migration of Migen platforms to nMigen
If the goal is to do exactly the same as Migen, there is no need for nMigen :)
The platform migration can probably be done with some script that reads the Migen platform file and generates a rough nMigen platform file, that can be polished by hand afterwards.
nmigen-issue-migration
commented
5 years ago
In the case of Edalize+Vivado, I do not know how to implement hooks such as
toolchain.additional_commands, which can be found in platforms/arty_a7.py.This is a case were we need to add commands to be executed after
write_bitstream, and the Edalize template is currently not extensible.
All Edalize backends that support tcl (currently ise, modelsim, quartus, rivierapro, spyglass and vivado) can be extended by passing tcl files and specifying them as tclSource. The backend will then source these files as part of the project. This can be used to set extra options or install hooks. The other option is to implement this as a post_build hook
This is also an example of a platform which requires toolchain configuration outside the constraints file.
Can you explain what kind of configuration you're referring too?
nmigen-issue-migration
commented
5 years ago
Can you explain what kind of configuration you're referring too?
If I'm not mistaken, in the previous example:
write_cfgmem -force -format bin -interface spix4 -size 16 -loadbit \"up 0x0 {build_name}.bit\" -file {build_name}.bin"
should end up in a tcl file separate from the xdc ?
nmigen-issue-migration
commented
5 years ago
I don't think that requiring Nix for nMigen is the way to go. For example, Glasgow supports Windows as a 1st class platform, which means that it could not use nMigen; I see no reason nMigen should specifically eschew Windows given that it does not really gain much from this.
I think Edalize generating shell or batch scripts with an appropriate abstraction is a good solution to the problem with migen.build where it would poorly construct ad-hoc build scripts with a lot of code duplication.
I also think it's more important to focus on problems with migen.build such as the pin management system as opposed to bikeshedding the best way to run a few subprocesses.
nmigen-issue-migration
commented
5 years ago
@jfng I apologize for my tone earlier in this thread (in https://github.com/m-labs/nmigen/pull/46#issuecomment-474535969). I see you understood me correctly, but I should not have implied that you haven't done design work.
nmigen-issue-migration
commented
5 years ago
It's not really bikeshedding since it leads to the idea that nmigen.build (for lack of a better name) should focus on managing FPGA I/O resources and generating EDA tool inputs, and not try too hard to be a build system. The user can supply their own external build system, e.g. based on Nix, that runs the EDA tools and performs other steps.
Also, the generated EDA tool inputs should not be monolithic as they are now. For example:
Having a "batteries included" script that runs all common steps and invokes the EDA tools is fine for simple projects though.
nmigen-issue-migration
commented
5 years ago
and not try too hard to be a build system
I agree, which is why I think Edalize is the way to go for something that would be built into nMigen.
The user can supply their own external build system, e.g. based on Nix, that runs the EDA tools and performs other steps.
Also, the generated EDA tool inputs should not be monolithic as they are now.
Also reasonable.
nmigen-issue-migration
commented
5 years ago 
@jfng Re: edalize being tied to make, the plan was to move away from that. @olofk even has a branch for it.
Re: "external build system", is that not what edalize is already doing? It will also generate the EDA inputs for you, so nMigen doesn't have to do it.
There should be a "fire and forget" way to generate all the EDA inputs into a self-contained package when using nMigen to distribute to other people and remote machines. But I'm not convinced nMigen itself needs to generate those files. NMigen can "just" focus on pin and resource management.
nmigen-issue-migration
commented
5 years ago
Re: "external build system", is that not what edalize is already doing? It will also generate the EDA inputs for you, so nMigen doesn't have to do it.
No to both. Edalize basically just runs the tool; and for the purposes of things like https://github.com/m-labs/nix-scripts/issues/11 I want to be able to disable the Edalize steps completely.
nmigen-issue-migration
commented
5 years ago
@sbourdeauducq You can use edalize to just generate a self contained project. See here for an example. Edalize does not invoke a toolchain in this demo; I do it manually.
Anyway is below an accurate summary of what you're discussing?
The advantage of migen.build for me was mostly reducing boilerplate (TCL, CDC, paths and sourcing scripts (!), etc) for every project I wrote. Yes there was a lot of duplication internally in migen.build, but over the years migen.build saved me a lot of time.
nmigen-issue-migration
commented
5 years ago
You can use edalize to just generate a self contained project. See here for an example
It still generates a monolithic output, doesn't allow an external build system to supply additional VHDL/Verilog files, doesn't allow generating a DCP instead of a bitfile, etc.
Again, having the option to do that, as long as it can be disabled, is fine and good for simple projects.
nmigen-issue-migration
commented
5 years ago
- some constraint names are Xilinx-centric (e.g. IOStandard). Do we need any constraint translation layer anyway or just put everything into something like the current Misc?
I agree to put vendor specific constraints in Misc.
What is the purpose of the PlatformInfo constraint ? I do not see it used in Migen platforms.
If IOStandard, Drive and PlatformInfo are not required, we could end up with only three types of constraint: Pins, Subsignal and Misc.
- error reporting
- code cleanliness in the pin management system
I think these two issues are related.
In migen.build, an I/O resource is defined by a name, a number and a list of constraints. As far as I understand, constraints are subject to the following rules:
Pins constraint or at least one Subsignal constraint.Subsignal constraint must contain a Pins constraint.These rules are implicitly validated in the _resource_type() function. However, violations are reported by an AssertionError with no further indication.
Despite being treated differently from other constraints, Pins and Subsignals can be positioned anywhere in the constraint list. This lack of distinction forces functions to do the separation themselves. See get_sig_constraints().
In order to make this distinction explicit, I suggest the following model:
An I/O resource is defined by:
Pins constraint or a list of Subsignal constraints,Misc constraints.A Subsignal is defined by:
Pins constraint,Misc constraints.By separating Pins and Subsignals from Misc constraints, resource validation and retrieval should become straightforward.
Re: "external build system", is that not what edalize is already doing? It will also generate the EDA inputs for you, so nMigen doesn't have to do it.
No to both. Edalize basically just runs the tool; and for the purposes of things like m-labs/nix-scripts#11 I want to be able to disable the Edalize steps completely.
We can split the build in two steps:
Step 2. is optional in case the user wants more control over their toolchain.
nmigen-issue-migration
commented
5 years ago
In order to make this distinction explicit, I suggest the following model:
An I/O resource is defined by:
- a name,
- a number,
- either one
Pinsconstraint or a list ofSubsignalconstraints,- an optional list of
Miscconstraints.A
Subsignalis defined by:
- a name,
- a
Pinsconstraint,- an optional list of
Miscconstraints.By separating
PinsandSubsignals fromMiscconstraints, resource validation and retrieval should become straightforward.
For example:
Before:
_io = [
("clk100", 0, Pins("E3"), IOStandard("LVCMOS33")),
("serial", 0,
Subsignal("tx", Pins("D10")),
Subsignal("rx", Pins("A10")),
IOStandard("LVCMOS33")
),
]After:
_io = [
Resource("clk100", 0, Pins("E3"), misc=[Misc("IOSTANDARD=LVCMOS33")]),
Resource("serial", 0,
Subsignal("tx", Pins("D10")),
Subsignal("rx", Pins("A10"))
misc=[Misc("IOSTANDARD=LVCMOS33")]
)
]
class Resource:
def __init__(self, name, number, *io, misc=[]):
self.name = name
self.number = number
if (len(io) == 1) and isinstance(io[0], Pins):
self.io = io[0]
elif all(isinstance(c, Subsignal) for c in io):
self.io = io
else:
raise TypeError("io has invalid type: should be either Pins or a list of Subsignals")
if misc and not all(isinstance(c, Misc) for c in misc):
raise TypeError("misc has invalid type: should be a list of Misc")
self.misc = misc
if misc and not all(isinstance(c, Misc) for c in misc): raise TypeError("misc has invalid type: should be a list of Misc")
What's the point of Misc then? Just make a list of strings. Also, the usual Python coding style is not to check for types unnecessarily.
nmigen-issue-migration
commented
5 years ago
Also, the usual Python coding style is not to check for types unnecessarily.
That style is garbage because all it results in is crashes some time later where you no longer have any context. The hypothetical advantages of being able to pass an object that implements the str interface but isn't a string are never realized here as well.
nmigen-issue-migration
commented
5 years ago
I don't think those errors are very hard to debug, and you can still have a similar issue by putting something other than a string inside the Misc object anyway. Python is simply not meant to be used without duck typing.
This is a proof-of-concept for a nMigen equivalent of Migen's build system.
The main difference lies in the use of Edalize to invoke vendor tools instead of doing so ourselves.
However, as far as I understand, constraint file (XDC, PCF, etc.) formatting must still be done on our side, which prevents nMigen from being completely vendor agnostic.
jfng included the following code: https://github.com/m-labs/nmigen/pull/46/commits