Interrupt in pin mode #6239

bmcdonnell-ionx · 2018-02-28T19:55:12Z

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

…tors

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.

pan- · 2018-03-01T10:44:17Z

drivers/InterruptIn.h

@@ -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,


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

~~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 · 2018-03-01T15:13:53Z

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

geky · 2018-03-01T16:06:20Z

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?

geky

Thanks!

cmonr · 2018-03-06T04:06:10Z

/morph build

mbed-ci · 2018-03-06T05:04:17Z

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 · 2018-03-06T08:47:50Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-03-06T13:16:20Z

Test : SUCCESS

Build number : 1136
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6239/1136

0xc0170 · 2018-03-08T11:41:21Z

/morph mbed2-build

0xc0170

LGTM

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

cmonr · 2018-03-21T02:50:53Z

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 · 2018-03-21T13:15:46Z

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?

0xc0170 · 2018-03-21T15:14:25Z

drivers/InterruptIn.cpp

@@ -19,16 +19,33 @@

 namespace mbed {

+// Note: This single-parameter constructor exists to maintain binary
+//       compatibility for the Wi-Fi drivers.


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.

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).

…ty (for Wi-Fi drivers). Revert "simplify InterruptIn - default parameter instead of multiple constructors"; add comment. This reverts commit d28dbf6.

cmonr · 2018-03-22T18:23:50Z

/morph build

mbed-ci · 2018-03-22T18:48:17Z

Build : FAILURE

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

cmonr · 2018-03-22T18:51:18Z

ARM license issues.

/morph build

bmcdonnell-ionx · 2018-03-22T18:53:53Z

@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 · 2018-03-22T19:32:57Z

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 · 2018-03-23T00:08:39Z

Exporter Build : FAILURE

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

cmonr · 2018-03-23T01:54:46Z

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

/morph export-build

mbed-ci · 2018-03-23T04:33:36Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-03-23T07:48:47Z

Test : SUCCESS

Build number : 1314
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/6239/1314

kjbracey · 2018-03-23T09:38:17Z

@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 · 2018-03-23T16:17:35Z

/morph mbed2-build

studavekar · 2018-03-23T16:19:56Z

/morph mbed2-build

bmcdonnell-ionx added 3 commits February 28, 2018 14:00

overload InterruptIn constructor to allow PinMode control

35308c1

factor out common function

dfa6a7a

simplify InterruptIn - default parameter instead of multiple construc…

d28dbf6

…tors

cmonr requested review from geky, pan- and kjbracey February 28, 2018 20:05

geky requested review from c1728p9 and 0xc0170 and removed request for geky February 28, 2018 22:57

cmonr added the needs: review label Feb 28, 2018

pan- previously approved these changes Mar 1, 2018

View reviewed changes

bmcdonnell-ionx added 2 commits March 1, 2018 11:13

add doxygen param info

e4bf6c4

linebreak

087b428

bmcdonnell-ionx dismissed pan-’s stale review via 087b428 March 1, 2018 16:14

geky previously approved these changes Mar 1, 2018

View reviewed changes

0xc0170 previously approved these changes Mar 8, 2018

View reviewed changes

0xc0170 requested a review from SenRamakri March 8, 2018 14:28

geky added the scope: feature label Mar 8, 2018

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

cmonr added the release-version: 5.9.0-rc1 label Mar 21, 2018

0xc0170 added the needs: review label Mar 21, 2018

0xc0170 removed the ready for merge label Mar 21, 2018

bmcdonnell-ionx dismissed stale reviews from 0xc0170 and geky via e685cb6 March 21, 2018 14:30

0xc0170 reviewed Mar 21, 2018

View reviewed changes

bmcdonnell-ionx added 2 commits March 21, 2018 11:18

Restore single-param InterruptIn ctor, to maintain binary compatibili…

895e19a

…ty (for Wi-Fi drivers). Revert "simplify InterruptIn - default parameter instead of multiple constructors"; add comment. This reverts commit d28dbf6.

linebreaks

8d2214f

bmcdonnell-ionx force-pushed the interrupt-in-pin-mode branch from e685cb6 to 8d2214f Compare March 21, 2018 15:22

kjbracey approved these changes Mar 22, 2018

View reviewed changes

SenRamakri approved these changes Mar 22, 2018

View reviewed changes

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

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

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

cmonr removed the ready for merge label Mar 23, 2018

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

Implement PR ARMmbed#6239

069a294

Interrupt in pin mode #6239

Interrupt in pin mode #6239

Uh oh!

Conversation

bmcdonnell-ionx commented Feb 28, 2018 • edited by 0xc0170 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

pan- left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmcdonnell-ionx Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmcdonnell-ionx commented Mar 1, 2018

Uh oh!

geky commented Mar 1, 2018

Uh oh!

geky left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Mar 6, 2018

Uh oh!

mbed-ci commented Mar 6, 2018

Triggering tests

Uh oh!

mbed-ci commented Mar 6, 2018

Uh oh!

mbed-ci commented Mar 6, 2018

Uh oh!

0xc0170 commented Mar 8, 2018

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

cmonr commented Mar 21, 2018

Uh oh!

kjbracey commented Mar 21, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Mar 22, 2018

Uh oh!

mbed-ci commented Mar 22, 2018

Uh oh!

cmonr commented Mar 22, 2018

Uh oh!

bmcdonnell-ionx commented Mar 22, 2018

Uh oh!

mbed-ci commented Mar 22, 2018

Triggering tests

Uh oh!

mbed-ci commented Mar 23, 2018

Uh oh!

cmonr commented Mar 23, 2018

Uh oh!

mbed-ci commented Mar 23, 2018

Uh oh!

mbed-ci commented Mar 23, 2018

Uh oh!

kjbracey commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Mar 23, 2018

Uh oh!

studavekar commented Mar 23, 2018

Uh oh!

Uh oh!

bmcdonnell-ionx commented Feb 28, 2018 •

edited by 0xc0170

Loading

bmcdonnell-ionx Mar 1, 2018 •

edited

Loading

kjbracey commented Mar 23, 2018 •

edited

Loading