Skip to content

zephyrCommon: Implement attachInterrupt()/detachInterrupt() #51

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

Merged
merged 2 commits into from
Nov 19, 2022

Conversation

soburi
Copy link
Member

@soburi soburi commented Sep 19, 2022

This implementation depends on CONFIG_GPIO_GET_CONFIG
to keep the GPIO state already configured by pinMode().

@soburi
Copy link
Member Author

soburi commented Sep 19, 2022

I'm testing with nrf52840 with this patch https://github.com/soburi/zephyr/tree/nrfx_gpio_get_config
(This patch needs more verification.)

@soburi soburi marked this pull request as draft September 24, 2022 05:15
@soburi soburi marked this pull request as ready for review September 24, 2022 06:34
@soburi soburi force-pushed the attachInterrupt branch 2 times, most recently from c01299f to 98cd47f Compare September 24, 2022 22:19
@@ -0,0 +1,4 @@
CONFIG_GPIO=y
CONFIG_CPLUSPLUS=y
Copy link
Collaborator

Choose a reason for hiding this comment

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

The module Kconfig already takes care of CPP and GPIO configs, if ARDUI API is enabled, so this can be dropped from prj.conf

        imply CPLUSPLUS 
         imply GPIO

Copy link
Collaborator

Choose a reason for hiding this comment

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

@soburi can you take a look at if this is required again please?

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed CONFIG_CPLUSPLUS and CONFIG_GPIO.

}
}

void pinModeImpl(pin_size_t pinNumber, gpio_flags_t pinmode, voidFuncPtr callback, gpio_flags_t intmode)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems like this PR is doing much more than just Implementing attachInterrupt()/detachInterrupt, and thus in my opinion deserves to have it's own commit that separates the modifications that have been made in the way that pinMode was earlier implemented.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also if possible, the commit message should justify the use of new pinMode being better than earlier one and also the addition of GPIO_MODE_MASK instead of directly using each individual GPIO_XX flag as and when required?

Copy link
Member Author

Choose a reason for hiding this comment

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

As a result of redesign, I was able to eliminate this.
It's just attachInterrupt/detachInterrupt and their internal implementation.

@soburi soburi marked this pull request as draft October 12, 2022 10:47
@soburi
Copy link
Member Author

soburi commented Oct 12, 2022

@DhruvaG2000

Redesign based on #60.

@soburi soburi requested review from DhruvaG2000 and removed request for szczys October 12, 2022 11:02
@DhruvaG2000
Copy link
Collaborator

DhruvaG2000 commented Oct 13, 2022

Any reason this is still a draft PR? Please mark as ready if you want it to be reviewed,
Thanks.

@soburi
Copy link
Member Author

soburi commented Oct 13, 2022

@DhruvaG2000

Any reason this is still a draft PR? Please mark as ready if you want it to be reviewed,
Thanks.

Because it is depends to #60.
I will remove the draft mark after approved #60.

The internal data structure named `gpio_port_callback` stores
GPIO callback handlers.
The `gpio_port_callback` allocate the data per GPIO device.

The `gpio_port_callback` has an array of function pointers
to store handler. The largest count of the gpio pin count
among the GPIO devices that using (which means it exists
in the `digital-pin-gpios`) determines the array size.

It calculates the data size at compile time.

Signed-off-by: TOKITA Hiroshi <[email protected]>
Add a sample to demonstrate how to use attachInterrupt

Signed-off-by: TOKITA Hiroshi <[email protected]>
@soburi soburi marked this pull request as ready for review November 13, 2022 23:13
Copy link
Collaborator

@DhruvaG2000 DhruvaG2000 left a comment

Choose a reason for hiding this comment

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

Tested on my ble sense 33 and I was able to toggle LED by giving interrupts on D2 pin.
Hence,
Tested-by: Dhruva Gole

Thanks for this great feature addition @soburi !

@DhruvaG2000 DhruvaG2000 merged commit 6d919d8 into zephyrproject-rtos:main Nov 19, 2022
@soburi soburi deleted the attachInterrupt branch November 19, 2022 21:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants