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

Allow a counter to be manually reset #1527

Merged
merged 2 commits into from
Aug 13, 2020

Conversation

nullobject
Copy link
Contributor

@nullobject nullobject commented Jul 29, 2020

This PR adds a reset parameter to the Counter API, that allows a counter to be reset to its initial value.

It is sometimes useful to be able to reset a counter arbitrarily. By default the reset parameter is false, which means the proposed change is compatible with the existing API.

For example

val counterReset = WireInit(false.B)
val (value, wrap) = Counter(0 to 3, reset = counterReset)

// Trigger a reset
counterReset := true.B

Type of change: other enhancement

Impact: API addition (no impact on existing code)

Development Phase: implementation

Release Notes
Allow counters to be reset

@nullobject nullobject changed the title Allow counters to be reset manually Allow a counter to be manually reset Jul 29, 2020
@seldridge
Copy link
Member

With #1515 merged, would you be able to rebase this onto master @nullobject?

And thanks for this. It's something that I've written one-off workarounds on multiple occasions for.

@nullobject
Copy link
Contributor Author

@seldridge Sure thing, I will get it out of draft status and mergable 👍

@nullobject nullobject marked this pull request as ready for review August 5, 2020 03:51
@nullobject nullobject requested a review from a team as a code owner August 5, 2020 03:51
@nullobject nullobject requested review from ducky64 and removed request for a team August 5, 2020 03:51
@nullobject
Copy link
Contributor Author

With #1515 merged, would you be able to rebase this onto master @nullobject?

@seldridge It has been rebased.

Copy link
Member

@seldridge seldridge left a comment

Choose a reason for hiding this comment

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

lgtm

I've wanted this for a while! Thanks for the contribution.

Comment on lines +73 to +76
property("Counter can be reset") {
forAll(smallPosInts) { (seed: Int) => assertTesterPasses{ new ResetTester(seed) } }
}

Copy link
Member

Choose a reason for hiding this comment

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

This test is probably overkill and it seems to repeat (at least locally, for me) seeds. Out of scope for this PR, though. I like having this PR consistent with the other tests here.

@seldridge seldridge added this to the 3.4.0 milestone Aug 13, 2020
@seldridge seldridge added Feature New feature, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI. labels Aug 13, 2020
@mergify mergify bot merged commit d9903de into chipsalliance:master Aug 13, 2020
@nullobject nullobject deleted the counter-reset branch August 13, 2020 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature New feature, will be included in release notes Please Merge Accepted PRs that are ready to be merged. Useful when waiting on CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants