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

Individual Pin Inversion #514

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

anuejn
Copy link
Contributor

@anuejn anuejn commented Oct 23, 2020

This PR implements individual pin inversion as discussed in RFC #510.
I was unsure about changing the internal representation of invert from a single bool to a tuple of bool. Is that a breaking change and should methods in vendor still support being passed a single bool?

@codecov
Copy link

codecov bot commented Oct 23, 2020

Codecov Report

Merging #514 into master will increase coverage by 0.04%.
The diff coverage is 89.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #514      +/-   ##
==========================================
+ Coverage   81.34%   81.38%   +0.04%     
==========================================
  Files          49       49              
  Lines        6412     6426      +14     
  Branches     1279     1286       +7     
==========================================
+ Hits         5216     5230      +14     
  Misses       1007     1007              
  Partials      189      189              
Impacted Files Coverage Δ
nmigen/build/plat.py 26.56% <0.00%> (ø)
nmigen/build/dsl.py 96.57% <100.00%> (+0.29%) ⬆️
nmigen/build/res.py 82.65% <100.00%> (ø)

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 df70aae...ba3f28d. Read the comment docs.

@@ -70,9 +78,19 @@ def __init__(self, p, n, *, dir="io", conn=None, assert_width=None):
"and {!r} do not"
.format(self.p, self.n))

if self.p.individual_inverts != self.n.individual_inverts:
raise SyntaxError("The individual invertaitons of the positive and negative pins do not match.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
raise SyntaxError("The individual invertaitons of the positive and negative pins do not match.")
raise SyntaxError("The individual inversions of the positive and negative pins do not match.")

@whitequark
Copy link
Member

I would strongly prefer it if there was only a single invert property, containing a tuple of booleans or perhaps more nicely a single int that can be XOR'd in the vendor code.

@anuejn
Copy link
Contributor Author

anuejn commented Oct 23, 2020

I would strongly prefer it if there was only a single invert property

In the DSL?
That would introduce a bunch of breaking changes (for example the repr of the Pins and DiffPairs objects), Moreover, the invert property of those DSL object would not be implementable as is (or am I wrong?).
However, I am fine with that as well.

@whitequark
Copy link
Member

whitequark commented Oct 23, 2020

In the DSL?

Internally. Let me review your changes more carefully first, though.

@anuejn
Copy link
Contributor Author

anuejn commented Oct 23, 2020

Okay 👍

Copy link
Member

@whitequark whitequark left a comment

Choose a reason for hiding this comment

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

As promised, I carefully reviewed the PR. Please do the following:

  • Replace the invert attribute in the DSL with a different attribute called flips*. The flips attribute would contain an integer that may be XOR'd with the buffer input or output to apply inversion as appropriate. Both the ~ prefix and the invert= keyword argument would affect this property.

    • It is almost never useful to iterate through the flips one by one (I think that's only used in iCE40), and even if it was, zip() of two lists isn't the nicest way to do it. If that ever becomes an operation we want to simplify, we should just add a custom iterator.
    • We can preserve the invert attribute by deprecating it at the same time as replacing it with a @property that would return False for 0, True for ~0 & ((1 << width) - 1), and raise an exception for any other flip value. Feel free to do it if you think it is valuable; I strongly doubt anyone depends on .invert.
  • Emit a warning if invert= and ~-prefix are used simultaneously (unless maybe there are some cases where it's reasonable...).

  • Revert the addition of vendor.util. The vendor support files are kept separate and with a large amount of redundancy very intentionally. I am aware of the downsides of that. Nevertheless, I am convinced that any common functionality should either be duplicated inline (as it currently happens), or turned into a public API elsewhere (which we aren't ready to do for something like get_ireg).

  • I'm not married to the name flips, but it's the one I dislike the least.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants