Mbed-os: Pinmap Pull-Up/Pull-Down Config is Ignored

[mattbrown015]

Description

AFAICT, the pull-up/pull-down config in the STM32 pinmaps has no effect.

For example, from mbed-os\targets\TARGET_STM\TARGET_STM32L4\TARGET_STM32L432xC\TARGET_NUCLEO_L432KC\PeripheralPins.c:

MBED_WEAK const PinMap PinMap_UART_TX[] = {
{PA_2, UART_2, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF7_USART2)}, // Connected to STDIO_UART_TX
// {PA_2, LPUART_1,STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF8_LPUART1)}, // Connected to STDIO_UART_TX
{PA_9, UART_1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF7_USART1)},
{PB_6, UART_1, STM_PIN_DATA(STM_MODE_AF_PP, GPIO_PULLUP, GPIO_AF7_USART1)},
{NC, NC, 0}
};

Specifying GPIO_PULLUP is pointless and achieves nothing.

This is because the generic pinmap_pinout from mbed-os\hal\mbed_pinmap_common.c forces the pull-up/pull-down configuration to PullNone after the STM32 specific pin_function has set it according to the pinmap:

void pinmap_pinout(PinName pin, const PinMap *map)
{
if (pin == NC) {
return;
}

while (map->pin != NC) {
if (map->pin == pin) {
// * Set pin config according to pinmap including pull-up/pull-down config *
pin_function(pin, map->function);

// * Override the mode just set in pin_function! *
pin_mode(pin, PullNone);
return;
}
map++;
}
MBED_ERROR1(MBED_MAKE_ERROR(MBED_MODULE_PLATFORM, MBED_ERROR_CODE_PINMAP_INVALID), "could not pinout", pin);
}

It appears, at least some of, the drivers set the pull-up/pull-down config explicitly and appropriately. For example, from mbed-os\targets\TARGET_STM\serial_api.c:

void serial_init(serial_t *obj, PinName tx, PinName rx)
{
...

// Configure UART pins
pinmap_pinout(tx, PinMap_UART_TX);
pinmap_pinout(rx, PinMap_UART_RX);

if (tx != NC) {
pin_mode(tx, PullUp);
}
if (rx != NC) {
pin_mode(rx, PullUp);
}

...
}

I've come to this because it appears the SPI driver, mbed-os\targets\TARGET_STM\serial_api.c, doesn't set the pull-up/pull-down configuration and, at least on our custom board, PullNone is not appropriate.

void spi_init(spi_t *obj, PinName mosi, PinName miso, PinName sclk, PinName ssel)
{
...

// Configure the SPI pins
pinmap_pinout(mosi, PinMap_SPI_MOSI);
pinmap_pinout(miso, PinMap_SPI_MISO);
pinmap_pinout(sclk, PinMap_SPI_SCLK);
spiobj->pin_miso = miso;
spiobj->pin_mosi = mosi;
spiobj->pin_sclk = sclk;
spiobj->pin_ssel = ssel;
if (ssel != NC) {
pinmap_pinout(ssel, PinMap_SPI_SSEL);
handle->Init.NSS = SPI_NSS_HARD_OUTPUT;
} else {
handle->Init.NSS = SPI_NSS_SOFT;
}

...
}

Hacking the code to ensure MISO is configured with a pull-up reduces the STM32 current consumption considerably because, on our board, there is no external pull-up.

I think we've got a number of problems tangled together here!

It seems somewhat belligerent that the generic pinmap_pinout sets PullNone on all pins on all targets. A compromise might be to set PullNone first and allow specific implementations of pin_function to then override it if they want. Given that this is a generic, heavily used function that hasn't changed for years, I can imagine that changing it will be difficult!

The STM32 specific pinmap configuration is misleading and pointless. The pull-up/pull-down config should be either:

• Removed
• Made to work as implied
• Heavily annotated as documentation only

The SPI driver needs to either set the pull-up/pull-down configuration appropriately or provide some configuration to do it.

Of course any app using the SPI driver can set the pull-up/pull-down configuration directly after initialising the driver but this would not be obvious and isn't necessary for other drivers.

In our particular situation we not using the SPI driver directly. We're actually using mbed-semtech-lora-rf-drivers and therefore the SPI is a bit remote from our app code.

Issue request type

[ ] Question
[ ] Enhancement
[x] Bug

[LMESTM]

@mattbrown015 - clear thanks fr the report.
do you confirm that commenting out the below line solves the issue ?

            // *** Override the mode just set in pin_function! ***
            pin_mode(pin, PullNone);

I'd like to get arm views on

It seems somewhat belligerent that the generic pinmap_pinout sets PullNone on all pins on all targets. A compromise might be to set PullNone first and allow specific implementations of pin_function to then override it if they want. Given that this is a generic, heavily used function that hasn't changed for years, I can imagine that changing it will be difficult!

@bulislaw @0xc0170

[mattbrown015]

do you confirm that commenting out the below line solves the issue ?

Commenting out this line (and changing the pin map) solves my issue and, I believe, is the correct thing to do for STM32.

What should happen on other targets with different styles of pin map is less clear to me.

Perhaps STM32 is the only target that includes the pull-up/pull-down config in the pin map which would mean this is probably fine for all other targets. Let's see what @bulislaw and @0xc0170 have to say! :-)

I've solved my specific and immediate problem by explicitly setting the SPI MISO pull-up in the application code after the SPI object initialisation has completed.

[DataDrake]

I agree that this is a problem as I just finished debugging an issue with an SPI CS pin that needed the internal Pull Up for CS to register as 1 and set our SPI Flash to its low-power idle mode.

[kjbracey-arm]

Pull ups and pull downs are board-specific, so I don't feel like they should be something for the application or C++ driver classes to deal with.

BUT - we already expect applications to do board-specific things by specifying the pin number to use for a function in the first place.

HOWEVER - the need for a pull or not on a given pin is independent of what software is actually doing, Even when a piece of application software is not using the function on a pin, the default state of the pin after power-up must have the pull correct (either by being in GPIO mode with appropriate pull configured, or autoconfigured to a function with appropriate inherent pull).

So I submit that board initialisation code should be getting the pulls right for the board, and then any function selection should not interfere with that, unless the application code makes an explicit pull specifying request.

So I would concur with removing that automatic "pull:=none" in principle.

You could move it to above the pin_function call - that would seem like a plausible backwards-compatibility to minimise risk of change for existing targets, but that forces pull selection into the HAL target code, and would preclude a board-aware application making a GPIO pull request before activating a function on the same pin.

[DataDrake]

Setting it before pin_function seems to make the most sense. Targets that don't change the pull selection will be unaffected, but others will at least have an opportunity to override the default setting.

[LMESTM]

@ARMmbed/mbed-os-core
could you take care of reviewing and possibly pushing the change proposed by @kjbracey-arm above ?

[bcostm]

Why adding the "devices: st" label ? The proposed change has to be made in the core files and not in ST ones...

It would be nice also to remove "STM32" in the title...

[cmonr]

@bcostm Updated

[ciarmcom]

Thank you for raising this detailed GitHub issue. I am now notifying our internal issue triagers.
Internal Jira reference: https://jira.arm.com/browse/IOTOSM-2226