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: RX Stall may occur locking the system #15

Merged
merged 1 commit into from
Sep 5, 2022

Conversation

ithinuel
Copy link
Member

@ithinuel ithinuel commented Sep 4, 2022

A the end of a writing operation with large amount of data, the tx fifo may get filled and then process enters the wait_idle method.

However, while the TX fifo gets emptied, the RX fifo gets filled and it may cause an RX stall, effectively stopping the state machine, and preventing the idle condition from ever raising.

A the end of a writing operation with large amount of data, the tx fifo may
get filled and then process enters the wait_idle method.
However, while the TX fifo gets emptied, the RX fifo gets filled and it
may cause an RX stall, effectively stopping the state machine, and preventing
the idle condition from ever raising.
Copy link
Member

@9names 9names left a comment

Choose a reason for hiding this comment

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

Lgtm! Nice explanation of the problem

@jannic
Copy link
Member

jannic commented Sep 5, 2022

Definitely a good catch!

What I don't like is that the new code just drops RX data, but does that unreliably: It depends on timing whether all incoming data is dropped before the function returns. For a public function I'd search for a better solution which either doesn't drop RX (return some error instead and let the caller handle it), or always drops all RX data. (The latter can't be 100% deterministic as it can't prevent new RX from appearing between the last check and the function return.)

For this library, I'd say this is fine: There are only two callers of this function, one clears RX anyways, and the other one is in some error branch on finish.

Regarding the error case: If one transaction ends with an error in finish, a subsequent call to setup could panic because of this code:

        // flush read fifo
        assert!(self.rx.read().is_none(), "rx FIFO shall be empty");

Shouldnt setup actually flush the RX FIFO instead of panicing?

In any case: This is independent from this pull request → LGTM.

@ithinuel ithinuel merged commit 23e3255 into rp-rs:main Sep 5, 2022
@ithinuel ithinuel deleted the fix-rx-stall branch September 5, 2022 06:47
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.

3 participants