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

Bikeshed: conventions for CDC primitives #97

Closed
whitequark opened this issue Jun 11, 2019 · 4 comments
Closed

Bikeshed: conventions for CDC primitives #97

whitequark opened this issue Jun 11, 2019 · 4 comments
Labels
Milestone

Comments

@whitequark
Copy link
Contributor

whitequark commented Jun 11, 2019

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

@sbourdeauducq
Copy link
Member

Sounds ok.

Wren6991 added a commit to Wren6991/nmigen that referenced this issue Jun 28, 2019
@Wren6991
Copy link
Contributor

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.

@whitequark
Copy link
Contributor Author

whitequark commented Jun 28, 2019

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

@whitequark
Copy link
Contributor Author

whitequark commented Sep 12, 2019

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

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

No branches or pull requests

3 participants