[TOPIC-GPIO] drivers: gpio_esp32: update to use new GPIO API #19753

ExtremeGTX · 2019-10-10T14:29:16Z

Updates gpio driver and device tree files to the new GPIO Config flags
Implements the new port_* APIs
Fixes GPIO Bank1 config
Add esp32.overlay to gpio_basic_api test

Tests:

samples/basic/blinky
samples/basic/button
tests/drivers/gpio/gpio_basic_api
tests/drivers/gpio/gpio_api_1pin

Board:

esp32 DevKitC V4

Known issues:
part of callback_variants tests are failing

GPIO_INT_LEVEL_HIGH/LOW,
GPIO_INT_LEVEL_ACTIVE/INACTIVE
GPIO_INT_EDGE_BOTH

will continue working on it and update this PR

Signed-off-by: Mohamed ElShahawi [email protected]

ydamigos · 2019-10-10T18:39:24Z

@ExtremeGTX I quickly tested the following samples/test on odroid_go:

samples/basic/blinky -> works
samples/basic/button -> doesn't work (blue_led turns on!!!)
tests/drivers/gpio/gpio_basic_api -> fails (gpio_basic_api.log)

drivers/gpio/gpio_esp32.c

zephyrbot · 2019-10-18T16:14:19Z

All checks passed.

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

ExtremeGTX · 2019-10-18T16:20:01Z

tests/drivers/gpio/gpio_basic_api

The following tests still fails:

GPIO_INT_LEVEL_HIGH/LOW,
GPIO_INT_LEVEL_ACTIVE/INACTIVE
GPIO_INT_EDGE_BOTH

GPIO_INT_EDGE_BOTH is working in application code but test case is not working, i tested it on DevKit V4 and M5-Stack using GPIOs and using Buttons.

The problem with LEVEL interrupts is clearing the interrupt, dunno why clearing status register is not working.
it is only working in esp-idf test suite

mnkp

When the issues are fixed, could you also try to run tests/drivers/gpio/gpio_api_1pin/ testcase. It will use the pin where LED0 is connected to perform the test. The test requires pin to work in simultaneous input/output mode. According to the documentation and comments in the pinmux_esp32.c file this is supported.

drivers/gpio/gpio_esp32.c

pabigot · 2019-10-19T17:10:21Z

I think I've resolved the core problem; at least in a way that makes sense to me: you can't support both edge and level interrupts simultaneously on this hardware.

CONFIG_GPIO_ESP32_IRQ defaults to 10, which is an edge-triggered interrupt. The test currently fails on the level-triggered tests.

If I set CONFIG_GPIO_ESP32_IRQ=12 in prj.conf then the level tests pass and the edge-triggered ones fail.

There are no CPU interrupts that are listed as both edge and level triggered.

Now it's possible all that is wrong, and in fact you can use any CPU interrupt line for both edge and level interrupts, but it doesn't look that way to me.

So in https://github.com/pabigot/zephyr/commits/pr/19753 I've added a commit that detects the supported trigger type of the configured GPIO interrupt line and rejects attempts to configure for unsupported interrupt types. I also tweaked some macros/comments, and moved the interrupt acknowledge before the callback. I have verified that this does in fact clear the status register, so the fact that it didn't may be because the level configuration conflicted with the interrupt support.

