Skip to content

stm32f4xx: Consider all DMA ready/busy states in conditionals #4263

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 19, 2017
Merged

stm32f4xx: Consider all DMA ready/busy states in conditionals #4263

merged 1 commit into from
May 19, 2017

Conversation

davedesro
Copy link
Contributor

Fix for #4262

This fixes potential corner case issues relating to
stopping, starting, init/deinit, and setting callbacks.

The issue is only found with the stm32f4xx targets.

@screamerbg
Copy link
Contributor

CC @bcostm @betzw

@screamerbg screamerbg requested review from bcostm and betzw May 4, 2017 09:26
@betzw
Copy link
Contributor

betzw commented May 4, 2017

cc @LMESTM

This is most likely a side-effect of the DMA patch for I2S.
The latest version of the I2S library works without this patch, so we could try to restore the original DMA HAL implementation/files and then ask @pliny if he still has issues like described in #4262.

@betzw betzw mentioned this pull request May 4, 2017
Closed
@0xc0170
Copy link
Contributor

0xc0170 commented May 11, 2017

Any update fro this pull request?

@LMESTM
Copy link
Contributor

LMESTM commented May 11, 2017

@pliny Could consider the proposal from Wolfgang as an alternative: in other words, may you try to revert #4262 and see if this solves your issue ?

@davedesro
Copy link
Contributor Author

I found this while debugging issue #4261. It was found by code review, so I don't have a focused test exposing it.

@LMESTM. I pulled the latest mbed-os, but didn't see any recent changes by @betzw relating to I2S and DMA in STM32F4. What I2S code change are you referring to?

@LMESTM
Copy link
Contributor

LMESTM commented May 11, 2017

@pliny oh sorry, the change of @betzw was actually pushed by bcostm in #3424

@davedesro
Copy link
Contributor Author

Ah ok, thanks. So the way I see it, the PR for #3424 is incomplete as it should have also included the patch submitted here.

Is @betzw suggesting #3424 can be reverted? That would work too.

@betzw
Copy link
Contributor

betzw commented May 12, 2017

Yes @pliny, I am suggesting to revert #3424 and let you then test things are working again on your side.
Maybe you could try the revert locally on your side and let us know before we submit the PR, what do you think?

cc @bcostm, @LMESTM

@davedesro
stm32f4xx: Revert ARMmbed#3424
42f66ee 
According to @betzw, ARMmbed#3424 was put in for I2S with DMA.  However, the latest I2S library now works without this patch.

The changes in DMA HAL for this potentially introduced corner case
scenarios. So it's best to revert the DMA changes.
@davedesro
Copy link
Contributor Author

Yes, reverting #3424 fixes my issue with HAL_DMA_Abort(). You can see the changes I made in my latest commit.

@0xc0170
Copy link
Contributor

0xc0170 commented May 15, 2017

@betzw @LMESTM Can you please review the latest change?

@LMESTM
Copy link
Contributor

LMESTM commented May 15, 2017

That's the revert of the changes suggested by @betzw - so as both @betzw and @pliny are happy with the changes - I'm for it ! +1

@0xc0170
Copy link
Contributor

0xc0170 commented May 16, 2017

/morph test

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 236

All builds and test passed!

@sg-
Copy link
Contributor

sg- commented May 18, 2017

@pliny Have you signed the CLA? Can you link to your developer.mbed.org account showing you have accepted? https://developer.mbed.org/contributor_agreement/

@sg- sg- added the needs: CLA label May 18, 2017
@davedesro
Copy link
Contributor Author

Done. https://developer.mbed.org/users/dsdesroc/

@sg- sg- merged commit a2a1581 into ARMmbed:master May 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@betzw betzw betzw approved these changes

@bcostm bcostm Awaiting requested review from bcostm

Assignees
No one assigned
Labels
release-version: 5.4.6
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@davedesro @screamerbg @betzw @0xc0170 @LMESTM @mbed-bot @sg- @theotherjimmy