Skip to content

cpu/esp32: fix of gpio_read for output ports#11950

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
gschorcht:cpu/esp32/periph-gpio_read-fix
Sep 24, 2019
Merged

cpu/esp32: fix of gpio_read for output ports#11950
miri64 merged 2 commits intoRIOT-OS:masterfrom
gschorcht:cpu/esp32/periph-gpio_read-fix

Conversation

@gschorcht
Copy link
Copy Markdown
Contributor

@gschorcht gschorcht commented Aug 1, 2019

Contribution description

Although it isn't explicitly specified in API, gpio_read should return the last written output value for output ports. Since the handling of inputs and outputs is strictly separated by several registers in ESP32, gpio_read returned always the initial value of the input register. Therefore, gpio_read always returned 0 for output ports.

This PR fixes this problem. For this purpose, a case distinction is made in gpio_read. While for input ports the real value are read from the ESP32 input register, the last written value for the output port is read from the ESP32 output register.

Testing procedure

Flash tests/periph_gpio with to ESP32 board, for example:

make BOARD=esp32-wroom-32 -C tests/periph_gpio flash term

Use the following commands:

> init_out 0 4
> read 0 4
GPIO_PIN(0.04) is LOW
> set 0 4
> read 0 4
GPIO_PIN(0.04) is HIGH
> clear 0 4
> read 0 4
GPIO_PIN(0.04) is LOW

Without this fix read 0 4 would always return GPIO_PIN(0.04) is LOW.

Issues/PRs references

@gschorcht
boards/esp32: corrected comment
607d9c6 
While testing the Arduino example for ESP32 boards, the problem of gpio_read for output ports was figured out. During these tests, also the wrong comment has been fixed.
@gschorcht
cpu/esp32: fix of gpio_read for output ports
9772562 
Although it isn't explicitly specified in API, gpio_read should return the last written output value for output ports. Since the handling of inputs and outputs is strictly separated by several registers in ESP32, gpio_read returned always the initial value of the input register. Therefore, a case distinction had to make. While for input ports the real value has to be read from the input register, the last written value for the output port has to be read from the output register.
@gschorcht gschorcht added Area: cpu Area: CPU/MCU ports Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 1, 2019
@gschorcht gschorcht added this to the Release 2019.10 milestone Sep 5, 2019
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 12, 2019
@kb2ma
Copy link
Copy Markdown
Member

kb2ma commented Sep 21, 2019

@gschorcht would like to see this merged for 2019.10, but we need someone to confirm the fix. @yegorich, did you actually test this?

@gschorcht
Copy link
Copy Markdown
Contributor Author

gschorcht commented Sep 21, 2019

@gschorcht would like to see this merged for 2019.10, but we need someone to confirm the fix.

Since it is a fix of a real bug, I would say yes.

@yegorich
Copy link
Copy Markdown
Contributor

yegorich commented Sep 23, 2019 •
edited
Loading

I've tested this PR on a TTGO T-Beam board (#11418). And everything is working as expected.

@kb2ma
Copy link
Copy Markdown
Member

kb2ma commented Sep 23, 2019

Thanks for testing, @yegorich! Let me see if I can motivate a maintainer to inspect the code.

@leandrolanzieri leandrolanzieri added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines labels Sep 24, 2019
leandrolanzieri
leandrolanzieri approved these changes Sep 24, 2019
View reviewed changes
Copy link
Copy Markdown
Contributor

@leandrolanzieri leandrolanzieri left a comment

Choose a reason for hiding this comment

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

Changes make sense. I tested this on the esp32-wroom-32, both for pins below and above number 32, inputs and outputs, and works as expected. ACK!

@leandrolanzieri leandrolanzieri added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Sep 24, 2019
@leandrolanzieri
Copy link
Copy Markdown
Contributor

leandrolanzieri commented Sep 24, 2019

Rebuilding once more just in case

@leandrolanzieri leandrolanzieri added Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines labels Sep 24, 2019
@miri64 miri64 added the Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines label Sep 24, 2019
@miri64 miri64 merged commit 1598c8c into RIOT-OS:master Sep 24, 2019
@gschorcht
Copy link
Copy Markdown
Contributor Author

gschorcht commented Sep 25, 2019

Thank you all for testing and merging.

@gschorcht gschorcht deleted the cpu/esp32/periph-gpio_read-fix branch September 25, 2019 05:35
@gschorcht gschorcht mentioned this pull request Nov 22, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandrolanzieri leandrolanzieri leandrolanzieri approved these changes

+1 more reviewer

@yegorich yegorich yegorich approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 3-testing The PR was tested according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.10

Development

Successfully merging this pull request may close these issues.

6 participants

@gschorcht @kb2ma @yegorich @leandrolanzieri @miri64 @benpicco