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

ARDUINO_UNO form factor and new PinNames for MIMXRT1050 target #14628

Merged
merged 3 commits into from
May 10, 2021

Conversation

notronrj
Copy link

@notronrj notronrj commented May 5, 2021

Summary of changes

These changes add support for the new ARDUINO_UNO PinName standard for the MIMXRT1050 target.
These changes are needed to avoid compiler warnings about ARDUINO form factor being deprecated and allows users of 
MIMXRT1050 to begin to use the new ARDUINO UNO Pin naming standard release in mbed-os v6.10.

Files changed: targets.json, PinNames.h

Impact of changes

No real need to change any application code. However using the old Arduino Pin naming standard of D0..D15 etc should now
be changed to use ARDUINO_UNO_D0 .. ARDUINO_UNO_D15 etc. Same for the analog pin names.

Old PinNames such as D0 can continue to be used if desired because of a pin name alias header file:
hal/include/hal/PinNameAliases.h. However, the next version of mbed-os will not include this alias file.

Migration actions required

Documentation

None.


Pull request type

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers


James Norton added 2 commits May 5, 2021 12:34
Renamed D0..D15 and A0..A5 to ARDUINO_UNO_D0 etc.

This allows user to use ARDUINO_UNO as the supported_form_factors in targets.json for MIMXRT1050_EVK.
@ciarmcom ciarmcom added the release-type: patch Indentifies a PR as containing just a patch label May 5, 2021
@ciarmcom ciarmcom requested a review from a team May 5, 2021 20:00
@ciarmcom
Copy link
Member

ciarmcom commented May 5, 2021

@notronrj, thank you for your changes.
@ARMmbed/mbed-os-maintainers please review.

@notronrj
Copy link
Author

notronrj commented May 5, 2021

The new Arduino pin naming standard porting guide says the following:

All I2C, SPI and UART pin name alias definitions for the Arduino Uno (Rev3) connector pins is defined in the Mbed OS HAL (hal/include/hal/ArduinoUnoAliases.h) and are common to all Arduino Uno compliant targets:

#ifdef TARGET_FF_ARDUINO_UNO
// Arduino Uno I2C signals aliases
#define ARDUINO_UNO_I2C_SDA ARDUINO_UNO_D14
#define ARDUINO_UNO_I2C_SCL ARDUINO_UNO_D15

// Arduino Uno SPI signals aliases
#define ARDUINO_UNO_SPI_CS   ARDUINO_UNO_D10
#define ARDUINO_UNO_SPI_MOSI ARDUINO_UNO_D11
#define ARDUINO_UNO_SPI_MISO ARDUINO_UNO_D12
#define ARDUINO_UNO_SPI_SCK  ARDUINO_UNO_D13

// Arduino Uno UART signals aliases
#define ARDUINO_UNO_UART_TX ARDUINO_UNO_D1
#define ARDUINO_UNO_UART_RX ARDUINO_UNO_D0
#endif // TARGET_FF_ARDUINO_UNO

I don't see this file in mbed-os v6.10. In fact, I only see these definitions in the PinNameAliases.h file which is supposed to be removed in the next version of mbed-os.

@notronrj
Copy link
Author

notronrj commented May 6, 2021

Looks like CI build failed due to pinvalidate failing. The report indicates "generic" is failing but "arduino_uno" is passing.
It's unclear to me why the "generic" pin name portion of the test is failing. My commits don't appear to apply to "generic" pin names, only arduino_uno.

Do I need to do anything?

ARDUINO_UNO_A4 = GPIO_AD_B1_01,
ARDUINO_UNO_A5 = GPIO_AD_B1_00,

I2C_SCL = ARDUINO_UNO_A5,
Copy link

@MarceloSalazar MarceloSalazar May 6, 2021

Choose a reason for hiding this comment

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

These I2C_SCL / I2C_SDA pins should be removed as already defined here #14617

Copy link
Author

Choose a reason for hiding this comment

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

@MarceloSalazar I will remove those lines. After the update and commit to my fork, will that automatically be seen in this PR? Or must a follow some procedure to update this PR with new commits?

Thank you!

Copy link
Author

Choose a reason for hiding this comment

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

@MarceloSalazar I will update the PR today. Thanks again.

@MarceloSalazar
Copy link

