Skip to content

risc-v/espressif: Fix I2C SCL/SDA pin attribute masks. - #20164

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
Aurora-QIU0:esp_i2c_pin_attr
Sep 18, 2026
Merged

xiaoxiang781216 merged 1 commit into
apache:masterfrom
Aurora-QIU0:esp_i2c_pin_attr

Conversation

@Aurora-QIU0

Copy link
Copy Markdown
Contributor

Summary

esp_i2c.c composes the pin attribute masks passed to esp_configgpio() with the
logical OR operator instead of the bitwise OR operator:

#define SCL_PIN_ATTR (FUNCTION_2 || INPUT_PULLUP || OUTPUT_OPEN_DRAIN)
#define SDA_PIN_ATTR (FUNCTION_2 || INPUT_PULLUP || OUTPUT_OPEN_DRAIN)

Every operand is a non-zero bit field, so the expression collapses to 1 instead
of the intended mask. With the encodings in esp_gpio.h the mask must be 171 (0xab):

FUNCTION_2        (2 << FUNCTION_SHIFT) = 128
INPUT_PULLUP      (INPUT | PULLUP)      = 9
OUTPUT_OPEN_DRAIN (OUTPUT | OPEN_DRAIN) = 34

Passing 1 selects input mode only: output and open-drain stay disabled, the
pull-up is not enabled and the function field does not match, so the pin falls
back to plain GPIO function. The I2C signal never reaches the pads while the
transfer state machine still reports completion.

Every other pin attribute mask in this directory (esp_i2c_slave.c,
esp_i2c_bitbang.c, esp_spi.c, esp_twai.c) already uses the bitwise operator for
the same encodings, so esp_i2c.c is the only outlier.

Impact

  • New feature? NO
  • Impact on user? NO (only corrects pin mux for I2C)
  • Impact on build? NO
  • Impact on hardware? YES (risc-v/espressif I2C)
  • Impact on documentation? NO
  • Impact on security? NO
  • Impact on compatibility? NO

Testing

This fix was validated on real hardware during an ESP32-P4 board bring-up.
The logical-OR mask left the I2C pins floating (bus never ACKed); after
switching to bitwise OR the bus enumerated correctly. (Hardware logs available
on request; the board was an ESP32-P4 Function EV Board running NuttX.)

@github-actions github-actions Bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: XS The size of the change in this PR is very small labels Sep 16, 2026
@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown

MemBrowse Memory Report

No memory changes detected for:

@fdcavalcanti

Copy link
Copy Markdown
Contributor

Nice catch! Please fix issues CI is complaining about.

esp_i2c.c composes the pin attribute masks handed to esp_configgpio()
using the logical OR operator instead of the bitwise OR operator:

    #define SCL_PIN_ATTR (FUNCTION_2 || INPUT_PULLUP || OUTPUT_OPEN_DRAIN)
    #define SDA_PIN_ATTR (FUNCTION_2 || INPUT_PULLUP || OUTPUT_OPEN_DRAIN)

Every operand is a non-zero bit field, so the expression collapses to 1
rather than to the intended combination.  With the encodings defined in
esp_gpio.h the mask must be 171 (0xab):

    FUNCTION_2        (2 << FUNCTION_SHIFT) = 128
    INPUT_PULLUP      (INPUT | PULLUP)      = 9
    OUTPUT_OPEN_DRAIN (OUTPUT | OPEN_DRAIN) = 34

Passing 1 to esp_configgpio() selects input mode only: output and
open-drain remain disabled, the pull-up is not enabled and the function
field does not match, so the pin falls back to plain GPIO function.  The
I2C peripheral signal then never reaches the pads; the bus is left
floating while the transfer state machine still reports completion.

Every other pin attribute mask in this directory (esp_i2c_slave.c,
esp_i2c_bitbang.c, esp_spi.c, esp_twai.c) already uses the bitwise
operator for the same encodings, so esp_i2c.c was the only outlier.

