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

gateware: fix an order-of-operations bug with cdc.synchronize #263

Merged
merged 3 commits into from
Jun 8, 2024

Conversation

antoinevg
Copy link
Member

In db75437 a subtle bug was introduced into cdc.synchronize where nested elements of type Pin with directionality output would be recursed into and end up being driven by FFSynchronizer rather than being skipped.

This would result in the signal being driven from multiple fragments: once from the top-level design and again by the undesired FFSynchronizer.

Prior to this change this was not an issue as the recursion operation only applied to elements of type Record.

Moving the directionality check to occur prior to the recursive operation resolves the issue.

@antoinevg antoinevg mentioned this pull request Jun 6, 2024
@antoinevg antoinevg self-assigned this Jun 6, 2024
@antoinevg
Copy link
Member Author

antoinevg commented Jun 6, 2024

Closes #101

@mossmann mossmann requested a review from miek June 6, 2024 15:23
@antoinevg antoinevg force-pushed the antoinevg/fix-cdc branch from afe31a6 to 3907919 Compare June 6, 2024 17:10
…utput signals would not be skipped if they were of Pin type
@antoinevg antoinevg force-pushed the antoinevg/fix-cdc branch from 3907919 to d46963a Compare June 7, 2024 14:59
@antoinevg
Copy link
Member Author

Updated (with bonus unit test!)

@antoinevg antoinevg force-pushed the antoinevg/fix-cdc branch from d46963a to 2d6df09 Compare June 7, 2024 16:31
@miek miek merged commit 640da58 into greatscottgadgets:main Jun 8, 2024
8 checks passed
@antoinevg antoinevg deleted the antoinevg/fix-cdc branch July 4, 2024 09:19
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.

2 participants