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

Detect rollover using positions rather than DMA flags #127

Merged
merged 1 commit into from
Sep 14, 2020

Conversation

electroCutie
Copy link
Contributor

I suspect that there is a timing issue with reading the flag then
clearing it in a non-atomic way. We can still use the flag to detect
overruns because it will never give a false positive, but it CAN give a
false negative it seems

Storing the position that we last witnessed the DMA as being at and
performing a wrap when that position is less than our current positon
gives a reliable detection and no fale overruns

We still examining the complete flag and the positions of the current
DMA pointer and our previous one to detect overruns since the fla will
never give a false positive

This is a fix for #104

I suspect that there is a timing issue with reading the flag then
clearing it in a non-atomic way. We can still use the flag to detect
overruns because it will never give a false positive, but it CAN give a
false negative it seems

Storing the position that we last witnessed the DMA as being at and
performing a wrap when that position is less than our current positon
gives a reliable detection and no fale overruns

We still examining the complete flag and the positions of the current
DMA pointer and our previous one to detect overruns since the fla will
never give a false positive
@hannobraun hannobraun self-requested a review September 4, 2020 08:14
@hannobraun
Copy link
Contributor

Thank you, @electroCutie!

I'll need to take a close look and get my gear out for some testing, but I won't be able to get to that this week. I'll try to do it some time next week.

@electroCutie
Copy link
Contributor Author

electroCutie commented Sep 4, 2020

I'll need to take a close look and get my gear out for some testing, but I won't be able to get to that this week

@hannobraun Anything I can check while I'm setup for it?

@hannobraun
Copy link
Contributor

Anything I can check while I'm setup for it?

I don't have anything specific in mind, I just wanted to poke around a bit. I'm sure I'll come up with a more detailed testing plan, as I think more closely about the changes you made :-)

Copy link
Contributor

@hannobraun hannobraun left a comment

Choose a reason for hiding this comment

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

Sorry, it took me a bit longer to get to this than I hoped.

The changes look great! This is exactly the minimally invasive fix that I hoped for, but couldn't find myself. Good job, @electroCutie!

@hannobraun hannobraun merged commit e4f4793 into stm32-rs:master Sep 14, 2020
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