Since this file is modified by this commit, the pre-existing nxstyle
violations reported by the check job are fixed as well, as asked in
CONTRIBUTING.md section 2.1 (adapt all modified files even if you did
not introduce the problem yourself):

* esp_i2c.c:1267      - statement over-indented inside its enclosing
                        block (8 spaces where the block body is at 6)
* esp_i2c.c:1303      - missing blank line after declarations
* esp_i2c.c:1592      - missing blank line after declarations
* esp_i2c.c:1710-1725 - 'case'/'default' labels inside switch(port)
                        sat at the same indent as the brace opening
                        the switch body; they belong one level further
                        in, with the case logic one more level in from
                        the label

Assisted-by: WorkBuddy:DeepSeek-V4.1-Flash
Signed-off-by: Aurora-QIU0 <2170685247@qq.com>
@Aurora-QIU0

Copy link
Copy Markdown
Contributor Author

The issues reported by the check job are fixed in e1c5174.

Because this PR touches esp_i2c.c, the pre-existing nxstyle violations in that
file were addressed as well, per CONTRIBUTING.md section 2.1 ("adapt all modified
files even if you did not introduce the problem yourself"):

  • 1267 - statement over-indented inside its enclosing block (8 spaces where the block body is at 6)
  • 1303, 1592 - missing blank line after declarations
  • 1710-1725 - case/default labels inside switch (port) were aligned with the brace opening the switch body instead of one level further in

Verified locally with tools/nxstyle.c (gcc 13.3, Ubuntu 24.04): before the change
the file reports 14 violations; afterwards
nxstyle arch/risc-v/src/common/espressif/esp_i2c.c exits 0 with no output.
All other ESP32-P4 / risc-v espressif files were left untouched.

The workflow runs for the new commit show action_required — would you mind
re-approving them when convenient?

@Aurora-QIU0

Copy link
Copy Markdown
Contributor Author

Testing update 鈥?build and runtime logs (ESP32-P4, real hardware)

This change was built and exercised on hardware as part of the same ESP32-P4
bring-up as PR #20165. To be precise about what the hardware evidence does and
does not show here:

  • The runtime mask measurement was taken after this change was already
    applied, and reads 171 (0xab) 鈥?i.e. it confirms the fixed value, not the
    original one. It is therefore not an isolated before/after for this hunk.
  • The value of the original expression is a compile-time fact: every operand is
    a non-zero bit field, so FUNCTION_2 || INPUT_PULLUP || OUTPUT_OPEN_DRAIN
    evaluates to 1.

So the case for this hunk rests on that static analysis plus consistency with
the sibling drivers (esp_i2c_slave.c, esp_i2c_bitbang.c, esp_spi.c,
esp_twai.c, all of which use | for the same encodings) 鈥?supported by the
fact that the corrected mask is what the board was actually running on when the
bus came up.

Build

ninja: Building C object .../espressif/esp_i2c.c.o
       -> libarch.a -> nuttx -> nuttx.bin
nuttx.bin   797756 bytes   md5 0c4ac4e6c0df1ed695f9911a30e39c97
flash: "Wrote 797756 bytes" + "Hash of data verified" (rc=0)

Runtime (same firmware as PR #20165)

I2CP[1] init enter id=0 scl=8 sda=7 attr_scl=171 attr_sda=171
I2CP[2] gpio cfg done scl_sig out=68 in=68 sda_sig out=69 in=69
GT911 touchscreen registered at /dev/input0 (polling mode)
nsh prompt reached; 0 panic / 0 assert / 0 ERROR

@github-actions github-actions Bot added Size: S The size of the change in this PR is small and removed Size: XS The size of the change in this PR is very small labels Sep 17, 2026
@acassis

acassis commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

@Aurora-QIU0 please rebase to force the CI to get the fix apache/nuttx-apps#3790

@xiaoxiang781216
xiaoxiang781216 merged commit 637a53c into apache:master Sep 18, 2026
41 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants