Bikeshed: conventions for CDC primitives

[nmigen-issue-migration]

Issue by whitequark
Tuesday Jun 11, 2019 at 04:06 GMT
Originally opened as m-labs/nmigen#97

---

One reason I've hesitated to merge #40 so far is that I think the oMigen conventions for CDC primitives are a bit annoying. I think a better situation than the current one would be:

• Instead of arguments named domain, idomain, rdomain, etc, which only allow a single letter for domain designation, use domain, i_domain, r_domain, pll_domain, etc.
• All modules which are likely to be transformed with DomainRenamer right after creation should allow changing the domain names through arguments. Notably this would include AsyncFIFO, which is probably responsible for most uses of DomainRenamer, but all other CDC primitives as well. Non-CDC modules may or may not allow that depending on intended use.

Thoughts? @sbourdeauducq @jordens @Wren6991

[nmigen-issue-migration]

Comment by sbourdeauducq
Tuesday Jun 11, 2019 at 04:14 GMT

---

Sounds ok.

[nmigen-issue-migration]

Comment by Wren6991
Friday Jun 28, 2019 at 05:52 GMT

---

I like this, but find some of the results inconsistent (c84cd8d):

class Gearbox(Elaboratable):
   ...
    def __init__(self, iwidth, cd_i, owidth, cd_o):

Inconsistent use of suffix vs prefix is something I would trip over when typing too fast.

[nmigen-issue-migration]

Comment by whitequark
Friday Jun 28, 2019 at 05:53 GMT

---

I think Gearbox should of course use width_i and width_o here, for pretty much the same reasons as cd_i etc.

[nmigen-issue-migration]

Comment by whitequark
Thursday Sep 12, 2019 at 20:08 GMT

---

Since nMigen uses domain and not cd everywhere in the public API, I changed the convention slightly to match that, see original comment.

I've adjusted lib.cdc for this convention since it was not very invasive. I've also adjusted lib.fifo for this convention, and also have consistent naming elsewhere, but that is a larger diff, so I haven't committed it to master yet. Diff.

Feedback? @sbourdeauducq @Wren6991