Exclude target-specific pins from GPIO tests with the FPGA shield

[fkjagodzinski]

Description

Most of the HAL tests that use the FPGA shield only use pins present in a PinMap specific for given peripheral. There is no PinMap for GPIOs, but some board-specific pins have to be excluded from testing due to hardware limitations. This patch adds a pinmap_restricted_pins_gpio() function that every target can override if needed.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[x] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@c1728p9 @jamesbeyond @mprse @maciejbocianski

Release Notes

Add a weak pinmap_restricted_pins_gpio() that every target can override and provide a list of pins that should be skipped during the GPIO tests (in addition to pinmap_restricted_pins()).

[c1728p9]

Its likely that more exclusion lists will be needed for various hardware quirks. What about something like pinmap_pin_info(PinName pin, pin_info_t *info) which returns a structure of info for the given pin? This could take the place of both pinmap_restricted_pins() and pinmap_restricted_pins_gpio() by having these as fields in the struct. As new types of hardware anomalies are found they could be added to the struct. The struct could look something like this (As an example):

typedef struct {
    bool restricted;    // Pin should be avoided for all tests
    PullType pull;      // Pin has a hardware pullup or pulldown or is not pulled
    bool i2c_shared;    // One or more I2C devices may be connected to this pin
    bool spi_shared;    // One or more SPI devices may be connected to this pin
    bool needs_jumper;  // This pin is unconnected with default board jumper setting
} pin_info_t;

@maclobdell what are your thoughts on something like this?

[fkjagodzinski]

Sounds like a good idea. A weak pinmap_pin_info() would do the trick. This would require updating https://github.com/ARMmbed/fpga-ci-test-shield/blob/master/mbed-code/test_utils.h, @maciejbocianski what do you think?

[maciejbocianski]

@c1728p9 what will be use case of i2c_shared?
Actually we have only problems with gpio/gpio_irq tests. Limitation of others (i2c/spi/qspi/...) are or should be covered in PinMap_XXX. I think that for now having pinmap_restricted_pins_gpio list with pulled pins is enough

[fkjagodzinski]

I agree with @maciejbocianski -- I don't see any need for more exclusion lists for now.

[ciarmcom]

@fkjagodzinski, thank you for your changes.
@mprse @maclobdell @maciejbocianski @MarceloSalazar @jamesbeyond @c1728p9 @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-maintainers please review.

[maclobdell]

Instead of having a black list of restricted pins, could we have a white list of pins that are suitable for gpio use? we could have partners provide this list for their boards based on criteria we set (pin doesn't have external components on it, etc). Also a white list could show customers which pins are available in a board for use. And it also would give a list of pins to test. That seems less complex than having to test all pins, see which fail, analyze why they fail, and then add them to the black list. Just some friendly feedback.

[0xc0170]

Instead of having a black list of restricted pins, could we have a white list of pins that are suitable for gpio use? we could have partners provide this list for their boards based on criteria we set (pin doesn't have external components on it, etc). Also a white list could show customers which pins are available in a board for use. And it also would give a list of pins to test. That seems less complex than having to test all pins, see which fail, analyze why they fail, and then add them to the black list. Just some friendly feedback.

@fkjagodzinski Can you review?

Actually we have only problems with gpio/gpio_irq tests. Limitation of others (i2c/spi/qspi/...) are or should be covered in PinMap_XXX. I think that for now having pinmap_restricted_pins_gpio list with pulled pins is enough

Maybe too big hammer but why this value "PinMapMode" is not part of PinMap ? Do we know additional function for just gpio? If we know this pin up has pull up on board, it is pin's attribute we can't change and should be part of the pin. If we test it, we would check this one and find out "pull up there, dont test any other pin mode".

[fkjagodzinski]

Instead of having a black list of restricted pins, could we have a white list of pins that are suitable for gpio use? (...)

@maclobdell Thanks for your input! Actually all of the pins present in a given form factor (e.g. returned by pinmap_ff_default_pins()) are expected to handle GPIO, so the PinList* returned by GPIO white list would likely duplicate that for most of the targets.

[fkjagodzinski]

Maybe too big hammer but why this value "PinMapMode" is not part of PinMap ? Do we know additional function for just gpio? If we know this pin up has pull up on board, it is pin's attribute we can't change and should be part of the pin. If we test it, we would check this one and find out "pull up there, dont test any other pin mode".

@0xc0170 Just to make it clear -- you suggest adding a PinMode member to

mbed-os/hal/pinmap.h

Lines 30 to 34 in 42a9a7a

	typedef struct {
	PinName pin;
	int peripheral;
	int function;
	} PinMap;

This sounds like a good idea, but unfortunately we do not have any PinMap for GPIO currently anyway.

Initially, I wanted to add a GPIO PinMap to have a list of pins testable with the FPGA shield, but eventually, I decided to propose this patch since it's a lot simpler.

[adbridge]

@0xc0170 are you happy with the responses to your questions ? Can we move this forward?

[0xc0170]

Yes it is, this can be as improvement then 👍 However, if this is not must be in 5.13, I would consider it. As it is here it is easier to patch but is this future proof ? Does this align what we have already? If answer is yes, then lets go ahead with this patch.

[0xc0170]

CI started meanwhile

[mbed-ci]

Test run: FAILED

Summary: 2 of 7 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

• jenkins-ci/mbed-os-ci_build-IAR
• jenkins-ci/mbed-os-ci_mbed2-build-IAR

[fkjagodzinski]

Ok, to speed things up a bit, I prepared an alternative patch that uses the whitelist/pinmap approach instead of providing a function to exclude pins.
@0xc0170 @maclobdell @c1728p9 Could you take a look at #10644?

[0xc0170]

Closing this in favor of 10644