[TOPIC: GPIO] documentation refinements, test conversion, and sx1509b conversion for new API #18689
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pabigot
added
area: GPIO
area: Tests
|
Found the following issues, please fix and resubmit: checkpatch issues |
|
@mnkp see the mcux commit 4260946e7e0d14a1d0b87e4f4c565476c56839ca which you might want to incorporate into your next update. |
|
@pabigot Thanks for the PR. I'm going to include the following minor updates directly into existing commits in #16648:
this will keep commit history clean. The changes could be part of the review process. I'm going to cherry pick the following commit into #16648:
I would like to include changes proposed in:
in #16648 but that needs a bit more discussion. I'll follow up. Remaining commits can be merged later directly into topic-gpio branch. |
mnkp
reviewed
Aug 29, 2019
include/drivers/gpio.h
Outdated
| * be toggled. | ||
| * Note that the behavior required for pins present in both | ||
| * ``set_pins`` and ``clear_pins`` is to toggle the current pin | ||
| * setting. |
There was a problem hiding this comment.
This text is written as if it was an instruction for a person implementing the driver. The API documentation should be targeted at the users of the API.
include/drivers/gpio.h
Outdated
| * @param clear_pins Value indicating which pins will be set to inactive or | ||
| * toggled. | ||
| * @param set_pins Pins selected by this mask are changed to active if | ||
| * they were not already active. |
There was a problem hiding this comment.
From the commit description:
It also clarifies the expected behavior of the driver in situations
where the driver can detect that an operation would have no effect,
which is signficant for external IO extenders where an unnecessary bus
transaction can be eliminated.
This optimization can be done by the driver also without changing the description. While we mention toggle functionality in the description of the function this text doesn't make it clear that "Pins selected by this mask can be changed to inactive if a matching pin is selected by clear_pins parameter." I'm not saying we shouldn't change the description of set_pins, clear_pins parameters but this proposal doesn't seem like an improvement.
There was a problem hiding this comment.
The issue here is one that was discussed some months ago: if a GPIO output state is requested, and the driver knows the pin is already in that state, does it have to execute an expensive operation that will have no effect? My recollection is the discussion converged to "no, it does not". (I believe this discussion was in a comment somewhere on #11880 that has since disappeared.)
There's a converse policy: if the driver knows the pin is already in that state is it permitted to go ahead and execute the operation anyway? Here I think the answer needs to be "yes" to simplify the common case where the cost of checking for effect is greater than just executing it unconditionally.
Somewhere that has to made clear to both the implementer and the user.
There are two goals of the commit that changes the documentation for this function:
- First, provide an interpretation of
set_pinsandclear_pinsthat explains why having a pin selected by both should result in a toggle operation. The toggle behavior emerges naturally when the effect of pin selection is conditional on pin state: only the selection that would change the state is relevant. It is helpful to explicitly recognize that effect so users and implementers are aware of the expectation. - Second, by identifying that some requests may have no effect it supports an implementer decision to do nothing in cases where the effectless-operation has side effects, e.g. performing one or more external bus transactions (for SX1509B).
I'm open to proposals for alternative wording that accomplish these goals.
pabigot
force-pushed
the
gpio/on16648
branch
2 times, most recently
from
3a6a5dd to
ca9ba44
Compare
|
@mnkp these new commits might be worth squashing into to your branch:
|
mnkp
reviewed
Aug 30, 2019
include/drivers/gpio.h
Outdated
| * Pins which are simultaneously selected by set_pins and clear_pins value will | ||
| * be toggled. | ||
| * Pins that are selected by both set_pins and clear_pins will be | ||
| * cleared to low. |
There was a problem hiding this comment.
I believe the API would be "cleaner" if the function disallowed to have a pin selected by both set_pins and clear_pins values. If the application asks to have a pin set and cleared at the same time something clearly went wrong. It's better to report an error (or catch the issue via ASSERT) rather than quietly clear the pin.
There was a problem hiding this comment.
I think that'd be OK. So the idea would be something like:
__ASSERT((set_pins & clear_pins) == 0,
"Pins cannot be both set and cleared");
return gpio_port_set_masked(port, set_pins | clear_pins, set_pins);
The effect in the default situation of ignoring __ASSERT is that pins that are both set and cleared are actually just set, which is as good a choice as clearing them, and because this is done in the common header inline function such (illegal) usage would produce consistent behavior across drivers, which is desirable.
There was a problem hiding this comment.
OK, I'll implement the change, you don't need to push an update to your PR.
pabigot
force-pushed
the
gpio/on16648
branch
from
77b6295 to
4814a60
Compare
pabigot
changed the title
pabigot
force-pushed
the
gpio/on16648
branch
from
4814a60 to
9043ffc
Compare
mnkp
reviewed
Sep 9, 2019
drivers/gpio/gpio_nrfx.c
Outdated
| @@ -132,6 +132,12 @@ static int gpio_nrfx_config(struct device *port, int access_op, | |||
| u8_t from_pin; | |||
| u8_t to_pin; | |||
|
|
|||
| /* Open Source is not supported on this platform */ | |||
There was a problem hiding this comment.
There was a problem with the testcase, I needed to add pull up / pull down on a pin. I couldn't rely on the LED and resistor to set pin value. The test is passing on nrf52840_pca10056 board. The platform supports Open Drain / Open Source mode.
pabigot
force-pushed
the
gpio/on16648
branch
from
9043ffc to
939c0ab
Compare
|
Some checks failed. Please fix and resubmit. Identity/Emails issuesab0d822: author email (Peter Bigot peter.bigot@nordicsemi.no) needs to match one of the signed-off-by entries. 33a28b4: author email (Peter Bigot peter.bigot@nordicsemi.no) needs to match one of the signed-off-by entries. checkpatch issuesGitlint issuesCommit ab0d822: Commit 33a28b4: Commit 2db9c66: Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages. |
The comments identifying replacement API were inadvertently removed along with the flag that triggers deprecation warnings. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
External GPIO drivers may not be supported from interrupt context because they involve blocking bus transactions. Describe the return value for this situation, and add the I/O error to operations where it was missing. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Add a comment that shows the hexadecimal representation of each flag, to help a user who needs to decode a packed set of flags. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Update driver code and board files to use new GPIO configuration flags such as GPIO_ACTIVE_LOW. Also add implementation of new port_* driver API. Tested on external SX1509B breakout board and Thingy:52. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Test that the new port API functions all behave as expected, including physical vs logical level for input and output as well as masked and set-based output operations. Also tests the new pin API functions. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Use the recommended replacement as the implementation of the deprecated pin read and write functions, rather than calling the old implementation. Retain the existing test, but silence the complaints related to using deprecated functions. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Switch to gpio_pin_interrupt_configure() and the new interrupt flags. Use logical level pin set operations. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Switch to gpio_pin_interrupt_configure() and the new interrupt flags. Use logical level pin set operations. Test all standard interrupt configurations including double edge. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Set the devicetree nodes to correctly reflect active levels for buttons, leds, and antenna switch. Update the board initialization code to use the non-deprecated GPIO API for antenna switching. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Remove the old read and write implementation functions from the driver API table, along with their system call implementations. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
The access-oriented GPIO read and write system calls have been replaced with new API. Remove the old implementation. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
The access-oriented GPIO read and write system calls have been replaced with new API. Remove the old implementation. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
The access-oriented GPIO read and write system calls have been replaced with new API. Remove the old implementation. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
The access-oriented GPIO read and write system calls have been replaced with new API. Remove the old implementation. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Provide board-specific overlays that specify which pins to use for the test, rather than use aliases in the board device tree. This uses a binding that's specifically documented for use in GPIO applications. Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
Time execution of a sequence of operations. s1 test takes 7.02 us on pca10056.
pabigot
force-pushed
the
gpio/on16648
branch
from
939c0ab to
ab0d822
Compare
|
Since this branch has content that won't pass shippable it's not suitable for review. Future reviews will have to be on an unsubmitted branch. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR provides a few documentation refinements to the API header and converts the generic GPIO test to confirm the legacy API and new API works as intended. It also switches the legacy API to be implemented in terms of the new API, though that may be premature.
This is based on PR #16648 under the assumption that's ready to be merged to the topic branch.