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

gpio_api.h: Clarify desired behaviour for NC #10489

Merged
merged 8 commits into from May 21, 2019

Conversation

Projects
None yet
9 participants
@kjbracey-arm
Copy link
Contributor

commented Apr 26, 2019

Description

Documentation clarified, and platforms that don't conform brought into line. Simple test added.

Clarification needed as shown by #7862, #10191, #9469 and #10482.

Pull request type

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

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:gpio_api_nc branch from d12e555 to c2db8a3 Apr 26, 2019

@ciarmcom ciarmcom requested review from ARMmbed/mbed-os-maintainers Apr 26, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Apr 26, 2019

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

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

Updated description looks good.

How many targets follow this? As stated, test would be good and have target code updated to this. Will this be few steps update?

This would be good to target for 5.13 (considering all targets will require update).

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Most, but not all, implement it. As SPI now relies on it (its original constructors now construct a DigitalOut(NC) for an unused software SSEL), any that don't are going to need patching.

So I would say this and target updates and test should go for patch release.

I can look at tests and target updates later this week.

@stevew817

This comment has been minimized.

Copy link
Contributor

commented Apr 29, 2019

IMO this definitely needs to go to patch release (together with #10482) since SPI on the current release is broken for Silicon Labs targets.

See #9469 for more details as to why it broke.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2019

Created a test, and waiting for its results, but by inspection, ARM SSG/FM, Atmel, RTL8195A, EFM32, TT, Wiznet and Ublox HI2110 do not handle NC as specified here.

The rest do.

(It's worth noting that gpio_irq_api does not incorporate this behaviour, and does not have a gpio_irq_is_connected call, so we are inconsistent here. I'd be inclined to bring that in line at some point.)

I've got commits for a test + changing targets (https://github.com/kjbracey-arm/mbed-os/commits/gpio_api_nc_test). Can incorporate those into this PR, or as one or more follow-ups.

@0xc0170

This comment has been minimized.

Copy link
Member

commented Apr 29, 2019

I've got commits for a test + changing targets (https://github.com/kjbracey-arm/mbed-os/commits/gpio_api_nc_test). Can incorporate those into this PR, or as one or more follow-ups.

👍 Lets add it here.

@0xc0170 0xc0170 added needs: work and removed needs: review labels May 2, 2019

kjbracey-arm added some commits Apr 26, 2019

gpio_api.h: Clarify desired behaviour for NC
It would probably be worth adding tests for the ability to initialise NC
pins and check `is_connected`. Some platforms are assert failing the
init, and can't be 100% sure `is_connected` is working on those
platforms either.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:gpio_api_nc branch from c2db8a3 to 2d07210 May 2, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2019

Added the extra commits for testing and correcting NC handling on platforms (other than EFM32 covered by #10482).

@0xc0170
Copy link
Member

left a comment

just SPDX identifier, LGTM otherwise

Show resolved Hide resolved TESTS/mbed_hal/gpio/main.cpp
Add GPIO NC test
Check two items of defined behaviour - that you can initialise a
gpio_t with NC, and you can detect that state with gpio_is_connected().

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:gpio_api_nc branch from 2d07210 to 66f446b May 2, 2019

@0xc0170

0xc0170 approved these changes May 2, 2019

@0xc0170 0xc0170 removed the needs: work label May 2, 2019

@0xc0170 0xc0170 added the needs: CI label May 2, 2019

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Should get reviews from each of the vendors though - not least for confirmation I haven't broken anything. (Not all of those are in CI, and I don't have them).

@0xc0170 0xc0170 added needs: review and removed needs: CI labels May 3, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 3, 2019

@ThunderSoft123 @ARMmbed/team-wiznet @ARMmbed/team-realtek @jamesbeyond Please review gpio files for your targets changed here

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 13, 2019

@ThunderSoft123 @ARMmbed/team-wiznet @ARMmbed/team-realtek @jamesbeyond Please review gpio files for your targets changed here

We would like to proceed with this pull request, please review

cc @maclobdell @MarceloSalazar

@0xc0170

This comment has been minimized.

Copy link
Member

commented May 13, 2019

I started CI job meanwhile

@mbed-ci

This comment has been minimized.

Copy link

commented May 13, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@adbridge

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@maclobdell @MarceloSalazar could you help get this reviewed by partners please ?

@bjnhur

bjnhur approved these changes May 20, 2019

@ThunderSoft123

This comment has been minimized.

Copy link
Contributor

commented May 21, 2019

@0xc0170 No problem with revision.

@0xc0170 0xc0170 merged commit f859289 into ARMmbed:master May 21, 2019

26 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-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage Success
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 Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8652 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8448B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details

@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:gpio_api_nc branch May 21, 2019

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.