Skip to content
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

Pinmap extensions #9449

Merged
merged 14 commits into from Feb 11, 2019

Conversation

@c1728p9
Copy link
Contributor

commented Jan 22, 2019

Description

Add pinmap getters and utility functions, along with form factor information in preparation for enhanced testing.

Pull request type

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

Release Notes

Added pinmap support to the HAL API. For more information see the porting targets docs page - https://os.mbed.com/docs/mbed-os/latest/porting/porting-targets.html.

@c1728p9

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

This PR depends on #9438

@c1728p9 c1728p9 force-pushed the c1728p9:pinmap-extension branch Jan 22, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

CI started to test this

targets/targets.json Outdated
},
"default-form-factor": {
"help": "Default form factor of this board taken from supported_form_factors. This must be a lowercase string",
"value": "arduino"

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Jan 22, 2019

Contributor

Hi
Default value should be "null"...?
And specified at target level?
Which is already done:
"supported_form_factors": ["ARDUINO"]

Note also, that this ARDUINO means Arduino Uno, some STM32 targets are Arduino Nano compatible.

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Jan 22, 2019

Author Contributor

Default value should be "null"...?
And specified at target level?

Yeah, I'll make those changes.

Which is already done:
"supported_form_factors": ["ARDUINO"]

The goal with default-form-factor is to allow a default to be specified when multiple form factors are present, such as ["ARDUINO", "MORPHO"] with the NUCLEO_F401RE.

Note also, that this ARDUINO means Arduino Uno, some STM32 targets are Arduino Nano compatible.

I didn't realize this. What are your thoughts @jeromecoutant on having a separate ARDUINO_NANO for those targets? That way supported_form_factors represents the form factors that can be physically connected to a board.

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 22, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
@bulislaw

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

How we'll test the extensions to HAL APIs?

@c1728p9

This comment has been minimized.

Copy link
Contributor Author

commented Jan 22, 2019

@bulislaw these extensions will be used to test hardware peripherals. Any bugs in the pinmaps will show up as a peripheral test failure. Does that answer your question?

@c1728p9 c1728p9 force-pushed the c1728p9:pinmap-extension branch Jan 22, 2019

Show resolved Hide resolved hal/analogin_api.h Outdated
Show resolved Hide resolved hal/analogin_api.h Outdated
@@ -34,6 +34,10 @@
"mpu-rom-end": {
"help": "Last address of ROM protected by the MPU",
"value": "0x0fffffff"
},
"default-form-factor": {

This comment has been minimized.

Copy link
@bulislaw

bulislaw Jan 23, 2019

Member

Hm you are not setting default value here, but in the code sort of circumventing the config system and hiding the fact. Wouldn't it be cleaner to set it here?

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Jan 23, 2019

Author Contributor

Setting the default here to arduino would also be misleading since some boards are not in the arduino form factor. Because of this @jeromecoutant suggested keeping this null and specifying it at the target level, which I think is a good idea.

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Jan 25, 2019

Contributor

Adding this new config is OK.
But who is going to replace the supported_form_factors line by this new config?
Why you are mixing both configurations ?

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Feb 4, 2019

Contributor

Hi
No answer for this question?
A new config is created, but never used...!

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Feb 11, 2019

Author Contributor

Hi @jeromecoutant sorry for the late response, I just noticed your question. The default-form-factor is intended to be used with supported_form_factors, not to replace it. Some boards may have multiple form factors that are testable and this value is intended to let you pick the one to test with. This is currently not needed by any targets so it is not used. I added it here for completeness. If you are concerned with it I can remove it an submit it as a PR only when it is needed. Let me know if you have a preference on this.

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Feb 12, 2019

Author Contributor

@jeromecoutant even though this PR has been merged please provide any feedback you may have. I'll address any changes needed in a separate PR.

@c1728p9 c1728p9 force-pushed the c1728p9:pinmap-extension branch 2 times, most recently Jan 23, 2019

@c1728p9

This comment has been minimized.

Copy link
Contributor Author

commented Jan 23, 2019

I created a docs PR to go along with this here:
ARMmbed/mbed-os-5-docs#930

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

Testing with CI

@mbed-ci

This comment has been minimized.

Copy link

commented Jan 24, 2019

Test run: FAILED

Summary: 6 of 8 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_mbed2-build-IAR
  • jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
  • jenkins-ci/mbed-os-ci_mbed2-build-ARM
  • jenkins-ci/mbed-os-ci_build-ARM
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-IAR
#include "PeripheralPins.h"

//*** Common form factors ***
#ifdef TARGET_FF_ARDUINO

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Jan 24, 2019

Contributor

Should be somthing like MBED_CONF_TARGET_DEFAULT_FORM_FACTOR==ARDUINO ?

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Jan 24, 2019

Author Contributor

I was thinking that all the form factors a board supports can be present at the same time. So for example ST boards can have both TARGET_FF_MORPHO and TARGET_FF_ARDUINO. Setting the value of MBED_CONF_TARGET_DEFAULT_FORM_FACTOR would just pick one of these to be used during testing. What do you think @jeromecoutant

This comment has been minimized.

Copy link
@jeromecoutant

jeromecoutant Jan 24, 2019

Contributor

To be honest, for me, TARGET_FF_MORPHO and TARGET_FF_ARDUINO are not used at all...

So be free to make the changes you need :-)

@c1728p9 c1728p9 force-pushed the c1728p9:pinmap-extension branch Jan 24, 2019

@c1728p9

This comment has been minimized.

Copy link
Contributor Author

commented Jan 24, 2019

Based on the test results, a lot of targets are failing because pinmaps are not publicly exposed. For example the LPC1768 has its serial pinamp static to serial_api.c. To address this I'm removed the weak definitions of the pinmap getter functoins from mbed_pinmap_default.c and have instead added them to each target. This simplifies things since the table name doesn't matter and there is no weak pinmap getter function to worry about.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Jan 24, 2019

Testing with CI

c1728p9 added some commits Feb 1, 2019

Add pinmaps to Ameba
Add testing pinmaps to the Ameba along with fixing build errors.
Add testing pinmaps to LPC15XX and LPC8XX devices
Add pinmap tables to the LPC15XX and LPC8XX families to allow testing.
Add a sanity check test for pinmaps
Add a test which sanity checks all pinmaps.

@c1728p9 c1728p9 force-pushed the c1728p9:pinmap-extension branch from 60fe934 to d5892ce Feb 8, 2019

@c1728p9

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

Rebased on top of master and fixed merge conflicts

@c1728p9

This comment has been minimized.

Copy link
Contributor Author

commented Feb 8, 2019

Re-triggered CI

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 8, 2019

Test run: SUCCESS

Summary: 12 of 12 test jobs passed
Build number : 15
Build artifacts

@0xc0170

This comment has been minimized.

Copy link
Member

commented Feb 11, 2019

Functionality change

One last addition, please add Release notes section , following https://os.mbed.com/docs/mbed-os/v5.11/contributing/workflow.html#functionality-change

cc @AnotherButler to review release notes section

@0xc0170 0xc0170 merged commit 8f932a4 into ARMmbed:master Feb 11, 2019

27 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARM Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARM Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9255 cycles (-664 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details

@c1728p9 c1728p9 deleted the c1728p9:pinmap-extension branch Feb 11, 2019

@c1728p9 c1728p9 referenced this pull request Feb 12, 2019

Merged

Pinmap design doc #9690

@janjongboom

This comment has been minimized.

@0xc0170 This broke the SAML21J18A target.

This comment has been minimized.

Copy link
Member

replied Mar 4, 2019

I'll review the fix soon

This comment has been minimized.

Copy link
Contributor Author

replied Mar 4, 2019

@0xc0170 any idea how this made it through CI?

This comment has been minimized.

Copy link
Member

replied Mar 4, 2019

SAML21J18A is not Mbed OS 5 enabled

This comment has been minimized.

Copy link
Contributor

replied Mar 7, 2019

@0xc0170 Not yet, but I caught it when running the CI for #8658

LMESTM added a commit to LMESTM/mbed that referenced this pull request Mar 12, 2019

STM32WB: Add missing analogin_pinmap
This is required since PR ARMmbed#9449
commit
"Add HAL API for analog in pinmap"

LMESTM added a commit to LMESTM/mbed that referenced this pull request Mar 15, 2019

STM32WB: Add missing analogin_pinmap
This is required since PR ARMmbed#9449
commit
"Add HAL API for analog in pinmap"

LMESTM added a commit to LMESTM/mbed that referenced this pull request Mar 29, 2019

STM32WB: Add missing analogin_pinmap
This is required since PR ARMmbed#9449
commit
"Add HAL API for analog in pinmap"

0xc0170 added a commit that referenced this pull request Apr 5, 2019

STM32WB: Add missing analogin_pinmap
This is required since PR #9449
commit
"Add HAL API for analog in pinmap"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.