[TOPIC-GPIO] miscellaneous API documentation updates #19252

pabigot · 2019-09-18T15:25:40Z

Make behavior and future intent more clear.

pabigot · 2019-09-18T15:28:14Z

@dbkinder could you please look at the last commit currently 45ef15c2e7923b17efa84b27eea227edd9a5d956 and tell me if there is some way to put content at the top of the API page. Basically I'd like a block of text at the top of the "API Reference" section, but to have it come from the header.

carlescufi · 2019-09-18T15:31:47Z

include/drivers/gpio.h

+ * * All functions may be invoked from interrupt service routines and
+ *   callbacks.  Functions that cannot fulfill their behavioral
+ *   requirements in that context may return `-EWOULDBLOCK`.
+ * * All pin configuration functions must be atomic.


Do you think atomic reflects the fact that we (will) disable interrupts as opposed to using another synchronization mechanism? I think it does, and it is sort of implied by the fact that all functions can be called from an ISR, but just checking.

I think atomic means what I said it means in #18970. Which we should reach agreement on and document.

Fair enough. Let's use the terms as they are in #18970 and get a PR in for that.

galak

I think the commit for updating the atomicity should be pulled out into its own PR that also updates the new drivers to match.

pabigot · 2019-09-18T16:26:05Z

I think the commit for updating the atomicity should be pulled out into its own PR that also updates the new drivers to match.

There's a problem with that: I'm not going to change the drivers because I don't know the underlying HALs and what atomicity promises they make, and even if I do have hardware for all the drivers it's not an effective use of my time to test my inept attempts to update them.

I really think driver changes should be the responsibility of the people who know and normally maintain the drivers, so they can do things they way they want them done, rather than having to redo what somebody else did.

carlescufi · 2019-09-18T18:51:13Z

I think the commit for updating the atomicity should be pulled out into its own PR that also updates the new drivers to match.

I think we can live with this. The driver updates will likely come from @mnkp, and there's a reason we have a staging branch. If you prefer what we could do is wait until the existing (new) drivers are updated to reflect the text in this PR in topic-gpio and then merge the present one.

galak · 2019-09-18T19:02:52Z

I think we can live with this. The driver updates will likely come from @mnkp, and there's a reason we have a staging branch. If you prefer what we could do is wait until the existing (new) drivers are updated to reflect the text in this PR in topic-gpio and then merge the present one.

However we want to do it is fine, we can either hold this PR until there's a merged PR with the changes or we drop the commit for that text and have @mnkp pick it up in a PR with the atomic changes.

carlescufi · 2019-09-19T09:13:31Z

I think we can live with this. The driver updates will likely come from @mnkp, and there's a reason we have a staging branch. If you prefer what we could do is wait until the existing (new) drivers are updated to reflect the text in this PR in topic-gpio and then merge the present one.

However we want to do it is fine, we can either hold this PR until there's a merged PR with the changes or we drop the commit for that text and have @mnkp pick it up in a PR with the atomic changes.

@mnkp and @pabigot what's your point of view? We should get this done this week so we can send an email to driver maintainers as soon as possible.

pabigot · 2019-09-19T10:17:37Z

However we want to do it is fine, we can either hold this PR until there's a merged PR with the changes or we drop the commit for that text and have @mnkp pick it up in a PR with the atomic changes.

@mnkp and @pabigot what's your point of view? We should get this done this week so we can send an email to driver maintainers as soon as possible.

@galak's options both require waiting to document the requirement until all drivers are updated to comply with it. I see no motivation for that, and strong motivation for adding the documentation now so people who look at this in the branch are informed what's expected of them. That full specification of API behavior was not addressed before the original merge was a process failure.

Note there are two commits that specify atomicity behavior: one generically, and one at each description of expected behavior.

If consensus is to not document atomicity behavior until it's implemented I'll remove both commits.

galak · 2019-09-19T13:14:32Z

@galak's options both require waiting to document the requirement until all drivers are updated to comply with it. I see no motivation for that, and strong motivation for adding the documentation now so people who look at this in the branch are informed what's expected of them. That full specification of API behavior was not addressed before the original merge was a process failure.

Note there are two commits that specify atomicity behavior: one generically, and one at each description of expected behavior.

If consensus is to not document atomicity behavior until it's implemented I'll remove both commits.

Just to be clear I mean the 4 drivers that have been updated. I'm assuming that should be a relatively quick thing for @mnkp to handle.

mnkp · 2019-09-19T22:17:51Z

That full specification of API behavior was not addressed before the original merge was a process failure.

There is a lot of work that needs to be done before the GPIO API is complete and clean. We've started by adapting flags used in DT to match Linux bindings. That required also some related changes to the API functions. That was the only goal of the PR that got merged on the topic branch. Which I've stated and re-stated multiple times already. Having atomicity and thread safety is important but @pabigot you're just piling extra work on top of an already huge pile. I would rather focus my time and energy on the two pin testcase to ensure that the functionality that we've implemented works as advertised. Also we need to convert more drivers. We're still not 100% sure that the we don't hit a block when implementing gpio_port_* functions on all the different platforms.

That said I appreciate the fact @pabigot that you've provided the proposal. I'll be happy to contribute to this issue as soon as I find some time.

pabigot · 2019-09-19T22:33:43Z

@mnkp Please understand, I'm not the one asking you to make these changes. I think they should be done by the normal maintainers as part of a pass through confirming that the implementation is what they want to deal with in the future. I absolutely do not understand why @galak wants the implementation changes made before we document the required behavior which (I hope) we all agree must be supported.

However, I accepted 16648 with the expectation that we would get the full definition of the API in place. Which means defining the expected behavior and interaction with respect to interrupts and context switches, even if we haven't implemented it yet.

We should not be asking people to convert drivers, then coming back two weeks later and asking them to change them because we finally got the specification complete.

@galak please reconsider your NACK of the documentation-only PR. This is only slowing us down.

As for the two-pin test case, AFAICT nobody's looked #19249 yet. Yes, for right now you need to hard-code specific pins. We can fix that up later.

mnkp · 2019-09-19T23:40:14Z

We should not be asking people to convert drivers, then coming back two weeks later and asking them to change them because we finally got the specification complete.

We're doing incremental changes. It's not like we will need to throw away the code we wrote and write it again. When atomicity/thread safety concept is complete I expect I will need to review the code and throw in some irq_locks in a few places. I understand that you have a different perspective since you were working on an external GPIO driver where this issues have higher impact. However, the atomicity/thread safety concept has been missing for a while now.

I would rather split the whole thing in a smaller manageable (and as much as possible independent) packages. If there are things which need to be handled together then we have no choice.

Regarding atomicity I believe we should handle the thread safety at the same time. Even if only to ensure that the returned error codes are what we want them to be. Thread safety will be required for external gpio drivers.

pabigot · 2019-09-19T23:45:07Z

Regarding atomicity I believe we should handle the thread safety at the same time. Even if only to ensure that the returned error codes are what we want them to be. Thread safety will be required for external gpio drivers.

It's supposed to all be covered. See #18530 (comment) and the linked general issue #18970 where the meaning of the terms is being defined.

mnkp · 2019-09-22T21:17:26Z

Regarding atomicity I believe we should handle the thread safety at the same time. Even if only to ensure that the returned error codes are what we want them to be. Thread safety will be required for external gpio drivers.

It's supposed to all be covered. See #18530 (comment) and the linked general issue #18970 where the meaning of the terms is being defined.

This PR covers atomicity only. I believe we should handle thread safety at the same time since both issues are related. If we guarantee atomicity in a normal SoC GPIO the thread safety is also insured but this doesn't hold true for the external GPIOs like gpio_sx1509b where we need to communicate over I2C bus.

pabigot · 2019-09-22T22:10:49Z

I hadn't intended an exception from the atomicity requirement for non-SOC implementations; however the original definitions in #18970 improperly required it to encompass "non-blocking", which was a problem.

I've updated the PR to note a requirement for both thread- and interrupt-safety for all functions. Reentrancy is not relevant for any current function.

mnkp · 2019-09-25T20:17:20Z

include/drivers/gpio.h

