New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

STM32 : set all PinMap structures as weak #5936

Merged
merged 1 commit into from Jan 30, 2018

Conversation

Projects
None yet
7 participants
@jeromecoutant
Contributor

jeromecoutant commented Jan 25, 2018

Description

This allow custom overwrites.

Status

READY

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 25, 2018

We should use MBED_WEAK that toolchain header file provides. But first a question: What is the use case for having them over-writable ? They define all MCU pins, is there anything missing?

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jan 25, 2018

OK to use MBED_WEAK.

What is the use case for having them over-writable ?

I can see 2 cases:

  • this was introduced by @helmut64 in #5591
    Goal was to introduce an "official" board, but to use a custom board.

  • the second point could be for issues like #5930
    In the pinmap structures, we have commented several pin lines for several reasons. But some customers could have some different reason. So he can now easily redefine the structure.

@jeromecoutant jeromecoutant force-pushed the jeromecoutant:PR_WEAK_PINMAP branch from 13276d1 to 5714340 Jan 25, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 25, 2018

For custom boards that might have more pins available, or some differencies. This might be a valid reason to extend API or provide functionality that instead of weak linking, set pinmap arrays, default ones provided? We shall think about it

@SenRamakri @bulislaw @c1728p9 Can you review this addition ?

@0xc0170 0xc0170 requested review from bulislaw and c1728p9 Jan 25, 2018

@helmut64

This comment has been minimized.

Contributor

helmut64 commented Jan 25, 2018

I believe declaring this as weak (as I started on the L433) series is a fair tradeoff. This is adding more flexibility for custom target boards. In general this allows to use more pins as GPIOs, however it is not perfect because special pin functionality e.g.: UART, SPI, Timer, interrupt vectors, etc are not enabled because the target device does not know about it. E.g.: See serial_device.c in the STM target folder.

My opinion is, that the weak solution is great for developers, they get access to more pins and can optimise their hardware pin design to use mbed for their target boards. Most engineers and students are can easily handle the mbed online system, however adding a new target is out of reach for most developers.

Making it perfect (with all features) is a super large task, and some MCU vendors (Atmel) don’t keep up their mbed support anyways. I recommend this little change with a great benefit.

@bcostm

bcostm approved these changes Jan 26, 2018

@jeromecoutant jeromecoutant force-pushed the jeromecoutant:PR_WEAK_PINMAP branch from 5714340 to 09065c4 Jan 26, 2018

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jan 26, 2018

Compilation issue should be solved
Thx

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jan 26, 2018

@jeromecoutant Could you rebase this PR so that we can start CI? Thanks!

STM32 : set all PinMap structures as weak
This allow custom overwrites

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jan 29, 2018

@jeromecoutant jeromecoutant force-pushed the jeromecoutant:PR_WEAK_PINMAP branch from 09065c4 to 8f647be Jan 29, 2018

@jeromecoutant

This comment has been minimized.

Contributor

jeromecoutant commented Jan 29, 2018

Could you rebase this PR so that we can start CI?

Done

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 29, 2018

/morph build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 29, 2018

@helmut64 Thanks for details 👍

LGTM. Be aware that for all these platforms, if mbed 2 lib is used, it wont be functional until we make these peripheral pins definitions into own object file.

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 29, 2018

Build : SUCCESS

Build number : 988
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5936/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci

This comment has been minimized.

@cmonr cmonr added needs: CI and removed needs: work labels Jan 29, 2018

@mbed-ci

This comment has been minimized.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Jan 30, 2018

@cmonr cmonr merged commit fff6c75 into ARMmbed:master Jan 30, 2018

19 checks passed

ARM mbed CI Verification build successful.
Details
AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Local events testing has passed
Details
travis-ci/littlefs Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-RENESAS Local mbed2-RENESAS testing has passed
Details
travis-ci/mbed2-SILICON_LABS Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM Local mbed2-STM testing has passed
Details
travis-ci/tools Local tools testing has passed
Details

@jeromecoutant jeromecoutant deleted the jeromecoutant:PR_WEAK_PINMAP branch Jan 31, 2018

@cmonr cmonr removed the ready for merge label Jan 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment