Skip to content

EFM32: fix weak PeripheralPins configuration #7491

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

Merged

Conversation

DBS06
Copy link
Contributor

@DBS06 DBS06 commented Jul 12, 2018

Description

PR for Issue #7424 .
Making the configuration tables weakly defined to be overridable.
This would be needed for i.e. custom targets which uses different Pin-Profiles as default defined.

Pull request type

[ ] Fix
[X] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

@DBS06 DBS06 mentioned this pull request Jul 12, 2018
@cmonr cmonr requested a review from stevew817 July 12, 2018 15:36
Copy link
Contributor

@cmonr cmonr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cmonr
Copy link
Contributor

cmonr commented Jul 12, 2018

Will wait for @stevew817 to ack before starting CI.

Copy link
Contributor

@stevew817 stevew817 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DBS06 How soon do you want this on master? Can we wait with this until #7079 is merged and update the GG11 pinmaps through an update to this PR?

@@ -25,7 +25,7 @@

/************ADC***************/
/* The third "function" value is used to select the correct ADC channel */
const PinMap PinMap_ADC[] = {
const PinMap __attribute__((weak)) PinMap_ADC[] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use MBED_WEAK

@stevew817
Copy link
Contributor

#7079 was merged, so please rebase and apply the same change for EFM32GG11.

@DBS06
Copy link
Contributor Author

DBS06 commented Jul 16, 2018

@stevew817 I can wait, right now it is not that urgent.

#7079 was merged, so please rebase and apply the same change for EFM32GG11.

I will do it asap

@DBS06
Copy link
Contributor Author

DBS06 commented Jul 16, 2018

@stevew817 I did the requested changes 😄

edit: I did the rebase to master but why does git thinks the "rebase commits" are new commits? Only the commit d1c313d8aff2aa4d89745380c3fb48010dedf0db is new...

Copy link
Contributor

@stevew817 stevew817 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I approve of the changes to the Silicon Labs part. However, to satisfy the mbed-os maintainers, you should take a look at the git rebase command instead of doing a merge. Rebasing your specific changes on top of master avoids all these superfluous commits in the PR log.

Copy link
Contributor

@stevew817 stevew817 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I approve of the changes to the Silicon Labs part. However, to satisfy the mbed-os maintainers, you should take a look at the git rebase command instead of doing a merge. Rebasing your specific changes on top of master avoids all these superfluous commits in the PR log.

@stevew817
Copy link
Contributor

@DBS06 If you can't get rebase to work, I'd suggest checking out a new feature branch based off of master, cherry-picking your commits to go on top, and force-pushing the 'clean' branch to your PR branch on github.

@DBS06 DBS06 force-pushed the feature/EFM32_make_PeripheralPins_overridable branch from d1c313d to 95d78df Compare July 16, 2018 10:30
@DBS06
Copy link
Contributor Author

DBS06 commented Jul 16, 2018

@stevew817 Cleaned my branch while my lunch getting cold 😄

@stevew817
Copy link
Contributor

Great! I wholeheartedly approve 👍

@cmonr
Copy link
Contributor

cmonr commented Jul 16, 2018

@DBS06 I wouldn't want your dinner to get cold as well, but @0xc0170 comment still needs to be adressed ;)

@DBS06
Copy link
Contributor Author

DBS06 commented Jul 16, 2018

@DBS06 I wouldn't want your dinner to get cold as well, but @0xc0170 comment still needs to be adressed ;)

Ah my fault didn't realized another change request...

@DBS06
Copy link
Contributor Author

DBS06 commented Jul 16, 2018

@cmonr Done!

@cmonr cmonr dismissed 0xc0170’s stale review July 16, 2018 13:57

Comment addressed.

@cmonr
Copy link
Contributor

cmonr commented Jul 16, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jul 16, 2018

Build : FAILURE

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

@stevew817
Copy link
Contributor

stevew817 commented Jul 16, 2018

Looks like MBED_WEAK needs to come before the type in a variable declaration, or mbed_toolchain.h is not part of the include chain.

@DBS06
Copy link
Contributor Author

DBS06 commented Jul 16, 2018

Looks like MBED_WEAK needs to come before the type in a variable declaration, or mbed_toolchain.h is not part of the include chain.

@stevew817 #include "mbed_toolchain.h" was missing.
I added #include "mbed_toolchain.h" like they did it for the STM targets i.e. ./mbed-os\targets\TARGET_STM\TARGET_STM32L4\TARGET_STM32L476xG\TARGET_NUCLEO_L476RG\PeripheralPins.c

@cmonr
Copy link
Contributor

cmonr commented Jul 16, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jul 16, 2018

Build : FAILURE

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

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 17, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Jul 17, 2018

Build : SUCCESS

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

Triggering tests

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

@0xc0170 0xc0170 changed the title EFM32 Make PeripheralPins.c configuration tables weakly defined to be… EFM32: fix weak PeripheralPins configuration Jul 17, 2018
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 17, 2018

/morph mbed2-build

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 17, 2018

/morph export-build

@mbed-ci
Copy link

mbed-ci commented Jul 17, 2018

@mbed-ci
Copy link

mbed-ci commented Jul 17, 2018

@cmonr
Copy link
Contributor

cmonr commented Jul 18, 2018

/morph export-build

2 similar comments
@0xc0170
Copy link
Contributor

0xc0170 commented Jul 18, 2018

/morph export-build

@cmonr
Copy link
Contributor

cmonr commented Jul 19, 2018

/morph export-build

@cmonr
Copy link
Contributor

cmonr commented Jul 20, 2018

/morph export-build

@0xc0170 This might need some special attention tomorrow...

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 20, 2018

/morph export-build

@mbed-ci
Copy link

mbed-ci commented Jul 20, 2018

Exporter Build : SUCCESS

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

@cmonr cmonr merged commit bb7b97c into ARMmbed:master Jul 20, 2018
@0xc0170 0xc0170 removed the needs: CI label Jul 20, 2018
pan- pushed a commit to pan-/mbed that referenced this pull request Aug 22, 2018
…pheralPins_overridable

EFM32: fix weak PeripheralPins configuration
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants