Pin manipulation lock board when number is equal/above NUM_DIGITAL_PINS

[VitorBoss]

• 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.

[fpistm]

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]

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]

Yes, i'm using pin numbers as they are stored in eeprom.

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.
...
Ok found when it was removed, in 2018 😉 in #368.

The check still persists on pinMode, if I pass a invalid number, lets say 255, the pinMode 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]

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]

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]

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]

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]

Just disassembled the binary, the function digitalRead() takes 84 cycles without the change.

int digitalRead(uint32_t ulPin)
{
  PinName p = digitalPinToPinName(ulPin);

  if (p == NC) {
    return 0;
  }
  return digitalReadFast(p);
}

This take 100, this is extra 95.2ns at 168MHz(if my math is correct)

[fpistm]

Based only on the cycles it adds 19% which is not neglectable.

I can't always check the pin number as some functions uses bit banging.

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]

This is current code assembly:

.text:0802E2DC                 EXPORT digitalRead
.text:0802E2DC digitalRead                             ; CODE XREF: checkForSDStart(void)+8E↑p
.text:0802E2DC                                         ; checkForSDStop(void)+BE↑p ...
.text:0802E2DC                 UXTB    R2, R0
.text:0802E2DE                 CMP     R2, #0x4A ; 'J'
.text:0802E2E0                 PUSH    {R4,LR}
.text:0802E2E2                 LDR     R1, =digitalPin
.text:0802E2E4                 AND.W   R3, R0, #0x700
.text:0802E2E8                 BHI     loc_802E2FE
.text:0802E2EA
.text:0802E2EA loc_802E2EA                             ; CODE XREF: digitalRead+38↓j
.text:0802E2EA                 LDR.W   R2, [R1,R2,LSL#2]
.text:0802E2EE                 ORRS    R3, R2
.text:0802E2F0                 UBFX.W  R2, R3, #4, #4
.text:0802E2F4                 CMP     R2, #8
.text:0802E2F6                 BLS     loc_802E316
.text:0802E2F8
.text:0802E2F8 loc_802E2F8                             ; CODE XREF: digitalRead+28↓j
.text:0802E2F8                                         ; digitalRead+2C↓j
.text:0802E2F8                 MOVS    R3, #0
.text:0802E2FA                 LDR     R3, [R3,#0x10]
.text:0802E2FC                 UND     #0xFF
.text:0802E2FE ; ---------------------------------------------------------------------------
.text:0802E2FE
.text:0802E2FE loc_802E2FE                             ; CODE XREF: digitalRead+C↑j
.text:0802E2FE                 AND.W   R4, R0, #0xC0
.text:0802E302                 CMP     R4, #0xC0
.text:0802E304                 BNE     loc_802E2F8
.text:0802E306                 CMP     R2, #0xCF
.text:0802E308                 BHI     loc_802E2F8
.text:0802E30A                 LDR     R2, =analogInputPin
.text:0802E30C                 AND.W   R0, R0, #0x3F
.text:0802E310                 LDR.W   R2, [R2,R0,LSL#2]
.text:0802E314                 B       loc_802E2EA
.text:0802E316 ; ---------------------------------------------------------------------------
.text:0802E316
.text:0802E316 loc_802E316                             ; CODE XREF: digitalRead+1A↑j
.text:0802E316                 LDR     R1, =GPIOPort
.text:0802E318                 AND.W   R3, R3, #0xF
.text:0802E31C                 LDR.W   R2, [R1,R2,LSL#2]
.text:0802E320                 LDR     R1, =pin_map_ll
.text:0802E322                 LDR     R2, [R2,#0x10]
.text:0802E324                 LDR.W   R3, [R1,R3,LSL#2]
.text:0802E328                 BICS    R3, R2
.text:0802E32A                 ITE EQ
.text:0802E32C                 MOVEQ   R0, #1
.text:0802E32E                 MOVNE   R0, #0
.text:0802E330                 POP     {R4,PC}
.text:0802E330 ; End of function digitalRead

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:

.text:0802E2DC                 EXPORT digitalRead
.text:0802E2DC digitalRead
.text:0802E2DC                 UXTB    R3, R0
.text:0802E2DE                 CMP     R3, #0x4A ; 'J'
.text:0802E2E0                 BHI     loc_802E316
.text:0802E2E2                 LDR     R2, =digitalPin
.text:0802E2E4                 LDR.W   R3, [R2,R3,LSL#2]
.text:0802E2E8                 AND.W   R0, R0, #0x700
.text:0802E2EC
.text:0802E2EC loc_802E2EC                             ; CODE XREF: digitalRead+5A↓j
.text:0802E2EC                 ORRS    R0, R3
.text:0802E2EE                 ADDS    R3, R0, #1
.text:0802E2F0                 BEQ     loc_802E338
.text:0802E2F2                 UBFX.W  R3, R0, #4, #4
.text:0802E2F6                 CMP     R3, #8
.text:0802E2F8                 BHI     loc_802E33C
.text:0802E2FA                 LDR     R2, =GPIOPort
.text:0802E2FC                 AND.W   R0, R0, #0xF
.text:0802E300                 LDR.W   R2, [R2,R3,LSL#2]
.text:0802E304                 LDR     R3, =pin_map_ll
.text:0802E306                 LDR     R2, [R2,#0x10]
.text:0802E308                 LDR.W   R3, [R3,R0,LSL#2]
.text:0802E30C                 BICS    R3, R2
.text:0802E30E                 ITE EQ
.text:0802E310                 MOVEQ   R0, #1
.text:0802E312                 MOVNE   R0, #0
.text:0802E314                 BX      LR
.text:0802E316 ; ---------------------------------------------------------------------------
.text:0802E316
.text:0802E316 loc_802E316                             ; CODE XREF: digitalRead+4↑j
.text:0802E316                 AND.W   R2, R0, #0xC0
.text:0802E31A                 CMP     R2, #0xC0
.text:0802E31C                 BNE     loc_802E338
.text:0802E31E                 CMP     R3, #0xCF
.text:0802E320                 BHI     loc_802E338
.text:0802E322                 LDR     R3, =analogInputPin
.text:0802E324                 AND.W   R2, R0, #0x3F
.text:0802E328                 AND.W   R0, R0, #0x700
.text:0802E32C                 LDR.W   R2, [R3,R2,LSL#2]
.text:0802E330                 LDR     R3, =digitalPin
.text:0802E332                 LDR.W   R3, [R3,R2,LSL#2]
.text:0802E336                 B       loc_802E2EC
.text:0802E338 ; ---------------------------------------------------------------------------
.text:0802E338
.text:0802E338 loc_802E338                             ; CODE XREF: digitalRead+14↑j
.text:0802E338                                         ; digitalRead+40↑j ...
.text:0802E338                 MOVS    R0, #0
.text:0802E33A                 BX      LR
.text:0802E33C ; ---------------------------------------------------------------------------
.text:0802E33C
.text:0802E33C loc_802E33C                             ; CODE XREF: digitalRead+1C↑j
.text:0802E33C                 MOVS    R3, #0
.text:0802E33E                 LDR     R3, [R3,#0x10]
.text:0802E340                 UND     #0xFF
.text:0802E340 ; End of function digitalRead

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]

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.