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

Fix bug related to clock divider simulation, fix #191 #192

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mkorbel1
Copy link
Contributor

@mkorbel1 mkorbel1 commented Nov 1, 2022

Description & Motivation

If the output of one Sequential is used as a clock trigger to another Sequential, the phased simulator can sometimes detect the edge and drive the outputs of the downstream Sequential one tick late.

This is because there was a gate in the Sequential clock sampling logic that ignored clock toggles during the clkStable phase. Instead, we should immediately execute the flop if we're already in clkStable.

Related Issue(s)

Fix #191

Testing

Added a new test where two counters are driven by an original clock and that clock divided, respectively.

Backwards-compatibility

Is this a breaking change that will not be backwards-compatible? If yes, how so?

No

Documentation

Does the change require any updates to documentation? If so, where? Are they included?

No

@mkorbel1 mkorbel1 requested a review from chaparalas November 1, 2022 17:19
Copy link
Contributor

@chaparalas chaparalas left a comment

Choose a reason for hiding this comment

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

Consider adding a async-reset clk-div testcase.

@mkorbel1
Copy link
Contributor Author

mkorbel1 commented Nov 4, 2022

Consider adding a async-reset clk-div testcase.

Added in latest commit, good idea, thanks

@mkorbel1 mkorbel1 marked this pull request as draft November 23, 2022 17:38
@mkorbel1
Copy link
Contributor Author

Need to add more testing around chains of clock dividers, which are broken with current implementation in this PR. Likely solution is to use topological sort to pick the order of Sequential execution.

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

Successfully merging this pull request may close these issues.

Sequential outputs as clock inputs to other Sequentials trigger late in the simulator
2 participants