+ * 0x 000C 0000  18..19  DS_LOW
+ * 0x 0030 0000  20..21  DS_LOW
+ * 0x FFC0 0000  22..31  unused
+ */


I believe we should drop the commit that adds this table. It's not really useful. There is no precedence, we don't have similar tables for other APIs and I wouldn't introduce one. This things tend to get outdated very quickly and can do more harm than help.

I find it extremely useful when debugging flags to be able to decode what some particular hex value maps into.

The interrupt flags are mapped to the enums so this part is not really useful. In any case, you are always welcome to keep and maintain your local copy.

mnkp · 2019-09-25T20:23:22Z

include/drivers/gpio.h

@@ -702,11 +757,15 @@ static inline int z_impl_gpio_port_get_raw(struct device *port,
 * Value of a pin with index n will be represented by bit n in the returned
 * port value.
 *
+ * This function delegates to `gpio_port_get_raw()` then converts line
+ * level to logic level.


Unlike the comment that explains functionality using pseudo code, which can be helpful to the user, this one merely describes the implementation detail. This is neither interesting nor relevant to the user and potentially can change. I propose to remove it. As well as all the other comments that talk about delegating functionality to another function.

Anybody else have an opinion? I can do this, though it will look weird if we define precise behavior for the _raw() functions and don't explain how the non-raw ones accomplish their task.

It seems like you're trying to add text just for the sake of adding text. The function behaves the same as gpio_port_get_raw() with the difference that it returns port logical value. We could use the same pseudo code adding that the value returned is a logical one.

mnkp · 2019-09-25T20:36:33Z

include/drivers/gpio.h

@@ -1055,6 +1179,8 @@ static inline int gpio_pin_toggle(struct device *port, unsigned int pin)
 * @param pin Pin number where the data is written.
 * @param value Value set on the pin.
 * @return 0 if successful, negative errno code on failure.
+ *
+ * @deprecated Replace with gpio_pin_set_raw().


gpio_pin_write() function honors GPIO_POL_INV flag. That was implemented by some of the drivers. So the text should say "Replace with gpio_pin_set() or gpio_pin_set_raw()."

There is one more issue, @deprecated doesn't seem to be rendered by the doxygen for some reason. We need to look at this, maybe @dbkinder could help? In any case, depending on how it's going to be rendered we may need to mentioned explicitly that the function was deprecated. As in

* @deprecated This function has been deprecated, please use gpio_pin_set() or * gpio_pin_set_raw() instead.

include/drivers/gpio.h

mnkp · 2019-09-25T21:16:57Z

include/drivers/gpio.h

+ *
+ * * All functions shall be thread-safe and interrupt-safe.
+ * * All functions may be invoked from interrupt service routines and
+ *   callbacks.  Functions that cannot fulfill their behavioral


We repeat ourselves. We've mentioned already that the functions are interrupt safe and then we say "All functions may be invoked from interrupt service routines and callbacks". I would remove the sentence.

No; "interrupt safe" and "can be invoked from ISRs and callbacks" are two different things.

What is the difference?

include/drivers/gpio.h

mnkp · 2019-09-25T21:22:42Z

include/drivers/gpio.h

+ * * All functions may be invoked from interrupt service routines and
+ *   callbacks.  Functions that cannot fulfill their behavioral
+ *   requirements in that context may return `-EWOULDBLOCK`.
+ * * All pin, interrupt, and callback configuration functions must be


Current implementation of callback configuration functions (which is common to all drivers) is not thread safe, neither atomic. Someone needs to implement the feature. Should be likely in the same PR.

The comments identifying replacement API were inadvertently removed along with the flag that triggers deprecation warnings. Signed-off-by: Peter Bigot <[email protected]>

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 <[email protected]>

pabigot · 2019-09-26T09:53:18Z

Until #18970 gets some love and everybody agrees on what terms related to thread and interrupt safety and atomicity mean we can't document what the required behavior is.

Also @mnkp has a point about thread safety of callback registration. There are specific guarantees in the existing text about adding and removing a callback from an interrupt, which is enough for me.

I've removed the objectionable commits. If somebody else wants some of them back, please feel free to do that in another PR.

@galak @mnkp please revisit your reviews.

Blocked issues have been dropped

N/A

pabigot requested review from carlescufi, galak, MaureenHelm and nashif as code owners September 18, 2019 15:25

pabigot added area: Documentation area: GPIO labels Sep 18, 2019

carlescufi reviewed Sep 18, 2019

View reviewed changes

zephyrbot added the area: API Changes to public APIs label Sep 18, 2019

galak previously requested changes Sep 18, 2019

View reviewed changes

carlescufi approved these changes Sep 18, 2019

View reviewed changes

galak force-pushed the topic-gpio branch from 5fc6ccb to 4bc09ff Compare September 19, 2019 20:29

galak force-pushed the gpio/20190918a branch from 3293b2b to 61ad826 Compare September 19, 2019 20:31

pabigot force-pushed the gpio/20190918a branch from 61ad826 to 0931b43 Compare September 22, 2019 22:09

pabigot force-pushed the gpio/20190918a branch from 0931b43 to 9bf5c3e Compare September 25, 2019 14:25

mnkp previously requested changes Sep 25, 2019

View reviewed changes

pabigot added 2 commits September 25, 2019 17:25

gpio: put back deprecation comments

e5ed5c6

The comments identifying replacement API were inadvertently removed along with the flag that triggers deprecation warnings. Signed-off-by: Peter Bigot <[email protected]>

pabigot force-pushed the gpio/20190918a branch from 9bf5c3e to 665ae82 Compare September 25, 2019 22:25

mnkp self-requested a review September 26, 2019 09:22

pabigot force-pushed the gpio/20190918a branch from 665ae82 to dc20b2b Compare September 26, 2019 09:51

carlescufi requested a review from galak October 1, 2019 16:28

galak approved these changes Oct 1, 2019

View reviewed changes

galak merged commit 470d655 into zephyrproject-rtos:topic-gpio Oct 1, 2019

pabigot deleted the gpio/20190918a branch October 5, 2019 17:04

pabigot mentioned this pull request Nov 20, 2019

Define and document expected behavior of API calls in various contexts #18970

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TOPIC-GPIO] miscellaneous API documentation updates #19252

