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

Move all instances using transceivers to one TX domain (+whole bunch of bug fixes) #716

Merged
merged 18 commits into from
Feb 5, 2025

Conversation

martijnbastiaan
Copy link
Contributor

@martijnbastiaan martijnbastiaan commented Feb 5, 2025

In #688 we modified the transceiver logic to operate on a single TX domain (but multiple RX domains). To keep the changes contained, we still exported multiple clocks still (though just as a repeat). This PR cleans this up and removes all superfluous synchronization. During this process we discovered a number of bugs:

  1. Transceiver up test would only fail if all links would fail. This is of course an error: it should fail if any link fails.
  2. We had two fields called txReady (both on Input and Output). This caused confusion, that lead to the input's txReady to be looped back to the Output. This was clearly incorrect: Output.txReady should clearly depend on whether a neighbor says it is ready to receive data!

Fixes #696

TODO

  • See if we can replicate handhshake failure in simulation test

@martijnbastiaan martijnbastiaan force-pushed the fix-transceiver-test-new2 branch 2 times, most recently from cf56e84 to e5a48e1 Compare February 5, 2025 12:18
@martijnbastiaan martijnbastiaan marked this pull request as ready for review February 5, 2025 12:32
@martijnbastiaan martijnbastiaan force-pushed the fix-transceiver-test-new2 branch from e5a48e1 to 2393ad0 Compare February 5, 2025 12:39
@martijnbastiaan martijnbastiaan force-pushed the fix-transceiver-test-new2 branch from 2393ad0 to 5d53bee Compare February 5, 2025 12:47
Copy link
Contributor

@hiddemoll hiddemoll left a comment

Choose a reason for hiding this comment

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

Discussed the only 2 comments I had with Martijn, who quickly fixed it.
Imo all changes make sense are the commit history clearly shows the process. I also know that the transceivers are really complex, and I don't have enough transceiver-knowledge to spot any errors in the current code.

If you found any potential issues in the transceiver code now, or other things you'd like to fix in the future, make issues now that they are fresh in your heads.

@martijnbastiaan
Copy link
Contributor Author

Thanks :). Virtually all code was written by @leonschoorl, I just made the PR. It also has my seal of approval.

If you found any potential issues in the transceiver code now, or other things you'd like to fix in the future, make issues now that they are fresh in your heads.

We haven't found anything we didn't fix.

@martijnbastiaan martijnbastiaan merged commit 1703f40 into staging Feb 5, 2025
26 checks passed
@martijnbastiaan martijnbastiaan deleted the fix-transceiver-test-new2 branch February 5, 2025 14:49
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.

Remove redundant logic dealing with multiple TX domains
3 participants