@ExtremeGTX you are welcome to cherry-pick and squash that into your work. This passes the two-pin test with line 10 (edge) and line 12 (level) (or will once #19784 gets merged).

However: I cannot make the both-edge case work, so I've just disabled support for that.

mnkp · 2019-10-20T21:13:20Z

I think I've resolved the core problem; at least in a way that makes sense to me: you can't support both edge and level interrupts simultaneously on this hardware.

There are no CPU interrupts that are listed as both edge and level triggered.

Well spotted, that indeed seems to be the culprit. It is possible to attach more than one "Peripheral Interrupt Source" to the same CPU IRQ, e.g. we can have SPI and I2C modules trigger interrupt on the same IRQ line, however it doesn't seem possible to attach the same "Peripheral Interrupt Source" to two different CPU IRQ lines. In this case, the GPIO module can indeed respond either to edge or level interrupts but not both since any given IRQ line is predefined as either edge or level.

That complicates the driver. One clean way to handle this situation would be to add a function which would take IRQ line number as a parameter and return the type of the interrupt this line supports: edge, level or invalid (since some IRQs are internal to the CPU). Then the driver would need to return -ENOTSUP for the type of the interrupt which is not supported.

pabigot · 2019-10-20T22:37:50Z

One clean way to handle this situation would be to add a function which would take IRQ line number as a parameter and return the type of the interrupt this line supports: edge, level or invalid (since some IRQs are internal to the CPU). Then the driver would need to return -ENOTSUP for the type of the interrupt which is not supported.

Like this, perhaps.

mnkp · 2019-10-20T22:50:38Z

Like this, perhaps.

Looks good. But maybe we should put switch (trig) { in the else branch. The macro verifying the supported interrupt type gets resolved at the compilation stage. With the else the switch (trig) { would only get compiled in if supported.

ExtremeGTX · 2019-10-20T22:52:58Z

@pabigot Good Catch 👍
Thanks for this, but dunno why you disabled BOTH_EDGE interrupts, it works in application (i know testcase is not passing), but i tested it with an ACTIVE LOW Button and i got 2 interrupts.
One on button press and the other on button release.
did i miss anything here ?

pabigot · 2019-10-21T03:05:03Z

Looks good. But maybe we should put switch (trig) { in the else branch. The macro verifying the supported interrupt type gets resolved at the compilation stage. With the else the switch (trig) { would only get compiled in if supported.

Without the else the compiler will discard the unreachable code that would follow the unconditional return.

Thanks for this, but dunno why you disabled BOTH_EDGE interrupts, it works in application (i know testcase is not passing),

That'd be why. There may be circumstances where it works, but it's not reliable.

carlescufi · 2019-10-22T16:49:28Z

@ExtremeGTX can you cherry-pick @pabigot's commit and squash it into yours and then the PR will be ready for merging.

ExtremeGTX · 2019-10-22T19:37:55Z

@carlescufi sure, will do it tomorrow morning.
But, am still want to leave EDGE_BOTH mode enabled, since it works with normal applications.

In the end, it is up to you

pabigot · 2019-10-22T19:52:15Z

@ExtremeGTX The general position when discussed today was that if the test doesn't pass we can't enable it, because something's clearly not right. We can continue to investigate and enable it later if we can figure out what's going on. I may find some time for that, or you can look at it, but we'd really like to get the basic driver in ASAP.

ExtremeGTX · 2019-10-22T19:55:42Z

ok, i will update this PR tomorrow morning

mnkp · 2019-10-22T23:03:21Z

GPIO_INT_EDGE_BOTH is working in application code but test case is not working, i tested it on DevKit V4 and M5-Stack using GPIOs and using Buttons.

I don't know the evaluation board in question. However - unless there is some extra circuitry - signal from the button will bounce / won't be clean. It's not a reliable way to verify edge interrupts. Having passing testcase would be preferable.

ExtremeGTX · 2019-10-23T09:21:48Z

The new push includes all changes requested by @mnkp and changes done by @pabigot
dunno if i should add something in commit msg to give credits for @pabigot work.

Both tests PASS:

tests/drivers/gpio/gpio_basic_api
tests/drivers/gpio/gpio_api_1pin

pabigot

Code looks good. No need for acknowledgment for such a small change.

The commit message is a little unusual, but except for the subject line including [TOPIC-GPIO] I don't think it violates any standards. There are existing merged PRs that have that same prefix though, so it can be fixed in a cleanup pass over the branch after it's merged.

ExtremeGTX · 2019-10-23T14:27:07Z

@mnkp if you are interested, here is the code i use for testing
https://gist.github.com/ExtremeGTX/fb5a3a35696eee48ec27541b282dfb76

Pins 16,17 gpio0 (Connect both pins using a jumper) - no external circuitry required
Espressif DevkitC V4 board

mnkp

LGTM with minor comment.

drivers/gpio/gpio_esp32.c

- Updates gpio driver and device tree files to the new GPIO Config flags - Implements the new port_* APIs - Update I2C and PWM Drivers to use new GPIO config - Add esp32.overlay to gpio_basic_api test - refactor convert_int_type, regs struct - remove config_polarity - add kConfig notes Tests: - samples/basic/blinky - samples/basic/button - tests/drivers/gpio/gpio_basic_api - tests/drivers/gpio/gpio_api_1pin Board: - esp32 DevKitC V4 Note about interrupts: The ESP32 requires specifying a CPU interrupt to be used for GPIO interrupt signals. CPU interrupts can be either level or edge (or special) triggered, but not both. Please check gpio/Kconfig.esp32 for more info. Signed-off-by: Mohamed ElShahawi <[email protected]>

ExtremeGTX requested review from mnkp and pabigot as code owners October 10, 2019 14:29

ExtremeGTX force-pushed the topic-gpio_esp32 branch from 172ac81 to 8db0677 Compare October 10, 2019 14:55

ExtremeGTX requested a review from ydamigos as a code owner October 10, 2019 14:55

zephyrbot added area: PWM Pulse Width Modulation area: I2C area: Boards area: Tests Issues related to a particular existing or missing test labels Oct 10, 2019

pabigot reviewed Oct 10, 2019

View reviewed changes

drivers/gpio/gpio_esp32.c Outdated Show resolved Hide resolved

mnkp requested changes Oct 10, 2019

View reviewed changes

mnkp added the area: GPIO label Oct 11, 2019

galak mentioned this pull request Oct 12, 2019

Convert GPIO drivers to new GPIO API #18530

Closed

ExtremeGTX force-pushed the topic-gpio_esp32 branch from 8db0677 to 1bb0294 Compare October 18, 2019 16:07

mnkp requested changes Oct 18, 2019

View reviewed changes

ExtremeGTX force-pushed the topic-gpio_esp32 branch from 1bb0294 to b606129 Compare October 23, 2019 09:18

pabigot approved these changes Oct 23, 2019

View reviewed changes

mnkp self-requested a review October 24, 2019 00:33

mnkp reviewed Oct 24, 2019

View reviewed changes

drivers/gpio/gpio_esp32.c Outdated Show resolved Hide resolved

ExtremeGTX force-pushed the topic-gpio_esp32 branch from b606129 to 4a4ec61 Compare October 24, 2019 13:30

mnkp approved these changes Oct 24, 2019

View reviewed changes

galak merged commit 68c1256 into zephyrproject-rtos:topic-gpio Oct 24, 2019

ExtremeGTX deleted the topic-gpio_esp32 branch December 5, 2019 16:42

carlescufi mentioned this pull request Dec 16, 2019

Convert GPIO users to new GPIO API #20017

Closed

[TOPIC-GPIO] drivers: gpio_esp32: update to use new GPIO API #19753

[TOPIC-GPIO] drivers: gpio_esp32: update to use new GPIO API #19753

Uh oh!

Conversation

ExtremeGTX commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ydamigos commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zephyrbot commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ExtremeGTX commented Oct 18, 2019

Uh oh!

mnkp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pabigot commented Oct 19, 2019

Uh oh!

mnkp commented Oct 20, 2019

Uh oh!

pabigot commented Oct 20, 2019

Uh oh!

mnkp commented Oct 20, 2019

Uh oh!

ExtremeGTX commented Oct 20, 2019

Uh oh!

pabigot commented Oct 21, 2019

Uh oh!

carlescufi commented Oct 22, 2019

Uh oh!

ExtremeGTX commented Oct 22, 2019

Uh oh!

pabigot commented Oct 22, 2019

Uh oh!

ExtremeGTX commented Oct 22, 2019

Uh oh!

mnkp commented Oct 22, 2019

Uh oh!

ExtremeGTX commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pabigot left a comment

Choose a reason for hiding this comment

Uh oh!

ExtremeGTX commented Oct 23, 2019

Uh oh!

mnkp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ExtremeGTX commented Oct 10, 2019 •

edited

Loading

ydamigos commented Oct 10, 2019 •

edited

Loading

zephyrbot commented Oct 18, 2019 •

edited

Loading

ExtremeGTX commented Oct 23, 2019 •

edited

Loading