Skip to content

nmigen.lib: Add RoundRobin#450

Merged
whitequark merged 20 commits intoamaranth-lang:masterfrom
jeanthom:roundrobin
Jul 28, 2020
Merged

nmigen.lib: Add RoundRobin#450
whitequark merged 20 commits intoamaranth-lang:masterfrom
jeanthom:roundrobin

Conversation

@jeanthom
Copy link
Copy Markdown
Contributor

Fixes #448

Comment thread nmigen/compat/genlib/roundrobin.py Outdated
@jeanthom jeanthom marked this pull request as draft July 24, 2020 10:20
@jeanthom jeanthom marked this pull request as ready for review July 24, 2020 10:39
@jeanthom jeanthom changed the title [WIP] nmigen.lib: Add RoundRobin nmigen.lib: Add RoundRobin Jul 24, 2020
@jeanthom jeanthom changed the title nmigen.lib: Add RoundRobin [WIP] nmigen.lib: Add RoundRobin Jul 24, 2020
@jeanthom jeanthom marked this pull request as draft July 24, 2020 11:22
@jeanthom
Copy link
Copy Markdown
Contributor Author

Currently implementing @jfng's suggestions (code styling, better doc, etc.).

@jeanthom jeanthom marked this pull request as ready for review July 24, 2020 12:31
@jeanthom jeanthom changed the title [WIP] nmigen.lib: Add RoundRobin nmigen.lib: Add RoundRobin Jul 24, 2020
Comment thread nmigen/lib/roundrobin.py Outdated
Comment thread nmigen/lib/roundrobin.py Outdated
Comment thread nmigen/lib/roundrobin.py Outdated
Comment thread nmigen/compat/genlib/roundrobin.py Outdated
Comment thread nmigen/lib/roundrobin.py Outdated
Comment thread nmigen/lib/roundrobin.py Outdated
Comment thread nmigen/lib/roundrobin.py Outdated
Comment thread nmigen/lib/roundrobin.py Outdated
Comment thread nmigen/lib/roundrobin.py Outdated
"""Round-robin scheduler.

For a given set of requests, the round-robin scheduler will
grant one request.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I feel like this should be elaborated. Something like (phrasing needs to be improved):

For a given set of requests, the round-robin scheduler will grant one request. Once it grants a request, if any other requests are active, it grants the next active request with a greater number, restarting from zero once it reaches the highest one. Use :class:EnableInserter to control when the scheduler advances.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I tweaked a little bit what you wrote. What do you think?

Comment thread nmigen/lib/roundrobin.py Outdated
@whitequark
Copy link
Copy Markdown
Member

Great, thanks for the work! There's only one remaining concern I have: should RoundRobin live in its own module? Do we plan to have other schedulers? Or perhaps should it be categorized differently?

@jeanthom
Copy link
Copy Markdown
Contributor Author

That's the way it was in omigen because there was only one scheduler. I seriously doubt that we're going to have other schedulers in nMigen, but just in case we could name the module scheduler? Or keep it this way until someone has another scheduler to offer?

@whitequark
Copy link
Copy Markdown
Member

I seriously doubt that we're going to have other schedulers in nMigen, but just in case we could name the module scheduler? Or keep it this way until someone has another scheduler to offer?

I think we can name the module scheduler and at the same time accept that there likely won't ever be a second one. The module name is still nice because from nmigen.lib.scheduler import RoundRobin is more clear on its purpose than from nmigen.lib.roundrobin import RoundRobin.

Comment thread nmigen/compat/genlib/roundrobin.py Outdated
Comment thread nmigen/compat/genlib/roundrobin.py Outdated
Jean THOMAS and others added 2 commits July 28, 2020 22:32
Co-authored-by: whitequark <whitequark@whitequark.org>
Co-authored-by: whitequark <whitequark@whitequark.org>
Comment thread nmigen/lib/scheduler.py Outdated
Copy link
Copy Markdown
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.

Thanks!

@whitequark whitequark merged commit 20f9ab9 into amaranth-lang:master Jul 28, 2020
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.

Adding RoundRobin to nmigen.lib

2 participants