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

[TOPIC-GPIO] proposed workaround to distinguish interrupt disable from default no-change #19553

Merged
merged 2 commits into from
Oct 25, 2019

Conversation

pabigot
Copy link
Collaborator

@pabigot pabigot commented Oct 2, 2019

Fixes #19552.

@zephyrbot zephyrbot added the area: API Changes to public APIs label Oct 2, 2019
@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 2, 2019

All checks are passing now.

checkpatch (informational only, not a failure)

-:101: WARNING:LONG_LINE: line over 80 characters
#101: FILE: include/drivers/gpio.h:640:
+	mode = (enum gpio_int_mode)(flags & (GPIO_INT_EDGE | GPIO_INT_DISABLE | GPIO_INT_ENABLE));

- total: 0 errors, 1 warnings, 118 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

@pabigot pabigot force-pushed the gpio/intcfg branch 2 times, most recently from 40e7a83 to c5fa5e2 Compare October 2, 2019 17:07
@dleach02 dleach02 added the DNM This PR should not be merged (Do Not Merge) label Oct 2, 2019
@pabigot pabigot removed the DNM This PR should not be merged (Do Not Merge) label Oct 7, 2019
@pabigot pabigot changed the title DNM proposed workaround to distinguish interrupt disable from default no-change proposed workaround to distinguish interrupt disable from default no-change Oct 7, 2019
@pabigot pabigot changed the title proposed workaround to distinguish interrupt disable from default no-change [TOPIC-GPIO] proposed workaround to distinguish interrupt disable from default no-change Oct 8, 2019
@carlescufi carlescufi requested a review from mnkp October 8, 2019 16:28
@mnkp
Copy link
Member

mnkp commented Oct 24, 2019

This is a well prepared PR, it does solve the problem with interrupt handling that we have. My only concern is that the approach is not quite clean. It's applicable as a temporary workaround but not as a long term solution. In the drivers one can find code like this one:

	if (mode != GPIO_INT_MODE_DISABLED) {
		/* Enable the interrupt. */
		...
	}

Which will be confusing to any new user reading the code. It's because part of the states encoded by enum gpio_int_mode are handled within this API and part by the driver. For the solution to be permanent / long term all of the states should be handled directly by the driver.

I would like to propose another approach that doesn't require adding GPIO_INT_MODE_UNCHANGED state. In z_impl_gpio_pin_interrupt_configure we need to remove the condition if (mode != GPIO_INT_MODE_UNCHANGED) { reverting back to the original implementation. Then we need to add the following assert (next to the existing one):

	__ASSERT((flags & (GPIO_INT_DISABLE | GPIO_INT_ENABLE)) != 0),
		 "Interrupt must be either enabled or disabled");

This way we keep the original behavior. gpio_pin_interrupt_configure function will either enable or disable interrupt. The don't modify behavior is in any case not useful. The gpio_pin_configure function will not accidentally disable interrupts since now empty and non empty interrupt configuration flags can be distinguished. The driver code will not require any updates and will still read well.

gpio_pin_interrupt_configure() verified that the pin was within range,
while gpio_pin_configure() did not.  Make them consistent since they
take the same set of flags.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
@pabigot pabigot force-pushed the gpio/intcfg branch 2 times, most recently from 6f80df5 to 814da32 Compare October 24, 2019 22:15
@pabigot
Copy link
Collaborator Author

pabigot commented Oct 24, 2019

Had this PR been addressed promptly we might not have a bunch of drivers that can't distinguish between interrupt disable and no-change states.

I've made the change as requested.

gpio_pin_interrupt_configure() is invoked from within
gpio_pin_configure() to support legacy code that combines pin and
interrupt configuration.  Expressing a disabled interrupt by a zero
value for interrupt flags causes this invocation to disable interrupts
when the intent is to change only a pin configuration, such as pull
direction.

Support a distinction between explicitly disabling interrupts and
leaving the interrupt configuration unchanged.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
@galak galak merged this pull request into zephyrproject-rtos:topic-gpio Oct 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: GPIO
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants