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

STM32F4_HAL_MMC erase command check is wrong. == has higher precedence than & #4851

Merged
merged 1 commit into from Aug 10, 2017

Conversation

Projects
None yet
6 participants
@janjongboom
Contributor

janjongboom commented Aug 3, 2017

Maybe I'm wrong, but I think the check here is incorrect. This gets evaluated as:

if((hmmc->MmcCard.Class) & (SDIO_CCCC_ERASE == 0U))

Because == has higher precedence than &.

@janjongboom janjongboom force-pushed the janjongboom:stm32f4_wrong_erase branch to 54929f6 Aug 3, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 3, 2017

@adustm

This comment has been minimized.

Member

adustm commented Aug 3, 2017

Hi @janjongboom
Let me rephrase your pull request just to be sure.
According to you:
The current behavior of if((hmmc->MmcCard.Class) & SDIO_CCCC_ERASE == 0U) is :
if((hmmc->MmcCard.Class) & (SDIO_CCCC_ERASE == 0U))

and the expected behavior should be
if(((hmmc->MmcCard.Class) & SDIO_CCCC_ERASE) == 0U)

If my rephrasing is correct, I can check internally within ST team, but you are probably right.
Kind regards
Armelle
ST_INTERNAL_REF 36423

@janjongboom

This comment has been minimized.

Contributor

janjongboom commented Aug 7, 2017

@adustm Yes, your rephrasing is correct.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 7, 2017

@adustm Let us know the feedback from the team. As this changes their driver code, if we accept this change now, and later with the new update will be overwritten?

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Aug 7, 2017

Hi

if we accept this change now, and later with the new update will be overwritten?

It should not :-)
We take into account MBED current code during out HAL code update.

@0xc0170 0xc0170 added needs: CI and removed needs: review labels Aug 7, 2017

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 7, 2017

/morph test

@adustm

adustm approved these changes Aug 7, 2017

@adustm

This comment has been minimized.

Member

adustm commented Aug 7, 2017

Hello @0xc0170 @janjongboom
I just got confirmation from our internal team. You can proceed with this PR.

We'll manage the merge with the official fix when it will be available in a later STM32F4 Cube FW release.
Thank you for the fix. Kind regards
Armelle

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 7, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 952

Build failed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Aug 8, 2017

Restarting, master included one bug that was fixed

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Aug 8, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 962

All builds and test passed!

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Aug 8, 2017

@theotherjimmy theotherjimmy merged commit 95ca299 into ARMmbed:master Aug 10, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment