New fix for HAL/STM32 fastio.h

[Serhiy-K]

Description

After applying the fix #19678 which solved the problem described in #19677, problem with pins, defined as analog in some variant.h files appeared again. This problem was solved in #19614 only partially .

This fix should solve the problem completely.
Tested on STM32F411 board in gebug mode to check right set/reset pins for PA-PC ports. Pins for port A tested as "analog" and digital in variant.h file.

Related Issues

#19614
#19677

[rhapsodyv]

STM docs says: high nibble is the PORT, low nibble is the PIN.

It allow to reference all pins, they just are in different ports.

Marlin has some boards already working in the current code.

What is specifically your problem?

@sjasonsmith can you help reviewing this?

[Serhiy-K]

I described the problem with pins in detail in #19614. This problem is related only to the pins of Port A, which are described as analog in the file variant.h for some boards.

[rhapsodyv]

I described the problem with pins in detail in #19614. This problem is related only to the pins of Port A, which are described as analog in the file variant.h for some boards.

Which board? Can you point me where I can find those "wrong" variant.h?

[Serhiy-K]

I use "BlackPill" devboard with STM32F411 MCU which uses the Generic_F401Cx variant from C:~.platformio\packages\framework-arduinoststm32\variants. The same description of pins as analog is present in other Generic_Fxxxx files. I don't think this is "wrong" variant.h., just such situation was not foreseen in fastio.h.

[sjasonsmith]

I won’t be surprised if there is a real problem. I won’t be able to spend time looking at it until tonight (it is morning right now).

[rhapsodyv]

The code seems a bit strange for me too... because the 2 _READ seems exactly the same.

The original:

#define _READ(IO)               bool(READ_BIT(FastIOPortMap[STM_PORT(digitalPin[IO])]->IDR, _BV32(STM_PIN(digitalPin[IO]))))

STM_PIN(digitalPin[IO])) === digitalPin[IO] & 0xF

The new _READ:

#define _READ(IO)             bool(READ_BIT(FastIOPortMap[STM_PORT(digitalPinToPinName(IO))]->IDR, _BV32(digitalPinToPinName(IO) & 0xF)))

digitalPinToPinName(IO) === (((uint32_t)p < NUM_DIGITAL_PINS) ? digitalPin[p] : NC)... So, the new _READ is:

digitalPinToPinName(IO) & 0xF === digitalPin[p] & 0xF === STM_PIN(digitalPin[IO]))....

Or did I miss something?

[rhapsodyv]

The only different is that I see is digitalPinToPinName can return NC, that is -1...

[thinkyhead]

Side-question: I'm interested in creating a define called NOPIN that equals -1, but should Marlin just use NC instead?

[thinkyhead]

The only different is that I see is digitalPinToPinName can return NC, that is -1...

I don't suppose the compiler will throw an error for FastIOPortMap[-1]…?

Does the _READ macro here need to be changed into an inline function that checks for digitalPinToPinName returning NC and returns LOW in that case (instead of something random)… or is this not a concern?

[sjasonsmith]

I added some cautionary labels, to make sure this doesn’t go in before it has been thoroughly vetted. These STM32 pin issues are certainly a problem, I just want to avoid repeating prior issues where fixing one board broke everything else.

[rhapsodyv]

The only different is that I see is digitalPinToPinName can return NC, that is -1...

I don't suppose the compiler will throw an error for FastIOPortMap[-1]…?

Nop... At least I'm almost sure that it don't do bound checks in C/C++, as everything is "pointer math".

Does the _READ macro here need to be changed into an inline function that checks for digitalPinToPinName returning NC and returns LOW in that case (instead of something random)… or is this not a concern?

The point is: looking at the changes, the code generated seems exactly the code we already have today!

We need to take a deeper look, but I didn't understand what is the problem here and how this may solve anything.

I could be missing something... I will need to do some testing...

[sjasonsmith]

I have not tested this, but I do believe a fix is necessary. I agree with @Serhiy-K that PA2 != PA_2, and it is incorrect to ever use the short PA2 name directly as an index to digitalPin.

This likely explains issues I had with some pins on my STM32F767 board.

[sjasonsmith]

When you look at my comments above, it seems that the only actual change needed is to find/replace digitalPin[IO] with digitalPinToPinName(IO) throughout the entire file.

I see two additional digitalPin uses in pinsDebug_STM32duino.h that probably need the same change.

All the other changes can probably be reversed.

@rhapsodyv previously mentioned that digitalPinToPinName did not do anything different than digitalPin. This is true for the GTR board that he checked.

None of our Marlin-provided variants use the analog method of defining pins. The problem only occurs for the framework-provided variants, which enable a different form of digitalPinToPinName.

[thinkyhead]

I've applied your suggestions, so maybe now it works including pins debugging?

[sjasonsmith]

@thinkyhead this looks correct to me. We should sanity check it with some hardware. @Serhiy-K can you verify whether this still solves your problem?

@rhapsodyv has an F1 board running in this HAL. Perhaps he can do a quick check to be sure this is still working.

I have a variety of hardware, but won’t be able to try anything until late tomorrow.

[Serhiy-K]

In some variant.h files all pins that can be as analog inputs defined as An and they have a pin numbers starting from (NUM_DIGITAL_PINS + 1). In this case digitalPin[IO] returns 0 and WRITE(IO, V), _READ(IO) and _TOGGLE(IO) only change bit 0. This is exactly the problem. digitalPinToPinName(IO) compares the pin number with NUM_DIGITAL_PINS and for pins like An returns the number as for a digital pin.
I'll check the changes made in the evening.

[thinkyhead]

Thanks for the extra documentation about the cause of the issue, and the technique that is used to number analog pins.

Marlin has its own "analog pin numbering" label for pins that use zero-based analog index numbering (e.g., thermistors) and that could be confusing to people just visiting this issue.

Maybe in the future Marlin will assign its analog pins using names like A0-A15 (where applicable) instead so that internally it will more easily be able to distinguish them too.

[Serhiy-K]

I tested the latest fastio.h variant with STM32F411 in gebug mode - it works correctly. I was not able to test on my STM32F103, as for now I can build the firmware only for maple, not STM32. Tomorrow I will try to compile firmware for another board.

[sjasonsmith]

@rhapsodyv you approved changes, did you actually try it on your board?

I didn't have time to test anything today. If @rhapsodyv tried it out then I don't have additional reservations.

[rhapsodyv]

Yes, Nano V2 (F1) keeps working in hal STM32, with the changes of this PR. For me, it can be merged.

[Serhiy-K]

I also tested on STM32F103 in hal STM32 - it works correctly.

[sjasonsmith]

With both @rhapsodyv & @Serhiy-K testing and saying it works, I think this should be ready..