InterruptIn changes in NCS36510 HAL. #2938

radhika-raghavendran · 2016-10-06T05:52:15Z

Notes:

Pull requests will not be accepted until the submitter has agreed to the contributer agreement.
This is just a template, so feel free to use/remove the unnecessary things

Description

A few sentences describing the overall goals of the pull request's commits.

Status

READY/IN DEVELOPMENT/HOLD

Migrations

If this PR changes any APIs or behaviors, give a short description of what API users should do when this PR is merged.

YES | NO

Related PRs

List related PRs against other branches:

branch	PR
other_pr_production	link
other_pr_master	link

Todos

Tests
Documentation

Deploy notes

Notes regarding the deployment of this PR. These should note any
required changes in the build environment, tools, compilers, etc.

Steps to test or reproduce

Outline the steps to test or reproduce the PR here.

0xc0170 · 2016-10-07T12:53:10Z

Can you share the test results - how was this tested?

sg- · 2016-10-07T16:30:07Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_api.c

@@ -143,16 +140,17 @@ void gpio_mode(gpio_t *obj, PinMode mode)
 void gpio_dir(gpio_t *obj, PinDirection direction)
 {
    /* Enable the GPIO clock */
-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO))


Formatting needs to match https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:30:29Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_api.c

    /* Enable the GPIO clock */
-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO))


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:30:34Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_api.c

@@ -186,12 +186,12 @@ int gpio_read(gpio_t *obj)
    int ret;

    /* Enable the GPIO clock */
-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO))


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:30:39Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

    GpioReg_pt gpioBase;

    /* Enable the GPIO clock */
-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO))


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:30:43Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

@@ -146,7 +146,10 @@ int gpio_irq_init(gpio_irq_t *obj, PinName pin, gpio_irq_handler handler, uint32
    gpioIds[pin] = id;

    /* Enable the GPIO clock */
-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO))


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:30:49Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

@@ -179,7 +183,10 @@ int gpio_irq_init(gpio_irq_t *obj, PinName pin, gpio_irq_handler handler, uint32
 void gpio_irq_free(gpio_irq_t *obj)
 {
    /* Enable the GPIO clock */
-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO))


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:30:57Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

    /* Enable the GPIO clock */
-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO))


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:31:14Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

@@ -239,9 +246,12 @@ void gpio_irq_set(gpio_irq_t *obj, gpio_irq_event event, uint32_t enable)
 void gpio_irq_enable(gpio_irq_t *obj)
 {
    /* Enable the GPIO clock */
-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO))


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:31:19Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

    /* Enable the GPIO clock */
-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO))


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:31:47Z

targets/TARGET_ONSEMI/TARGET_NCS36510/pinmap.c

-
-        default:
-            break;
+    case PushPullPullDown:


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

sg- · 2016-10-07T16:31:55Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

-                obj->GPIOMEMBASE->IRQ_POLARITY_CLEAR = (IO_ALL ^ (obj->pinMask));
-            }
-            break;
+    case IRQ_RISE:


https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

I did run AStyle on this code before submitting a PR. The result is that AStyle shows changes in files but they don't seem to show up in the PR. Is it recommended to format files manually or is running AStyle the preferred method? Please let us know for future PRs.

kjbracey

What about the disables of GPIO_CLOCK in other modules like I2C and serial?

kjbracey · 2016-10-11T15:27:36Z

targets/TARGET_ONSEMI/TARGET_NCS36510/clock.h

@@ -83,6 +83,7 @@
 #define CLOCK_ENABLE(a)		CLOCKREG->PDIS.WORD &= ~(1 << a)
 #define CLOCK_DISABLE(a)	CLOCKREG->PDIS.WORD |= (uint32_t)(1 << a)

+#define CLOCK_IS_ENABLED(a)     ((CLOCKREG->PDIS.WORD >> a) & 1)?0:1


This is dangerous due to lack of enclosing brackets. Indeed, I believe all your uses of it actually fail.

Actually, no, they do work, but rather by accident - the first expression gets boolean inverted before being fed to the ?, so the right answer comes out.

Does it actually matter if you "enable" a clock that is already enabled anyway? Just a write to the peripheral may be as fast as the read to check if it is enabled.

@0xc0170 suggested in a previous pull request that this implementation be changed from calling an enable function repeatedly. Instead check for the clock and enable only if required.
I have added enclosing brackets.

kjbracey · 2016-10-11T15:29:28Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

-    obj->GPIOMEMBASE->IRQ_POLARITY_SET = (obj->pinMask);
-    obj->GPIOMEMBASE->ANYEDGE_SET = IO_NONE;
+
+    obj->GPIOMEMBASE->W_IN |= obj->pinMask;


