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

Drive invalid in AXI VCs #994

Merged
merged 3 commits into from
Mar 11, 2024
Merged

Drive invalid in AXI VCs #994

merged 3 commits into from
Mar 11, 2024

Conversation

LukasVik
Copy link
Contributor

@LukasVik LukasVik commented Mar 4, 2024

When the 'valid' signal is low for an AXI (-Lite) channel, the signals of the channel should be driven with 'X'.
They should not keep their value from the last time the channel was valid.
This can hide errors in the DUT if the DUT samples values in the wrong clock cycle.

This change is largely inspired by the AXI-Stream VCs.

Note that this changes the default behavior of the three VCs, so in some sense it is a breaking change.
This could be avoided by setting the default value drive_invalid generic to false.
Though having it set to true is symmetric with the AXI-Stream VCs.
And in my personal opinion it should be enabled in pretty much all cases, to get a stronger test.

When the 'valid' signal is low for an AXI (-Lite) channel, the signals of the channel should be driven with 'X'.
They should not keep their value from the last time the channel was valid.
This can hide errors in the DUT if the DUT samples values in the wrong clock cycle.

This change is largely inspired by the AXI-Stream VCs.

Note that this changes the default behavior of the three VCs, so in some sense it is a breaking change.
This could be avoided by setting the default value 'drive_invalid' generic to 'false'.
Though having it set to 'true' is symmetric with the AXI-Stream VCs.
And in my personal opinion it should be enabled in pretty much all cases, to get a stronger test.
@LukasVik LukasVik changed the title Drive invalid Drive invalid in AXI VCs Mar 4, 2024
@LukasVik
Copy link
Contributor Author

LukasVik commented Mar 4, 2024

Note that this is a reopen of #802, which I accidentally closed by deleting my fork, not realizing that this would close the PR. Could not find a way to open that again, so I create this new PR instead.

The diff is exactly the same, which has been reviewed by two people in #802. I have added two commits above, however, that fix some minor formatting and comments.

@LarsAsplund
Copy link
Collaborator

I'm ok with these changes but still some work on the test cases to have them pass. At this point, breaking changes are ok since next release is v5.0.

@LukasVik
Copy link
Contributor Author

Ah sorry, I didn't check CI status. Should be fixed now @LarsAsplund. Thank you!

@LarsAsplund LarsAsplund merged commit 1749a9b into VUnit:master Mar 11, 2024
14 checks passed
@eine eine added this to the v5.0.0 milestone Mar 11, 2024
@umarcor
Copy link
Member

umarcor commented Mar 11, 2024

See #997.

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

Successfully merging this pull request may close these issues.

4 participants