-
Notifications
You must be signed in to change notification settings - Fork 58
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
build: add DSL for defining platform resources. #59
Conversation
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
+ Coverage 85.73% 85.91% +0.17%
==========================================
Files 27 28 +1
Lines 4010 4060 +50
Branches 803 817 +14
==========================================
+ Hits 3438 3488 +50
Misses 479 479
Partials 93 93
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See review comments above.
Also, please rename the entire module such that it becomes nmigen.build.dsl
.
Also, please write tests for Pins
and DiffPairs
to verify that these APIs work correctly. I think I will ask for more changes to Subsignal
so you don't need to write tests for that for now.
for i in identifiers: | ||
if not isinstance(i, str): | ||
raise TypeError("Identifier must be a string, not {!r}".format(i)) | ||
self.identifiers = " ".join(identifiers).split(" ") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you use .split()
here, then identifiers with tabs between them will be correctly separated. (This is a marginal case but still useful.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also think there should be only one string of identifiers accepted. Rationale: we have a DiffPairs
API that currently accepts a fairly loosely defined set of strings. There are a lot of different ways to write the arguments for DiffPairs
and it is not obvious what the result will be.
I propose that these should be the only valid formats:
Pins("A0 A1 A2 A3 A4 A5")
DiffPairs("B0 B2 B4 B5",
"B1 B3 B5 B7")
It should also be possible, and maybe preferred, to write this:
DiffPairs(p="B0 B2 B4 B5",
n="B1 B3 B5 B7")
to make it absolutely clear which pin has which function. (On some FPGAs like iCE40 which is positive and which is negative is fixed, so you only really need one pin to identify the pair, but e.g. on ECP5 you can assign them arbitrarily, so you need to carefully specify positive and negative.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, this is very good! I thought about Resource
and it is basically OK. Please add a test for that and I'll merge this.
No description provided.