NUCLEO_F207ZG extending PeripheralPins.c: all available alternate functions can be used now

[ohagendorf]

Description

Through some minor extensions it is now possible to use all available alternate functions of a specific gpio pin. These alternatives exist up to now only as commented lines in PeripheralPins.c.
An API change is not necessary for this new functionality, only several pin definitions were added.

The new definitions now looks like:

{PA_0,       ADC_1, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 0, 0)},  // ADC1_IN0
{PA_0_ALT0,  ADC_2, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 0, 0)},  // ADC2_IN0 // choice: PA_0 with ADC_1
{PA_0_ALT1,  ADC_3, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 0, 0)},  // ADC3_IN0 // choice: PA_0 with ADC_1

PA_0, PA_0_ALT0 or PA_0_ALT1 has to be used as pin names for the usage of the three possible ADC blocks (ADC1, ADC2, ADC3) connected to the pin (PA_0).

Status

IN DEVELOPMENT

Migrations

NO API changes

Before this PR only specific alternate function could be used, e.g.:

AnalogIn analogInput(PA_0);

Now all avalibale alternatives can be used, e.g.

AnalogIn analogInput(PA_0_ALT0);

or

AnalogIn analogInput(PA_0_ALT1);

The original behaviour was not changed.

Todos

This is an extension to be discussed, maybe @bcostm can have a look at it.

Deploy notes

This PR doen't required changes in the build environment, tools, compilers, etc.

Steps to test or reproduce

Please see the example above.

[bcostm]

Hi,
This is a really good idea, I like it :)

But we need to find a way to change all these files automatically because to do this for all platforms requires a lot of work... We already have a python script used to generate the PeripheraPins.c files. We'll see how to improve it.

Another point concerns the labels written in the pinout images. For example we have today PWM9/2 on pin PA_3. If we add these alternate pins we'll have to add also PWM2/4 and it will be difficult to fit all these labels... So maybe we'll have to simply put "PWM"....

cc @adustm

[ohagendorf]

Maybe another thing can also be discussed.

All pins which are connected to a peripheral at a discovery/nucleo board is commented. In most cases this is OK. But the PWM can be used very well at a gpio with a connected LED.

I think it would be a good idea to enable the PWM config for the LED pins.

[LMESTM]

@adustm @bcostm
@ohagendorf : thanks for your proposal.

I understand the concept and wish of users to get a fully flexible pin map. Nevertheless I feel like the proposed change would not be so easy to explain and document to every users.

Typically how to document the PA_0_ALT1 definition ? This is actually not a PIN, the only existing pin is PA_0.
AnalogIn analogInput(PA_0_ALT1);

To fully address the point it may be best to request MBED API change to be able to specify a specific IP instance. To be explicit:
The default would be
AnalogIn analogInput(PA_0); // leads to using ADC_1
Another constructor would specify the instance:
AnalogIn analogInput(PA_0, ADC2); // clearly state I want to use ADC2

This is I think easier to understand and more generic.
In the other hand, the obvious drawback is the need for updating all the APIs :-(

In any case I think this should be discussed with MBED experts to get their feedback and offer a consistent approach.

final note: your current PR could also be temporarily accepted in a first step, as there isn't any visible conflict between our 2 proposals and they could even live together.

[ohagendorf]

@LMESTM I also had your idea or a very similar one, extending the API. I realized it and was not so satisfied - because of the API change. The result of this attempt looks like this:

AnalogIn analogInput(PA_0);
AnalogIn analogInput(PA_0, 0); // same as without 0

AnalogIn analogInput(PA_0, 1); // for 1st alternative
AnalogIn analogInput(PA_0, 2); // for 2nd alternative
...

with the following PeripheralPins.c definition:

{PA_0,  ADC_1, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 0, 0)},  // ADC1_IN0
{PA_0,  ADC_2, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 0, 0)},  // ADC2_IN0 // choice: PA_0 with ADC_1
{PA_0,  ADC_3, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 0, 0)},  // ADC3_IN0 // choice: PA_0 with ADC_1

The search function looks for PA_0 and uses the 1st located line when parameter is missing or equals 0, 2nd line when parameter equals 1, ...

The necessary changes where not too overwhelming.

One idea for a documentation of the changes in this PR: using the alternate function number from the datasheet (AFx instead of my ALTx). But this is not consistent: the different ADCs are not really alternate functions. That's the reaseon I used ALT instead of AF.

I've been thinking about this problem for a longer time and discussed it with collegues. We already had several times the problem, that the specific block, timer, adc, ... we wanted to use was commented. But I didn't had better ideas than these two.

[bcostm]

The documentation of the Px_y_ALTz pins is quite easy to do. We can add a brief description near the pinout in the platform page. It is done today for the other pins described in the PinNames.h file for example. What would be nice also is to add a link to the PeripheralPins.c file. The user will then have access to all the available pins.

i.e.
You can also use other pins named "Px_y_ALTz". The suffix "ALTz" corresponds to other pins alternatives. Please see the PeripheralPins.c file for a complete description of all available pins.

The problem is that this link is not fixed as it depends of the mbed-dev library revision. It will be difficult to update all these links at each mbed-dev version...

[LMESTM]

@ohagendorf as mentioned earlier, I 'm not fully satisfied with the proposal, but I'm okay to proceed further with it until we find something better. At least advanced users will know how to use it in a first step as it doesn't break existing code.

@bcostm About document, we may generate a readable table to be added to the board html page on top of the existing drawings?

[ohagendorf]

@LMESTM @bcostm
If you want I could send a PR with the API extension. I realized it a longer time ago, so it needs a rebase and maybe some rework. But it worked as expected.

I would appreciate such a table, the best would be the PeripheralPins.c in a shortened form.

[0xc0170]

@ohagendorf What is this PR trying to solve (might need more details reasoning behind ALT0 or ATL1, why a user should care if it's 0 or 1, its the same pin) ? What are the limitations of ADC for instance in this case for P0 pin?
As a user, I got a pin that I want to use. Why should I be concerned if that pin is wired to ADC_0 or ADC_1 ?

I recall maxim provides similar functionality, look at the pwm pinmaps: https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_Maxim/TARGET_MAX32610/PeripheralPins.c#L131, the implementation https://github.com/ARMmbed/mbed-os/blob/master/targets/TARGET_Maxim/TARGET_MAX32610/pwmout_api.c#L57

cc @jeremybrodt (might be interested in this API discussion).

[ohagendorf]

This functionality is a bit more interesting for the PWM pins. The assignment between pins and timers is slightly more fixed i.e. I can't choose a pin and use it for any timer as an output. The MAX32610 seems to have another pin function assignment functionality.

Maybe I want to use a specific timer. As far as I know each timer in a STM32 mcu has 4 PWM channels. But in PeripheralPins.c the assignement is restricted, only a few of all possible timer outputs are usable (not commented) because the pin name is used as an index into the table. That's why the pin name has to be unique.

Up to know I have to change the commented lines manually. With this PR I can use every possible combination.

[ohagendorf]

I forgot to mention that the same extension can/should be introduced to all STM targets.

[ohagendorf]

Is there anything I can do to improve this PR?

[bcostm]

For me it's ok. The only thing is about documentation. I have added a link to the PeripheralPins.c and PinNames.h files in the NUCLEO_F207ZG page after the pinout description (as a test).
The only problem is that this file is linked to a version of mbed-dev. @0xc0170 do you know how we can point to the latest version of this file ?
I found the syntax. Now the file should point to the latest version (default tag).

[ohagendorf]

@bcostm do you thing we could use the same principle I sugested here for all the other nucleo/discovery targets?

Your idea with linking PeripheralPins.c and PinNames.h onto the plattform page is extremly helpfull!

[bcostm]

Yes it's ok for me to do the same on other platforms. But note that we (ST) are not in position to do it right now. So, if other people can help they are welcome ;-)

Concerning the link to the Pins files, it has been done for all platforms :-)

[adustm]