MarceloSalazar commented May 6, 2021

Thanks @notronrj for the PR.
You can run the pinvalidate tool with verbose mode to check the reason why the generic pins are failing (we'll consider adding in Travis as well).
However, I see LED1 = LED_GREEN, same as LED2, LED3, LED4 which isn't compliant.

Edit: see docs https://os.mbed.com/docs/mbed-os/v6.10/porting/standard-pin-names.html

@0xc0170
Copy link
Contributor

0xc0170 commented May 6, 2021

I was about to add a comment why I can't see failures at least

0.06s$ git diff --name-only --diff-filter=d FETCH_HEAD..HEAD \
  | ( grep '.*[\\|\/]PinNames.h$' || true ) \
  | while read file; do python ./hal/tests/pinvalidate/pinvalidate.py -vfp "${file}"; done
+-----------------+--------------+----------+---------------+
| Platform name   | Test suite   | Result   |   Error count |
+=================+==============+==========+===============+
| MIMXRT1050_EVK  | generic      | FAILED   |             2 |
+-----------------+--------------+----------+---------------+
| MIMXRT1050_EVK  | arduino_uno  | PASSED   |             0 |
+-----------------+--------------+----------+---------------+
One or more test cases failed
The command "git diff --name-only --diff-filter=d FETCH_HEAD..HEAD \
  | ( grep '.*[\\|\/]PinNames.h$' || true ) \
  | while read file; do python ./hal/tests/pinvalidate/pinvalidate.py -vfp "${file}"; done" exited with 1.
0.03s$ git diff --exit-code --diff-filter=d --color

log should contain what has failed (2 messages indicated what is incorrect). can they be added?

@MarceloSalazar
Copy link

@0xc0170 I think we should add an extra verbose mode in CI, e.g. -vvfp

@notronrj
Copy link
Author

notronrj commented May 6, 2021

Thanks @notronrj for the PR.
You can run the pinvalidate tool with verbose mode to check the reason why the generic pins are failing (we'll consider adding in Travis as well).
However, I see LED1 = LED_GREEN, same as LED2, LED3, LED4 which isn't compliant.

Edit: see docs https://os.mbed.com/docs/mbed-os/v6.10/porting/standard-pin-names.html

@MarceloSalazar I can make the needed changes. I just want to point out that those LED defines have existed since support for the RT1050 EVK board was added. See this commit

My only concern with changing the LED defines is that it could potentially break applications that use them.

Removed I2C_SCL and I2C_SDA from PinName enum as these are now defined in PinNameAliases.h
@notronrj
Copy link
Author

notronrj commented May 6, 2021

@MarceloSalazar @0xc0170
I've committed a new change to PinNames.h that should fix the pinvalidation "generic" pin test failure.
Hopefully this will bring this PR into pin naming compliance.

@0xc0170
Copy link
Contributor

0xc0170 commented May 7, 2021

@0xc0170 I think we should add an extra verbose mode in CI, e.g. -vvfp

@gpsimenos Can you send a new PR?

@mergify mergify bot added needs: CI and removed needs: work labels May 7, 2021
@0xc0170
Copy link
Contributor

0xc0170 commented May 7, 2021

CI started

@mbed-ci
Copy link

mbed-ci commented May 7, 2021

Jenkins CI Test : ✔️ SUCCESS

Build Number: 1 | 🔒 Jenkins CI Job | 🌐 Logs & Artifacts

CLICK for Detailed Summary

jobs Status
jenkins-ci/mbed-os-ci_unittests ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-cloud-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-greentea-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_build-example-ARM ✔️
jenkins-ci/mbed-os-ci_build-example-GCC_ARM ✔️
jenkins-ci/mbed-os-ci_cmake-example-ARM ✔️
jenkins-ci/mbed-os-ci_greentea-test ✔️

@notronrj
Copy link
Author

notronrj commented May 7, 2021

I know it wasn't much of a contribution, but it feels good to have helped out just a little bit. :-)

@0xc0170 0xc0170 merged commit ffd5389 into ARMmbed:master May 10, 2021
@mergify mergify bot removed the ready for merge label May 10, 2021
@mbedmain mbedmain removed release-type: patch Indentifies a PR as containing just a patch Release-pending labels May 24, 2021
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.

None yet

6 participants