Skip to content

Conversation

maribu
Copy link

@maribu maribu commented Mar 17, 2023

The GPIO peripheral of the F4 series has no BRR register, only a BSRR register. Hence, delete all misleading defines.

The GPIO peripheral of the F4 series has no BRR register, only a
BSRR register. Hence, delete all misleading defines.
@TOUNSTM TOUNSTM self-assigned this Apr 25, 2023
@TOUNSTM TOUNSTM added the cmsis CMSIS-related issue or pull-request. label Apr 25, 2023
@TOUNSTM
Copy link
Contributor

TOUNSTM commented Apr 25, 2023

Hello @maribu,

Thank you for this report. We will get back to you as soon as we analyze it further. This may take some time. Thank you for your comprehension.

With regards,

@TOUNSTM
Copy link
Contributor

TOUNSTM commented May 12, 2023

Hello @maribu,

Thank you for your report. Unfortunately, the GPIO_BRR register was defined in previous versions and to avoid any regression in our users' code, our development team decided to keep it in the cmsis device drivers as a legacy definition.

Please allow me to close this thread. Thank you for your comprehension.

With Regards,

@TOUNSTM TOUNSTM closed this May 12, 2023
@TOUNSTM TOUNSTM added the wontfix This will not be worked on label May 12, 2023
@maribu
Copy link
Author

maribu commented May 12, 2023

This fixes a bug. I'm pretty sure you haven't even read the PR description...

@maribu
Copy link
Author

maribu commented May 12, 2023

In case my impression is wrong that this is closed with a copy pasted "customer compatibility" without every looking into this: Let me explain why this is a bug and will break code, but cannot help legacy code written for other family run.

Customer code written for STM32 MCU families with a BRR register will look like this:

    gpio->BRR = GPIO_BRR_BR0 | GPIO_BRR_BR1;

There now is a compatibility GPIO_BRR_BR1 define, but there still is no BRR member in the GPIO_TypeDef, as there is no BRR register. It won't compile. With the compatibility defines one could now use:

    gpio->BSRR = GPIO_BRR_BR0 | GPIO_BRR_BR1;

However, when having to touch that file anyways one could also update the macro names. Second, with mismatching register name and macro name everyone ever looking into this code would directly assume to have spotted a bug: Constants defined for use in the GPIO's BRR register are written into the BSRR register. And one would directly assume that pins are set, rather than reset.

Now let's assume we have code that is written for multiple STM32 MCU families (taking STM32F4 out of the picture here). One may have code looking like this:

void clear_pins(GPIO_TypeDef *gpio, uint32_t pins)
{
#ifdef GPIO_BRR_BR0
    gpio->BRR = pins;
#else
    gpio->BSRR = pins << 16;
#endif
}

This would now compile and run correctly with only BSRR or with both BSRR and BRR being present - except for the STM32F4: There it will fail due to the compatibility flag, as incorrectly the code would assume there is a BRR register despite no such register being present.

This is not a compatibility define easing the live of customers, but a bug that will cause issues.

@TOUNSTM
Copy link
Contributor

TOUNSTM commented May 24, 2023

Dear @maribu,

Thank you for having pointed out this aspect. The solution you suggested will be adopted and made available in a future release.

With regards

@TOUNSTM TOUNSTM reopened this May 24, 2023
@TOUNSTM
Copy link
Contributor

TOUNSTM commented May 25, 2023

Hello @maribu,

I hope you are well. Your request has been submitted to our development team who has concluded that it is more appropriate to keep these legacy definitions. Indeed, some users of STM32F4 series may be using these definitions in their applications.

Even though, this decision may affect portability across series, these definitions must be retained for backwards compatibility in STM32CubeF4 firmware, which is given higher priority in our strategy.

Unfortunately, your request could not be processed this time. I appreciate your understanding and thank you again for having pointed this out.

Please allow me to close this issue now.

With Regards,

@TOUNSTM TOUNSTM closed this May 25, 2023
@TOUNSTM TOUNSTM added the enhancement New feature or request label May 25, 2023
@maribu maribu deleted the GPIO_BRR branch May 25, 2023 15:10
@maribu
Copy link
Author

maribu commented May 25, 2023

Hi,

thanks for looking into this. We have workarounds for the bug implemented and this isn't worth fighting over. But just one final remark:

    gpio->BSRR = GPIO_BRR_BR0 | GPIO_BRR_BR1;

is the only code this defined actually can be helpful. If this would be compiled for any other STM32 family that does have a BRR register, GPIO_BRR_BR0 would have the value 0x1 instead of (0x1 << 16). And there the same code would set the register rather than clear it.

This is a pretty obvious footgun...

Kind regards,
Marian

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cmsis CMSIS-related issue or pull-request. enhancement New feature or request wontfix This will not be worked on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants