Individual Pin Inversion #514
Conversation
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
| @@ -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.") | |||
There was a problem hiding this comment.
| 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.") |
|
I would strongly prefer it if there was only a single |
In the DSL? |
Internally. Let me review your changes more carefully first, though. |
|
Okay 👍 |
There was a problem hiding this comment.
As promised, I carefully reviewed the PR. Please do the following:
-
Replace the
invertattribute in the DSL with a different attribute calledflips*. Theflipsattribute would contain an integer that may be XOR'd with the buffer input or output to apply inversion as appropriate. Both the~prefix and theinvert=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
invertattribute by deprecating it at the same time as replacing it with a@propertythat would returnFalsefor0,Truefor~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.
- 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,
-
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 likeget_ireg). -
I'm not married to the name
flips, but it's the one I dislike the least.
|
@anuejn This PR is pretty old at this point. Are you still interested in pursuing it? |
|
Oh sorry, I completely forgot about this PR. I still really want this functionality, and I am willing to continue. It would be sad if the work that already went into this was lost. That said, I am on holiday for the following two weeks but would continue afterward if it is okay for you. Otherwise also feel free to close this PR for now, and I'll reopen another when I get back to it. |
|
No problem if you want to continue pursuing it. I think the existing RFC, despite not being written quite to the same high standard that we now require, seems to be good enough to pick up working on this feature. (Among my reasons are that it's not a core language feature and the platform system is likely to get a major reshape before 1.0.) |
|
closing this and maybe re-implementing it on top of rfc-55 once it lands |
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?