Assuming the datasheet I have is correct this is wrong, as is the comment above - you should never be performing read-modify-write on any of these registers.

kjbracey · 2016-10-11T15:29:40Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

+    obj->GPIOMEMBASE->IRQ_EDGE |= obj->pinMask;
+    obj->GPIOMEMBASE->IRQ_POLARITY_SET |= obj->pinMask;
+    obj->GPIOMEMBASE->IRQ_ENABLE_SET |= obj->pinMask;
+    obj->GPIOMEMBASE->ANYEDGE_SET |= IO_NONE;


This presumably should be ANYEDGE_CLEAR = obj->pinMask;

kjbracey · 2016-10-11T15:30:13Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

+    obj->GPIOMEMBASE->W_IN |= obj->pinMask;
+    obj->GPIOMEMBASE->IRQ_EDGE |= obj->pinMask;
+    obj->GPIOMEMBASE->IRQ_POLARITY_SET |= obj->pinMask;
+    obj->GPIOMEMBASE->IRQ_ENABLE_SET |= obj->pinMask;


Should interrupts really be enabled initially, before irq_set or irq_enable is called? I'm not sure.

kjbracey · 2016-10-11T15:30:33Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

-            }
-            break;
+    case IRQ_RISE:
+        obj->GPIOMEMBASE->IRQ_EDGE |= obj->pinMask;


Again, wrong by the data sheet.

kjbracey · 2016-10-11T15:32:15Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

