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

nmigen.lib: Add RoundRobin #450

merged 20 commits into from Jul 28, 2020

Conversation

@jeanthom
Copy link
Contributor

@jeanthom jeanthom commented Jul 24, 2020

Fixes #448

nmigen/compat/genlib/roundrobin.py Outdated Show resolved Hide resolved
Loading
@jeanthom jeanthom marked this pull request as draft Jul 24, 2020
@jeanthom jeanthom marked this pull request as ready for review Jul 24, 2020
@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 Jul 24, 2020
@jeanthom
Copy link
Contributor Author

@jeanthom jeanthom commented Jul 24, 2020

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

Loading

@jeanthom jeanthom marked this pull request as ready for review Jul 24, 2020
@jeanthom jeanthom changed the title [WIP] nmigen.lib: Add RoundRobin nmigen.lib: Add RoundRobin Jul 24, 2020
nmigen/lib/roundrobin.py Outdated Show resolved Hide resolved
Loading
nmigen/lib/roundrobin.py Outdated Show resolved Hide resolved
Loading
nmigen/lib/roundrobin.py Outdated Show resolved Hide resolved
Loading
nmigen/compat/genlib/roundrobin.py Outdated Show resolved Hide resolved
Loading
nmigen/lib/roundrobin.py Outdated Show resolved Hide resolved
Loading
nmigen/lib/roundrobin.py Outdated Show resolved Hide resolved
Loading
nmigen/lib/roundrobin.py Outdated Show resolved Hide resolved
Loading
nmigen/lib/roundrobin.py Outdated Show resolved Hide resolved
Loading
"""Round-robin scheduler.
For a given set of requests, the round-robin scheduler will
grant one request.
Copy link
Member

@whitequark whitequark Jul 28, 2020

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.

Loading

Copy link
Contributor Author

@jeanthom jeanthom Jul 28, 2020

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?

Loading

nmigen/lib/roundrobin.py Outdated Show resolved Hide resolved
Loading
@whitequark
Copy link
Member

@whitequark whitequark commented Jul 28, 2020

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?

Loading

@jeanthom
Copy link
Contributor Author

@jeanthom jeanthom commented Jul 28, 2020

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?

Loading

@whitequark
Copy link
Member

@whitequark whitequark commented Jul 28, 2020

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.

Loading

nmigen/compat/genlib/roundrobin.py Outdated Show resolved Hide resolved
Loading
nmigen/compat/genlib/roundrobin.py Outdated Show resolved Hide resolved
Loading
Jean THOMAS and others added 2 commits Jul 28, 2020
Co-authored-by: whitequark <whitequark@whitequark.org>
Co-authored-by: whitequark <whitequark@whitequark.org>
nmigen/lib/scheduler.py Outdated Show resolved Hide resolved
Loading
Copy link
Member

@whitequark whitequark left a comment

Thanks!

Loading

@whitequark whitequark merged commit 20f9ab9 into nmigen:master Jul 28, 2020
5 checks passed
Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants