Dev stm32 gpio pins rework #3665

LMESTM · 2017-01-31T13:50:07Z

Description

This PR is an update of the pins, gpio and gpio irqs management.

Until now all those 3 services were using the same STM32 high level service from HAL which was a bit cumbersome. After this change the design is simpler and each drivers gpio_api.c, gpio_irq_api.c and pinmap.c will manage the corresponding registers in an independant way.

After this PR, we have following benefit:

simpler design pattern as explained above
common code for those drivers instead of 1 driver per STM family. Only the family specific part s managed in a separate pin_device.h file. Mostly F1 family has a specific implementation.
increased performances (addressing STM targets are extremely inefficient at toggling GPIOs between input and output #3299)
will BTW solve timeout issues when running PWM tests with ci-test-shield (as in here NUCLEO_F401RE - SPI, PWM and InterruptIn failues (CI test shield) #3501)
this also updates the pin_mode parameters so that application can configure IOs in OpenDrain mode.

Status

READY

Related PRs

Need #3629 (also part of this branch for now - would disappear after rebase when #3629 gets merged)

Tests results

Non regression on with ci-tests-shields are ok
ci-test-shield.zip

MBED non-regression ongoing for all platforms.

F1 results:
F1 pins non-reg

bcostm · 2017-01-31T15:08:36Z

targets/TARGET_STM/TARGET_STM32F0/pin_device.h

+ case GPIO_PULLDOWN:
+ LL_GPIO_SetPinPull(gpio, ll_pin, LL_GPIO_PULL_DOWN);
+ break;
+ case GPIO_NOPULL:


Maybe use "default:" instead ?

bcostm · 2017-01-31T15:11:09Z

targets/TARGET_STM/TARGET_STM32F1/pin_device.h

+ LL_GPIO_SetPinMode(gpio, ll_pin, LL_GPIO_MODE_INPUT);
+ LL_GPIO_SetPinPull(gpio, ll_pin, LL_GPIO_PULL_DOWN);
+ break;
+ case GPIO_NOPULL:


Use default: ?

bcostm · 2017-01-31T15:13:10Z

targets/TARGET_STM/TARGET_STM32F2/pin_device.h

+ case GPIO_PULLDOWN:
+ LL_GPIO_SetPinPull(gpio, ll_pin, LL_GPIO_PULL_DOWN);
+ break;
+ case GPIO_NOPULL:


Use default: ?

bcostm · 2017-01-31T15:13:40Z

targets/TARGET_STM/TARGET_STM32F3/pin_device.h

+ case GPIO_PULLDOWN:
+ LL_GPIO_SetPinPull(gpio, ll_pin, LL_GPIO_PULL_DOWN);
+ break;
+ case GPIO_NOPULL:


Use default: ?

bcostm · 2017-01-31T15:14:19Z

targets/TARGET_STM/TARGET_STM32F4/pin_device.h

+ case GPIO_PULLDOWN:
+ LL_GPIO_SetPinPull(gpio, ll_pin, LL_GPIO_PULL_DOWN);
+ break;
+ case GPIO_NOPULL:


Use default: ?

bcostm · 2017-01-31T15:14:51Z

targets/TARGET_STM/TARGET_STM32F7/pin_device.h

+ case GPIO_PULLDOWN:
+ LL_GPIO_SetPinPull(gpio, ll_pin, LL_GPIO_PULL_DOWN);
+ break;
+ case GPIO_NOPULL:


Use default: ?

@bcostm Thanks Bruno - I updated the code as suggested

sg- · 2017-02-02T17:54:04Z

/morph test-nightly

mbed-bot · 2017-02-02T23:44:31Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1496

All builds and test passed!

0xc0170 · 2017-02-03T11:37:59Z

@LMESTM Can you please rebase ? L0 PR was merged, so we can have a look at this one and test

LMESTM · 2017-02-03T12:49:36Z

@0xc0170 rebased now.

0xc0170 · 2017-02-06T09:46:33Z

increase performance

any numbers, how does it change? What about footprint? I had a look at the structure for gpio IRQ , looks to me that it shall be the same at least for those pins RAM data.

Ill rerun tests:

/morph test-nightly

0xc0170 · 2017-02-06T09:48:54Z

targets/TARGET_STM/gpio_api.c

+inline void gpio_dir(gpio_t *obj, PinDirection direction) {
+
+ if (direction == PIN_INPUT)
+ {


nit: block alignment as it was

corrected with new commits

0xc0170 · 2017-02-06T09:51:49Z

targets/TARGET_STM/TARGET_STM32F1/gpio_irq_device.h

+} exti_lines_t;
+
+// Used to return the index for channels array.
+static exti_lines_t pin_lines_desc[16] =


Why is array defined in the header file ?

Because this is the family specific part - each family has a specific exti lines mapping.

note: I just realized this can probably be declared as const

should be definitely in flash.

I was asking mainly as this should be then included just once otherwise would be multiple defined (with multiple inclusions), variables should not be defined in header files is a good advice. The question would be why this is not defined in a code file, and included with some common header file where declaration is provided.

I understand.
As it is now in this PR, gpio_irq_device.h is an extension with static inline functions and table of gpio_irq_api.c and only included from this file. I could indeed replace this file with 2 files: gpio_irq_device.h with declarations and gpio_irq_device.c file for instance

@0xc0170 so do you want me to change the .h file into 2 separate files ?

If you agree, then would be better (to overcome surprises).

sure - will do

0xc0170 · 2017-02-06T09:54:03Z

targets/TARGET_STM/gpio_irq_api.c

- obj->event = EDGE_FALL;
- }
+ else
+ {


Code style consistency

corrected with new commits

LMESTM · 2017-02-06T13:32:50Z

increase performance
any numbers, how does it change? What about footprint? I had a look at the structure for gpio IRQ , looks to me that it shall be the same at least for those pins RAM data.

Here is the improvement as seen from FastIO bench:
Target - Fixed write - Variable write - Read - Operator toggling - Input/output mode
Before PR
NUCLEO F4X1RE (84MHz) 24ns (18%) 83ns (47%) 60ns (42%) 60ns (16%) 60ns (1%)
After PR
NUCLEO F4X1RE (84MHz) 24ns (26%) 83ns (59%) 60ns (100%) 60ns (27%) 60ns (5%)

Before PR
NUCLEO F030R8 (48MHz) 68ns (19%) 115ns (23%) 125ns (33%) 188ns (20%) 130ns (1%)
After PR
NUCLEO F030R8 (48MHz) 68ns (42%) 115ns (43%) 125ns (100%) 188ns (35%) 130ns (5%)

mbed-bot · 2017-02-06T14:54:09Z

Result: FAILURE

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

/morph test-nightly

Output

mbed Build Number: 1508

Test failed!

bridadan · 2017-02-06T15:24:08Z

The failure was caused by Windows 10 installing an update in the middle of a test. Even though I disabled it Microsoft still pushes them through sometimes it looks like 🙄

/morph test-nightly

mbed-bot · 2017-02-06T16:35:04Z

Result: ABORTED

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

/morph test-nightly

Output

mbed Build Number: 1510

Test Prep failed!

bridadan · 2017-02-06T16:35:25Z

@LMESTM I saw you pushed another commit. Please let me know when you've finished your changes and we'll restart CI.

LMESTM · 2017-02-06T16:43:57Z

@bridadan - I took into account latest comments from @0xc0170 (hope this is ok now). and I actually made a small mistake during commit and had to push changes twice - this is complete now on my side unless new comments :-)

LMESTM · 2017-02-16T17:52:15Z

@bridadan thx - I'll keep you posted

This requires the creation of gpio_irq_device.h file, where family specific EXTI IRQ mapping is defined

Instead of using HAL_GPIO_Init / Deinit which makes a lot of registers being written and re-written, and which creates extra gpio / pin / irq dependencies, we directly set the IRQ related registers thanks for the STM32 LL layers which provides APIs to modify registers.

Directly return a GPIO_TypeDef pointer to avoid extra casts. Also move it to GPIO file.

Those are minor changes to increase performance of widely used GPIO functions.

this first makes pinmap.c a common file then rework it with several goals: - avoid gpio / irq / pin management extra dependencies - improve performances when switching between pin modes This change is based on LL layer to access to registers level instead of using HAL higher level API. The family specific functions are implemented in pin_device.h of each family. Mostly this is F1 family that is differnt from other ones.

The default pin mode shall be set as part of the pinamp_pinout, and as defined in tables of PeripheralPins.c, but this is currently over-written by a call to pin_mode(pin, PullNone); from mbed_pinmap_common.c, so we need a set the mode again here, including OpenDrain config as needed for I2C.

As commented during PR review, better use default case.

Move the const table initialization from the header file to a new C file to avoid any multiple defined errors.

Correcting the style format errors. Also updating the copyright year.

LMESTM · 2017-02-16T18:43:47Z

@bridadan @sg- @0xc0170
I messed up at one stage when solving one comment or rebasing (note for later, do not use --force :-( )
... but this is fixed now !

bridadan · 2017-02-16T21:53:56Z

/morph test-nightly

mbed-bot · 2017-02-17T00:32:02Z

Result: SUCCESS

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

/morph test-nightly

Output

mbed Build Number: 1620

All builds and test passed!

LMESTM force-pushed the dev_stm32_gpio_pins_rework branch 2 times, most recently from def4528 to 514b45f Compare January 31, 2017 14:04

bcostm reviewed Jan 31, 2017

View reviewed changes

0xc0170 added the needs: review label Feb 1, 2017

sg- added needs: CI and removed needs: review labels Feb 2, 2017

0xc0170 added needs: status update and removed needs: CI labels Feb 3, 2017

LMESTM force-pushed the dev_stm32_gpio_pins_rework branch from 5af842c to ea15b87 Compare February 3, 2017 12:45

0xc0170 removed the needs: status update label Feb 6, 2017

0xc0170 added the needs: CI label Feb 6, 2017

0xc0170 requested changes Feb 6, 2017

View reviewed changes

0xc0170 added the needs: work label Feb 6, 2017

LMESTM force-pushed the dev_stm32_gpio_pins_rework branch from 6a524cf to 927a113 Compare February 6, 2017 15:45

bridadan added do not merge and removed ready for merge labels Feb 16, 2017

LMESTM added 12 commits February 16, 2017 18:55

STM32 : Make port_api.c a common file

0e2cc38

STM32 Make gpio_irq_api.c a common files

74774f9

This requires the creation of gpio_irq_device.h file, where family specific EXTI IRQ mapping is defined

STM32: Change Set_GPIO_Clock return type

3517eb1

Directly return a GPIO_TypeDef pointer to avoid extra casts. Also move it to GPIO file.

STM32: gpio few performance improvements

dce2ca7

Those are minor changes to increase performance of widely used GPIO functions.

stm32: use default in switch case

273ac81

As commented during PR review, better use default case.

STM32: pins: move pin_lines_desc table to const

73955b6

STM32: gpio: style consistency

18abfdb

STM32: move pin_lines_desc to c file

32ecd00

Move the const table initialization from the header file to a new C file to avoid any multiple defined errors.

Style consistency

d7902e5

Correcting the style format errors. Also updating the copyright year.

LMESTM force-pushed the dev_stm32_gpio_pins_rework branch from ce3ecb4 to d7902e5 Compare February 16, 2017 18:39

bridadan added needs: CI and removed do not merge labels Feb 16, 2017

sg- added ready for merge release-version: 5.4.0 and removed needs: CI labels Feb 17, 2017

sg- merged commit b7dce71 into ARMmbed:master Feb 21, 2017

sg- removed the ready for merge label Feb 21, 2017

pmancele mentioned this pull request Feb 22, 2017

STM32L4 : Fix GPIO G port compatibility #3780

Merged

LMESTM mentioned this pull request Feb 27, 2017

STM32: gpio SPEED - always set High Speed by default #3840

Merged

bcostm mentioned this pull request Feb 27, 2017

mbed OS 5.3.5 - Blinky does not work on xdot_l151cc target #3823

Closed

Sissors mentioned this pull request May 9, 2017

STM targets are extremely inefficient at toggling GPIOs between input and output #3299

Closed

Dev stm32 gpio pins rework #3665

Dev stm32 gpio pins rework #3665

Conversation

LMESTM commented Jan 31, 2017 • edited Loading

Description

Status

Related PRs

Tests results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sg- commented Feb 2, 2017

mbed-bot commented Feb 2, 2017

Output

0xc0170 commented Feb 3, 2017

LMESTM commented Feb 3, 2017

0xc0170 commented Feb 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LMESTM commented Feb 6, 2017

mbed-bot commented Feb 6, 2017

Output

bridadan commented Feb 6, 2017

mbed-bot commented Feb 6, 2017

Output

bridadan commented Feb 6, 2017

LMESTM commented Feb 6, 2017

LMESTM commented Feb 16, 2017

LMESTM commented Feb 16, 2017

bridadan commented Feb 16, 2017

mbed-bot commented Feb 17, 2017

Output

LMESTM commented Jan 31, 2017 •

edited

Loading