+            obj->GPIOMEMBASE->IRQ_POLARITY_SET |= obj->pinMask;
+        } else if (enable == 0) {
+            /* Disable rising edge */
+            obj->GPIOMEMBASE->IRQ_POLARITY_SET |= (IO_ALL ^ (obj->pinMask));


enable 0 doesn't mean "disable rising edge" - it's a request to disable the interrupts.

I'd suggest two separate tests - one to program IRQ_POLARITY_SET or CLEAR depending on event, and a separate one to program IRQ_ENABLE_SET or IRQ_ENABLE_CLEAR depending on enable.

And again, all of those just being = obj->pinMask, if my register map is correct. (And it seems logical to me).

I have taken care of all the comments above. Would I need to rebase again, since the structure of mbed-os has changed since this PR was generated?

I have rebased my fork on the latest mbed-os structure. Let me know if I need to do something more on my side.

kjbracey

Seems better. Does it work for 2 buttons now?

I'm still worried by all the CLOCK_DISABLE(CLOCK_GPIO) in other drivers. Would it be a good idea to remove those?

kjbracey · 2016-10-13T12:58:10Z

targets/TARGET_ONSEMI/TARGET_NCS36510/clock.h

@@ -82,6 +82,7 @@

 #define CLOCK_ENABLE(a)         CLOCKREG->PDIS.WORD &= ~(1 << a)
 #define CLOCK_DISABLE(a)        CLOCKREG->PDIS.WORD |= (uint32_t)(1 << a)
+#define CLOCK_IS_ENABLED(a)     (((CLOCKREG->PDIS.WORD) >> a) & 1)?0:1


The brackets need to be around the entire expression. Otherwise something like if (!CLOCK_IS_ENABLED(foo) && something_else) would go horribly wrong.

Taken care and modified.

kjbracey · 2016-10-13T12:59:21Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c


-    obj->GPIOMEMBASE->W_IN = (IO_ALL ^ (obj->pinMask));
+    /* Make the pin as output in order to release it */
+    obj->GPIOMEMBASE->W_OUT = obj->pinMask;


Do you really want to make the pin an output? Start driving your input line high or low? Surely not.

Don't you just want to disable the interrupt, but leave the pin as it is?

Taken care and modified.

kjbracey · 2016-10-13T13:00:13Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

            if (enable == 1) {
-                /* Enable rising edge */
-                obj->GPIOMEMBASE->IRQ_POLARITY_SET = (obj->pinMask);
+


This if is the same, and so no longer needs to be in the switch.

Rebase and conflict resolution went horribly wrong and original files were chosen during an auto merge process. Hence none of the recent changes in my fork came out during the rebase. This piece of code has been modified in this checkin.

My comment still applies - you're repeating the same enable code in each case of the switch - the enable code does not depend on the mode, and it doesn't need to be in the switch.

kjbracey · 2016-10-13T13:02:09Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

@@ -157,10 +161,8 @@ int gpio_irq_init(gpio_irq_t *obj, PinName pin, gpio_irq_handler handler, uint32
     * then change this setting to  obj->GPIOMEMBASE->W_IN |= obj->pinMask. All parameter setting needs to change from = to |=
     */


This comment is obsolete and wrong, so should be removed. (Although have you actually tested it with two inputs now? That's what we were trying to get working - the two buttons.)

I have tested with two buttons as well as the same button triggering two inputs. Both scenarios are working.

kjbracey · 2016-10-13T13:07:06Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_api.c

-    CLOCK_ENABLE(CLOCK_GPIO);
+    if(!CLOCK_IS_ENABLED(CLOCK_GPIO)) {
+        CLOCK_ENABLE(CLOCK_GPIO);
+    }


I'd like to ask @0xc0170 whether he really thinks this is worthwhile. Reading the hardware to find out if it's worth writing to the hardware? More code, and probably no faster - usually 1 read instead of 1 write.

The GPIO disable in other drivers was also part of a rebase and automerge gone horribly wrong. I hope it is reflected in my latest checkin and pull request. Please let me know.

I'd like to ask @0xc0170 whether he really thinks this is worthwhile

It's not, good point !

bridadan · 2016-10-17T22:33:16Z

/morph test

mbed-bot · 2016-10-18T08:15:40Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 0

All builds and test passed!

radhika-raghavendran · 2016-10-18T12:01:03Z

Is this merged with the main branch? The label status has not changed over the last couple of days. All the tests have passed in the checklist.

0xc0170 · 2016-10-18T12:29:19Z

Is this merged with the main branch? The label status has not changed over the last couple of days. All the tests have passed in the checklist.

What do you think about checking if gpio is enabled vs just enable the gpio clocks? The question asked above.

It would be useful to provide more details in the commit messages

…c and gpio_irq_api.c

radhika-raghavendran · 2016-10-19T04:26:40Z

I have reverted GPIO clock enablement code to a previous implementation which was present in older version of the code.
I hope this pull request can be merged with main branch now.

kjbracey

Okay, but still a couple of minor concerns.

kjbracey · 2016-10-19T07:13:20Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

@@ -178,10 +169,11 @@ int gpio_irq_init(gpio_irq_t *obj, PinName pin, gpio_irq_handler handler, uint32
 */
 void gpio_irq_free(gpio_irq_t *obj)
 {
-    /* Enable the GPIO clock */
+    /* Enable the GPIO clock which may have been switched off by other drivers */


Really you need to ensure no other driver does turn it off - assuming the clock needs to stay running for the interrupts to work, not just for the register access? (If the clock only needed to be on for the register access, you could disable it at the end of the function).

kjbracey · 2016-10-19T07:14:24Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

            if (enable == 1) {
-                /* Enable rising edge */
-                obj->GPIOMEMBASE->IRQ_POLARITY_SET = (obj->pinMask);
+


My comment still applies - you're repeating the same enable code in each case of the switch - the enable code does not depend on the mode, and it doesn't need to be in the switch.

radhika-raghavendran · 2016-10-19T08:16:47Z

IRQ enable code moved out of switch case. Regarding clock disable of GPIO from other drivers, other code may need modification and testing. Hence CLOCK_ENABLE(CLOCK_GPIO) remains in code for now.

kjbracey · 2016-10-19T08:18:12Z

targets/TARGET_ONSEMI/TARGET_NCS36510/gpio_irq_api.c

            break;

        default:
            /* No event is set */
            break;
    }
+    /* Enable the IRQ based on enable parameter */
+    if (enable == 1) {


I personally would simplify further to just if (enable) {} else {}, on the assumption that it is supposed to be a boolean, even if not declared as that. But this is fine.

radhika-raghavendran · 2016-10-19T08:20:38Z

The IRQ API is void gpio_irq_set(gpio_irq_t *obj, gpio_irq_event event, uint32_t enable)
enable is a 32 bit parameter, where a simple Boolean would have been sufficient. Hence checking against 1 or 0.

kjbracey · 2016-10-19T08:31:26Z

Hence checking against 1 or 0.

Hard to say whether that's better or worse, as it's not actually documented anywhere (as far as I can see) that the "enable" value is specifically 1.

Generally speaking writing if (x == true) is bad C style, as it looks like it's the same as if (x) or the opposite of if (!x) or of if (x == false), but it isn't.

Specifically writing it like this means you are expecting future expansion, so deliberately want to do nothing if 2 is passed. I personally think if passed 2, it's probably better to enable.

Quick survey of 4 other drivers showed the 3 doing if (enable) else, and one evil driver doing if (enable == 1) else.

But I don't actually really care - this is fine if you prefer it. It seems safe to assume (given all the different current driver behaviour) that extra values will never be used.

radhika-raghavendran · 2016-10-19T08:43:14Z

Done. Check is now against if(enable) else.

radhika-raghavendran · 2016-10-19T10:49:38Z

All changes in place now. All checks have passed.

sg- · 2016-10-19T11:38:12Z

/morph test

mbed-bot · 2016-10-19T14:55:19Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 0

All builds and test passed!

Ports for Upcoming Targets Fixes and Changes 2966: Add kw24 support ARMmbed/mbed-os#2966 3068: MultiTech mDot - clean up PeripheralPins.c and add new pin names ARMmbed/mbed-os#3068 3089: Kinetis HAL: Remove clock initialization code from serial and ticker ARMmbed/mbed-os#3089 2943: [NRF5] NVIC_SetVector functionality ARMmbed/mbed-os#2943 2938: InterruptIn changes in NCS36510 HAL. ARMmbed/mbed-os#2938 3108: Fix sleep function for NRF52. ARMmbed/mbed-os#3108 3076: STM32F1: Correct timer master value reading ARMmbed/mbed-os#3076 3085: Add LOWPOWERTIMER capability for NUCLEO_F303ZE ARMmbed/mbed-os#3085 3046: [BEETLE] Update BLE stack on Beetle board ARMmbed/mbed-os#3046 3122: [Silicon Labs] Update of Silicon Labs HAL ARMmbed/mbed-os#3122 3022: OnSemi RAM usage fix ARMmbed/mbed-os#3022 3121: STM32F3: Correct UART4 and UART5 defines when using DEVICE_SERIAL_ASYNCH ARMmbed/mbed-os#3121 3142: Targets- NUMAKER_PFM_NUC47216 remove mbed 2 ARMmbed/mbed-os#3142

sg- reviewed Oct 7, 2016

View reviewed changes

sg- added the needs: work label Oct 7, 2016

kjbracey reviewed Oct 11, 2016

View reviewed changes

Serial printf and InterruptIn changes

20e6a05

radhika-raghavendran force-pushed the master branch from 177322b to 20e6a05 Compare October 13, 2016 12:10

kjbracey reviewed Oct 13, 2016

View reviewed changes

radhika-raghavendran added 2 commits October 14, 2016 18:16

Fixed automerge problems.

b026524

gpio is connected API

a3bccfd

0xc0170 added needs: review and removed needs: work labels Oct 18, 2016

Reverting Clock GPIO code to previous implementation of the gpio_api.…

17ed9e9

…c and gpio_irq_api.c

kjbracey approved these changes Oct 19, 2016

View reviewed changes

IRQ enable code moved out.

1573405

kjbracey reviewed Oct 19, 2016

View reviewed changes

kjbracey approved these changes Oct 19, 2016

View reviewed changes

Enable not checking against 0 or 1.

7598b05

sg- added needs: CI and removed needs: review labels Oct 19, 2016

0xc0170 added ready for merge release-version: 5.2.2 release-version: 5.2.1 and removed needs: CI release-version: 5.2.2 ready for merge labels Oct 19, 2016

0xc0170 merged commit 2175009 into ARMmbed:master Oct 25, 2016

		@@ -157,10 +161,8 @@ int gpio_irq_init(gpio_irq_t *obj, PinName pin, gpio_irq_handler handler, uint32
		* then change this setting to obj->GPIOMEMBASE->W_IN \|= obj->pinMask. All parameter setting needs to change from = to \|=
		*/

InterruptIn changes in NCS36510 HAL. #2938

InterruptIn changes in NCS36510 HAL. #2938

Conversation

radhika-raghavendran commented Oct 6, 2016

Description

Status

Migrations

Related PRs

Todos

Deploy notes

Steps to test or reproduce

0xc0170 commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey left a comment

Choose a reason for hiding this comment

kjbracey Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjbracey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridadan commented Oct 17, 2016

mbed-bot commented Oct 18, 2016

Output

radhika-raghavendran commented Oct 18, 2016

0xc0170 commented Oct 18, 2016

radhika-raghavendran commented Oct 19, 2016

kjbracey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radhika-raghavendran commented Oct 19, 2016

Choose a reason for hiding this comment

radhika-raghavendran commented Oct 19, 2016

kjbracey commented Oct 19, 2016

radhika-raghavendran commented Oct 19, 2016

radhika-raghavendran commented Oct 19, 2016

sg- commented Oct 19, 2016

mbed-bot commented Oct 19, 2016

Output

kjbracey Oct 11, 2016 •

edited

Loading

kjbracey Oct 11, 2016 •

edited

Loading

kjbracey Oct 11, 2016 •

edited

Loading