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

Interrupt in pin mode #6239

Merged
merged 7 commits into from Mar 23, 2018

Conversation

@bmcdonnell-ionx
Contributor

bmcdonnell-ionx commented Feb 28, 2018

Description

Currently there's no way to specify the PinMode (PullUp, PullDown, etc.) in the InterruptIn constructor. So I added one, like DigitalIn.

Later: Would it make even more sense to have InterruptIn inherit from DigitalIn?

Pull request type

  • Fix (arguably)
  • Refactor
  • New Target
  • Feature

@cmonr cmonr requested review from geky, pan- and kjbracey-arm Feb 28, 2018

@geky geky requested review from c1728p9 and 0xc0170 and removed request for geky Feb 28, 2018

@cmonr cmonr added the needs: review label Feb 28, 2018

@pan-

Thanks for the submission.

Would it make even more sense to have InterruptIn inherit from DigitalIn?

I think it would make sense, an InterruptIn is a DigitalIn.

@@ -66,6 +66,12 @@ class InterruptIn : private NonCopyable<InterruptIn> {
* @param pin InterruptIn pin to connect to
*/
InterruptIn(PinName pin);
/** Create an InterruptIn connected to the specified pin,

This comment has been minimized.

@pan-

pan- Mar 1, 2018

Member

You may want a line break here; same between the constructor and the destructor.

This comment has been minimized.

@bmcdonnell-ionx

bmcdonnell-ionx Mar 1, 2018

Contributor

I tried to match the existing style. Doesn't matter to me which way you prefer it. Let me know, or feel free to change it yourself.

EDIT: Done.

@bmcdonnell-ionx

This comment has been minimized.

Contributor

bmcdonnell-ionx commented Mar 1, 2018

Is there something I need to do to resolve these CI failures?

@geky

This comment has been minimized.

Member

geky commented Mar 1, 2018

Ah, it looks like the new mode parameter doesn't have documentation.

Could you add a "@param mode Description of mode" line to the doxygen comment?

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 6, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 6, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Mar 8, 2018

/morph mbed2-build

@0xc0170

LGTM

The same behavior as it is now (gpio_init_in sets it to pull default).

@0xc0170 0xc0170 requested a review from SenRamakri Mar 8, 2018

@geky geky added the scope: feature label Mar 8, 2018

@cmonr cmonr added ready for merge and removed needs: review labels Mar 21, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 21, 2018

Marking as "Ready to Merge" due to lack of review activity.

If anyone else has any additional input, speak now. @SenRamakri @pan- @kjbracey-arm @c1728p9

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Mar 21, 2018

Extension is reasonable from a source point of view, but introduces a binary compatibility break likely to hit the Wi-fi drivers that keep breaking.

Could you retain the original constructor and add a new one, rather than adding a default parameter?

@bmcdonnell-ionx bmcdonnell-ionx dismissed stale reviews from 0xc0170 and geky via e685cb6 Mar 21, 2018

@@ -19,16 +19,33 @@
namespace mbed {
// Note: This single-parameter constructor exists to maintain binary
// compatibility for the Wi-Fi drivers.

This comment has been minimized.

@0xc0170

0xc0170 Mar 21, 2018

Member

This would be fine in the commit message but I would not keep this in the source here.

It is not just for wifi driver, it was written as an example that the previous change broke binary compability and we should keep it as far as we can, and here we could do it.

This comment has been minimized.

@bmcdonnell-ionx

bmcdonnell-ionx Mar 21, 2018

Contributor

I removed the bit about Wi-Fi drivers, but left the comment. My thinking is I don't want someone to come along later and think they can simplify like I did. A comment in the code should prevent that, whereas a comment just in a commit message is less likely to do so.

If you still want it gone, LMK, or you can remove it (I gave maintainers push permission on this branch).

bmcdonnell-ionx added some commits Mar 21, 2018

Restore single-param InterruptIn ctor, to maintain binary compatibili…
…ty (for Wi-Fi drivers).

Revert "simplify InterruptIn - default parameter instead of multiple constructors"; add comment.

This reverts commit d28dbf6.

@bmcdonnell-ionx bmcdonnell-ionx force-pushed the bmcdonnell-ionx:interrupt-in-pin-mode branch from e685cb6 to 8d2214f Mar 21, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 22, 2018

/morph build

@cmonr cmonr added needs: CI and removed needs: review labels Mar 22, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 22, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 22, 2018

ARM license issues.

/morph build

@bmcdonnell-ionx

This comment has been minimized.

Contributor

bmcdonnell-ionx commented Mar 22, 2018

@kjbracey-arm

Extension is reasonable from a source point of view, but introduces a binary compatibility break likely to hit the Wi-fi drivers that keep breaking.

I just realized... Did you mean that as a response to this?

Later: Would it make even more sense to have InterruptIn inherit from DigitalIn?

Meaning that no one should do this?

@mbed-ci

This comment has been minimized.

mbed-ci commented Mar 22, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 23, 2018

I suspect a Jenkins machine decided to call it a day a bit too early.

/morph export-build

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Mar 23, 2018

@bmcdonnell-ionx - that would be another compatibility break, but just removing the no-parameter constructor is enough to be a breakage.

We don't generally guarantee no breakage - it often happens accidentally - but for this simple case we can avoid removing a constructor that at least one binary component is likely to be calling.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Mar 23, 2018

/morph mbed2-build

1 similar comment
@studavekar

This comment has been minimized.

Collaborator

studavekar commented Mar 23, 2018

/morph mbed2-build

@cmonr cmonr added ready for merge and removed needs: CI labels Mar 23, 2018

@cmonr cmonr merged commit 801f27e into ARMmbed:master Mar 23, 2018

11 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build 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 Passed, code size is 10092B
Details
travis-ci/tools Local tools testing has passed
Details

@cmonr cmonr removed the ready for merge label Mar 23, 2018

marcemmers added a commit to marcemmers/mbed-os that referenced this pull request Mar 28, 2018

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