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

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

Merged
merged 1 commit into from May 19, 2017

Conversation

Projects
None yet
8 participants
@Pliny
Contributor

Pliny commented May 3, 2017

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

This comment has been minimized.

Member

screamerbg commented May 4, 2017

@screamerbg screamerbg requested review from bcostm and betzw May 4, 2017

@betzw

This comment has been minimized.

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.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 11, 2017

Any update fro this pull request?

@LMESTM

This comment has been minimized.

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 ?

@Pliny

This comment has been minimized.

Contributor

Pliny commented May 11, 2017

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

This comment has been minimized.

Contributor

LMESTM commented May 11, 2017

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

@Pliny

This comment has been minimized.

Contributor

Pliny commented May 11, 2017

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

This comment has been minimized.

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

stm32f4xx: Revert #3424
According to @betzw, #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.

@Pliny Pliny force-pushed the Pliny:master branch to 42f66ee May 12, 2017

@Pliny

This comment has been minimized.

Contributor

Pliny commented May 12, 2017

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

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 15, 2017

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

@LMESTM

This comment has been minimized.

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

@betzw

betzw approved these changes May 16, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented May 16, 2017

/morph test

@0xc0170 0xc0170 added needs: CI and removed needs: review labels May 16, 2017

@mbed-bot

This comment has been minimized.

mbed-bot commented May 17, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 236

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels May 17, 2017

@sg-

This comment has been minimized.

Member

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

@Pliny

This comment has been minimized.

Contributor

Pliny commented May 18, 2017

@sg- sg- merged commit a2a1581 into ARMmbed:master May 19, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@sg- sg- removed the ready for merge label May 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment