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

nmigen.lib: Add RoundRobin #450

Merged
merged 20 commits into from Jul 28, 2020
Merged

Conversation

jeanthom
Copy link
Contributor

Fixes #448

@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
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
"""Round-robin scheduler.

For a given set of requests, the round-robin scheduler will
grant one request.
Copy link
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
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?

@whitequark
Copy link
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
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
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.

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>
nmigen/lib/scheduler.py Outdated Show resolved Hide resolved
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.

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