[TOPIC-GPIO] miscellaneous API documentation updates #19252

pabigot commented Sep 18, 2019

pabigot commented Sep 18, 2019

carlescufi Sep 18, 2019

pabigot Sep 18, 2019

carlescufi Sep 18, 2019

galak left a comment

pabigot commented Sep 18, 2019

carlescufi commented Sep 18, 2019

galak commented Sep 18, 2019

carlescufi commented Sep 19, 2019

pabigot commented Sep 19, 2019

galak commented Sep 19, 2019

mnkp commented Sep 19, 2019

pabigot commented Sep 19, 2019 •

edited

Loading

mnkp commented Sep 19, 2019

pabigot commented Sep 19, 2019

mnkp commented Sep 22, 2019

pabigot commented Sep 22, 2019

mnkp Sep 25, 2019

pabigot Sep 25, 2019

mnkp Sep 26, 2019

mnkp Sep 25, 2019

pabigot Sep 25, 2019

mnkp Sep 26, 2019

mnkp Sep 25, 2019

mnkp Sep 25, 2019

pabigot Sep 25, 2019

mnkp Sep 26, 2019

mnkp Sep 25, 2019

pabigot commented Sep 26, 2019

[TOPIC-GPIO] miscellaneous API documentation updates #19252

[TOPIC-GPIO] miscellaneous API documentation updates #19252

Conversation

pabigot commented Sep 18, 2019

pabigot commented Sep 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

galak left a comment

Choose a reason for hiding this comment

pabigot commented Sep 18, 2019

carlescufi commented Sep 18, 2019

galak commented Sep 18, 2019

carlescufi commented Sep 19, 2019

pabigot commented Sep 19, 2019

galak commented Sep 19, 2019

mnkp commented Sep 19, 2019

pabigot commented Sep 19, 2019 • edited Loading

mnkp commented Sep 19, 2019

pabigot commented Sep 19, 2019

mnkp commented Sep 22, 2019

pabigot commented Sep 22, 2019

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

pabigot commented Sep 26, 2019

pabigot commented Sep 19, 2019 •

edited

Loading