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

periph/gpio: support for extension API #9860

Closed
wants to merge 10 commits into from

Conversation

ZetaR60
Copy link
Contributor

@ZetaR60 ZetaR60 commented Aug 29, 2018

Contribution description

This is an implementation of the intercept method outlined by my proposal in #9582 (parts 1-4) for the periph/gpio interface. In short, it allows other drivers (such as GPIO expanders) to be addressed using gpio_t so that all existent code can easily gain support through those drivers. For non-implementation detail and discussion, please see #9582

At @kYc0o 's suggestion, I am breaking the implementation of #9582 / #9690 into several pieces to make it easier to review. This PR only contains the API intercept code.

Testing procedure

A test case is provided for detecting collisions between use of GPIO_PIN and GPIO_EXT_PIN. Otherwise the existing GPIO tests may be used to confirm CPU based pins work with the API change. This PR only contains API definitions for the redirection functions, so implementation of them will be tested in a follow-up PR.

Issues/PRs references

Partially replaces and closes #9190
Implements parts 1-4 of my proposal in #9582
Part of work tracked by #9690

@ZetaR60 ZetaR60 added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Aug 29, 2018
@ZetaR60 ZetaR60 added this to the Release 2018.10 milestone Aug 29, 2018
@ZetaR60 ZetaR60 requested review from kaspar030 and kYc0o August 29, 2018 19:41
@gschorcht
Copy link
Contributor

@ZetaR60 Do you think that you will be able to rebase this PR soon? The ESP32 CPU is not in the master on which your branch is based. I need it to test the periph/gpio.c changes for the ESP32.

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Nov 27, 2018

Rebased and cpu/esp* supported added.

I basically had to throw out all of the cpu/*/periph/gpio.c changes and start over because all the gpio.c files got reorganized with some recent changes. I have already done this same task several times in #9190, so it would be nice to prioritize this PR amongst the extension PRs for merging before it gets outdated again. This PR is self-contained so long as the extension interface is not enabled.

@gschorcht
Copy link
Contributor

gschorcht commented Nov 27, 2018

Great, thanks. It has to be frustrating to do the same again and again.

I would prefer that someone from the core team takes a look again (@kaspar030 @gebart @kYc0o). From my point of view, this part of the GPIO extension is OK with the only exception that I would prefer suffix _mcu instead of _ll, but this is a personal taste and I can live with it 😉

@gschorcht
Copy link
Contributor

gschorcht commented Dec 17, 2018

@ZetaR60 I'm wondering whether it would possible to define a macro that is set by the CPU peripheral configuration which defines the number of ports provided by the MCU, for example:

#define GPIO_PORT_NUMOF    (4)

In this case it would become possible to handle GPIO expansion devices based on a port ID starting with GPIO_PORT_NUMOF. In addition, the GPIO_PIN(x, y) macro could be redefined to handle MCU ports and extension ports in the same way. The extension interface would be completely invisible to the application. It always uses GPIO_PIN(port, pin), regardless of whether the GPIO is provided by the MCU or by the expansion device.

#define GPIO_PIN(x,y)       (x < GPIO_PORT_NUMOF) \
                             ? GPIO_PIN_LL(x, y) \
                             : GPIO_EXT_PIN(x - GPIO_PORT_NUMOF, y))

or even better with more consistent naming as proposed in #9582 (comment) 😉

#define GPIO_PIN(x,y)       (x < GPIO_PORT_NUMOF) \
                             ? GPIO_PIN_MCU(x, y) \
                             : GPIO_PIN_EXT(x - GPIO_PORT_NUMOF, y))

The only change to your PR would be to rename GPIO_PIN to GPIO_PIN_LL (MCU) in all cpu definitions, which shouldn't be a big thing, and to define GPIO_PORT_NUMOF for MCUs with multiple ports. There could be a default definition of 1 port.

For use by drivers or applications, nothing would change.
What are your thoughts.

@gschorcht
Copy link
Contributor

@ZetaR60 Any thoughts to my last idea to introduce a macro GPIO_PORT_NUMOF and to rename GPIO_PIN to GPIO_PIN_LL in low level drivers to be able to define a new GPIO_PIN macro which allows the seamless integration of GPIO extension devices as ports.

#define GPIO_PIN_LL(x, y) ...
#define GPIO_PORT_NUMOF (x)
#define GPIO_PIN(x,y)       (x < GPIO_PORT_NUMOF) \
                             ? GPIO_PIN_LL(x, y) \
                             : GPIO_EXT_PIN(x - GPIO_PORT_NUMOF, y))

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Dec 22, 2018

@gschorcht

Any thoughts to my last idea

Sorry for the late reply. I have not been able to be as prompt as I would like to be lately.

In this case it would become possible to handle GPIO expansion devices based on a port ID starting with GPIO_PORT_NUMOF. In addition, the GPIO_PIN(x, y) macro could be redefined to handle MCU ports and extension ports in the same way. The extension interface would be completely invisible to the application. It always uses GPIO_PIN(port, pin), regardless of whether the GPIO is provided by the MCU or by the expansion device.

The extension interface is already completely invisible, because the compiler does not "see" the macros. I don't think that getting rid of GPIO_EXT_PIN would give any advantages in increasing compatibility. I also recall that when working out how to write GPIO_EXT_PIN I had major problems on some systems because of weird gpio_t encoding methods. Not everything uses ascending port numbers, so I don't think the port encoding idea will be practical. Using the leading bit to indicate an extension device made the encoding agnostic to any MCU weirdness (as apparently no system collided with the leading bit, as determined by tests/periph_gpio_coll).

I'm wondering whether it would possible to define a macro that is set by the CPU peripheral configuration which defines the number of ports provided by the MCU

This would be a useful thing to add to the extension API follow-up macro changes that we have been discussing in #9582.

@gschorcht
Copy link
Contributor

The extension interface is already completely invisible, because the compiler does not "see" the macros.

Yes, for the compiler it is clear. I meant seamless integrated or invisible from the application developer's perspective.

I don't think that getting rid of GPIO_EXT_PIN would give any advantages in increasing compatibility.

Maybe, we have different perspectives in mind when we are talking about extensions. The application developer uses the GPIO_PIN macro either hard-coded or in configuration parameters, such as for drivers. Using GPIO_EXT_PIN instead of GPIO_PIN requires that the application developer has to know which GPIOs are provided by the MCU and which GPIOs are provided by the extension. I'm wondering, is it always necessary that the application developer has to aware that.

Let's suppose the board developer creates a board where 4 GPIO ports are used from MCU and 2 GPIO ports are provided by an GPIO extensions. He could tell the application developer in the board documentation that the board provides 6 GPIO ports with 8 pins each. The application developer would then use them from GPIO_PIN(0, x) ... GPIO_PIN(5, x). This is what I meant when I said seamless integration.

So the question is:
Do you think that from the perspective of application developers, it is always necessary to know what an MCU GPIO is and what an expansion GPIO is? If so, everything is fine the way it is. If not, we should hide GPIO_EXT_PIN a bit more from the application developer's point of view, at least optionally.

For me, as an application developer, it would be really cool not to know which GPIOs are provided by the MCU and which GPIOs by extensions.

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Dec 28, 2018

Rebased, changed *_ll to *_cpu as discussed in #9582, and backported a few of the improvements from @gschorcht 's PRs.

@ZetaR60
Copy link
Contributor Author

ZetaR60 commented Dec 28, 2018

@gschorcht When you get a chance, can you update your PRs to change *_ll to *_cpu ?

@gschorcht
Copy link
Contributor

@ZetaR60 Sure, will do it tomorrow.

@gschorcht
Copy link
Contributor

@ZetaR60 ADC, DAC and PWM extensions APIs are changed too.

drivers/include/periph/gpio.h Outdated Show resolved Hide resolved
drivers/include/periph/gpio.h Outdated Show resolved Hide resolved
Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

Blocking so noone merges accidentally.

@gschorcht
Copy link
Contributor

@kaspar030 Sorry I simply approved that my change requests have been considered. As long as it is not squashed it should not happen that someone merges accidentally.

@stale
Copy link

stale bot commented Apr 10, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you want me to ignore this issue, please mark it with the "State: don't stale" label. Thank you for your contributions.

@stale stale bot added the State: stale State: The issue / PR has no activity for >185 days label Apr 10, 2020
@stale stale bot closed this May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. State: stale State: The issue / PR has no activity for >185 days Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants