Description
- OS: Win10 22H2
- vsCode + PlatformIO
- STM32 core version: v2.8.1
- Board: Black F407VE
Additional context
I have a quite complex code in hands, since the core don't protect used peripherals(like USB) pins I had to code my own.
I have pins that can be reassign, and need to check is it isn't USB/SPI/UART, when I detect a conflict I set the pin to NUM_DIGITAL_PINS but this is causing the CPU to lock, and the weird thing is, this don't happen while debugging!
After entering the rabbit hole of stepping the code via STM32CubeProgrammer I found the problem is in the digitalRead
, since it don't check if the pin number is valid it causes an exception when the get_GPIO_Port(STM_PORT(pn)
part returns a NULL
and the LL_GPIO_IsInputPinSet
tries to read a NULL offset.
The code also uses EEPROM emulation using internal flash, even when the NULL pointer access don't lock the CPU it causes the flash to stop writing.
The fix is pretty simple, I have tested it right now. Just add a check on the digital manipulation on wiring_digital.c
like this
void digitalWrite(uint32_t ulPin, uint32_t ulVal)
{
PinName p = digitalPinToPinName(ulPin);
if (p != NC) {
digitalWriteFast(p, ulVal);
}
}
This is how pinMode
do it, and the Teensy core also do this check.
Metadata
Metadata
Assignees
Labels
Type
Projects
Status
Activity
fpistm commentedon Dec 20, 2024
Hi @VitorBoss
I understand the issue but wonder if it is a good idea to add a check because it add check every time
digitalWrite
is called.And I guess that's why it is not checked assuming the pin number have to be correct.
Using alias PYn prevent this issue as if it does not exist it fails at build time. So I guess you use a number.
So I see 3 possibilities:
1- do nothing, check have to be done at user application level
2- add the proposed fix
3- add an assert (https://github.com/stm32duino/Arduino_Core_STM32/wiki/HAL-configuration#hal-assertion-management)
fpistm commentedon Dec 20, 2024
Ok found when it was removed, in 2018 😉 in #368.
In this commit: 7eeb731
I guess the assert is the best solution and maybe add an entry in the Debug symbols and core logs to enable easily the assert.
Any feedback are welcome.
VitorBoss commentedon Dec 20, 2024
Yes, i'm using pin numbers as they are stored in eeprom.
The check still persists on
pinMode
, if I pass a invalid number, lets say 255, thepinMode
will fail as it should, but subsequent pin manipulations don't check it again and cause problems, from a performance point of view this would add just 2 instructions.fpistm commentedon Dec 21, 2024
Well, I agree with you anyway, some libraries requires GPIO to be as fast as possible and adding this check will probably change the behavior.
I would recommend to add assert.
Moreover if user (you) do not have time constraints check value of the pin before calling those API.
VitorBoss commentedon Dec 26, 2024
The code is a RT engine management, time isn't that critical but the amount of necessary changes is.
Maybe a macro to enable the check? This would keep current behavior and add the check to who needed it?
fpistm commentedon Dec 31, 2024
Why not. Anyway even if it is a RT engine, I don't understand how you reach this issue as pins are known and fixed so how you reach this? Moreover simply check in user app if pin less than NUM_DIGITAL_PINS easily fix the issue.
VitorBoss commentedon Dec 31, 2024
I think you didn't understood the problem, I'll try to explain better.
Currently all peripherals are in pin numbers range, that means I need to deal with user input, lets say the function is assigned to pin 20, this is USB D+ pin on Black F407VE, I need to change the value so the board don't stop communicating.
Since the pin numbers start from 0 and not all supported boards have the same pin numbers/capacity I need to set the value to an invalid number to avoid locking/bricking another peripheral/device. I can't always check the pin number as some functions uses bit banging.
I accept suggestions on how to deal with this.
VitorBoss commentedon Dec 31, 2024
Just disassembled the binary, the function digitalRead() takes 84 cycles without the change.
This take 100, this is extra 95.2ns at 168MHz(if my math is correct)
fpistm commentedon Jan 2, 2025
Based only on the cycles it adds 19% which is not neglectable.
So adding this check will also impact the bit banging AFAIU.
I admit I don't understand your use case. Have you a simple sketch demonstrating it?
VitorBoss commentedon Jan 10, 2025
This is current code assembly:
With a valid pin number the code take 22 clock cycles to finalize, with analog number it take 23 clock cycles.
This is with change:
With a valid pin number the code take 23 clock cycles to finalize, with analog number it take 32 clock cycles.
As you can see the speed difference is just a single clock cycle with valid pin number below board maximum, inside the analog range the hit is 9 clock cycles, this isn't nothing to sneeze at, but as you can see the protection don't increase the whole 16 instructions it grow.
fpistm commentedon Jan 10, 2025
Hi @VitorBoss
and happy new year.
OK. You convinced me 😉
Please, would you be so kind to create a PR for this?
And I've checked some libraries using GPIO with precise timing and they used LL or *Fast api.
5 remaining items