Hello,
I would like to know if we take this opportunity to offer every pin alternate functionalities to the user ?

Cheers
Armelle

[ohagendorf]

The reason for me for this PR was to present an idea for all STM targets.
I used the Nucleo_F207 as an example because its the only target in the its family source tree here and I wasn't sure how many changes are necessary when I started.

[ohagendorf]

It seems that the interest into this topic is not so big.
But currently in issue #3541 a problem is discussed which could be (partly) solved through this idea without a hardware modification, just through software. This kind of problems was my reason to send this PR.

The NUCLEO_F401 defines pin function differently to NUCLEO_F446 and some other NUCLEOs. This allows the usage of the F401 with the mentioned Bluetooth boards X-NUCLEO-IDB04A1 and X-NUCLEO-IDB05A1 out of the box and prevents the easy usage of other NUCLEO boards.
Both shields use pin D3 (PB_3) for the SPI clock. This pin belongs to SPI_1. Together with D11 and D12 (PA_6 and PA_7) all three SPI pins belongs to SPI_1. The configuration in PeripheralPins.c looks like this:

const PinMap PinMap_SPI_MOSI[] = {
    {PA_7,  SPI_1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF5_SPI1)}, // ARDUINO
...
};
 
const PinMap PinMap_SPI_MISO[] = {
    {PA_6,  SPI_1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF5_SPI1)}, // ARDUINO
...
};
 
const PinMap PinMap_SPI_SCLK[] = {
...
    {PB_3,  SPI_1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF5_SPI1)},
//  {PB_3,  SPI_3, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF6_SPI3)},
...
};

The pin definition of other NUCLEO boards is different, e.g. here the one of NUCLEO_F446:

const PinMap PinMap_SPI_MOSI[] = {
    {PA_7,  SPI_1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF5_SPI1)}, // ARDUINO D11
...
};
 
const PinMap PinMap_SPI_MISO[] = {
    {PA_6,  SPI_1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF5_SPI1)}, // ARDUINO D12
...
};
 
const PinMap PinMap_SPI_SCLK[] = {
...
    {PA_5,  SPI_1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF5_SPI1)}, // ARDUINO
//  {PB_3,  SPI_1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF5_SPI1)},
    {PB_3,  SPI_3, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF6_SPI3)},
...
};

With a NUCLEO_F446 PB_3 can't be used because it belongs to SPI_3 and not to SPI_1.

Without the suggestion of this PR, a solder jumper has to be changed at the Bluetooth boards so D13 (PA_5) is used for SPI clock. It is not only the disadvantage of soldering but also D13 and the connected LED1 can be a problem.

With this PR it's just another SPI instance initialisation: PB_3 (for F401) or PB_3_ALT0 (for F446 and other) as SPI clock pin.

[LMESTM]

@ohagendorf I think we agree on the usefulness of your proposal.
Nevertheless I don't think it would help in the scope of #3541 because the board referred to is NUCLEO_F429ZI (not F446) where pin D3 correspond to PE_13 and SPI_SCLK cannot be muxed on this one.

[LMESTM]

@0xc0170 what is the status for this PR on your side. We think the proposal is acceptable and does not impact current API. What is your point of view ?

[LMESTM]

@0xc0170 any update ?
I think ST mostly agree with the proposal here.
It wouldn't break anything for current usage and basic users and would provide more flexibility for more advanced users.

[0xc0170]

@0xc0170 any update ?
I think ST mostly agree with the proposal here.
It wouldn't break anything for current usage and basic users and would provide more flexibility for more advanced users.

Thanks for the bump, LGTM as it is now, going to run CI. As it stalled a bit, sorry about that, shall we rebase this one to be up to date however ther e are no conflicts.. I'll run CI

[0xc0170]

/morph test

[mbed-bot]

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1486

All builds and test passed!

[LMESTM]

As it stalled a bit, sorry about that, shall we rebase this one to be up to date however ther e are no conflicts.. I'll run CI

I don't think that rebase is needed as those file haven't changed recently (except maybe if we wait for the folder structure rework from @adustm )