ulp_riscv: prioritize error bits over data flags in I2C interrupt wait (IDFGH-17733)

[AbanoubSalah]

Description

I have verified and compiled a structural fix for issue #18628 on the active codebase.

In components/ulp/ulp_riscv/ulp_core/ulp_riscv_i2c.c, the ulp_riscv_i2c_wait_for_interrupt function uses an else if block that prioritizes success flags (RX_DATA_INT_ST / TX_DATA_INT_ST) over critical hardware error flags. When an external I2C slave issues a NAK specifically during the data transmission phase, both the error bits and the data status flags can become latched simultaneously on the RTC peripheral status register.

Because of the current conditional routing, the success condition takes priority, skipping the error evaluation block entirely and returning 0 (Success) to the caller. This masks the data NAK, causes the calling iteration loop to continuously advance memory buffer pointers into unallocated memory spaces, and leads to the exact cascading failure (48 retries, 64 zero-addresses, random bus data corruption, and an eventual Hardware Watchdog Timer timeout reset) described in the report.

This patch refactors the conditional priority layout to evaluate hardware error statuses (ACK Errors, Timeouts, Arbitration Loss) first. If any hardware channel error flag is raised, the function drops out immediately and returns -1, ensuring deterministic termination before memory boundaries are breached.

Related

Closes #18628

Testing

• Environment: Clean local build tree compiled against the active v6.0/v6.1-dev toolchain.
• Target Configuration: ESP32-S3 with CONFIG_ULP_COPROC_TYPE_RISCV=y explicitly enabled via sdkconfig.
• Verification: Ran idf.py fullclean followed by idf.py build inside the ULP peripheral test workspaces to guarantee zero compilation regressions, proper header directory inclusion, and clean macro-expansion.

---

Checklist

Before submitting a Pull Request, please ensure the following:

• 🚨 This PR does not introduce breaking changes.
• All CI checks (GH Actions) pass.
• Documentation is updated as needed.
• Tests are updated or added as necessary.
• Code is well-commented, especially in complex areas.
• Git history is clean — commits are squashed to the minimum necessary.

[CLAassistant]

All committers have signed the CLA.

[KonstantinKondrashov]

Hi @AbanoubSalah!
LGTM! Thank you for fixing this issue.

[KonstantinKondrashov]

sha=f98aa5de1bf30b288ff57c0abea1ce02b1497ed3

[AbanoubSalah]

Thanks for the approval! Glad I could help with this fix. It's my first contribution here!

[BitsForPeople]

Some I2C slaves will NACK the last (valid!) byte of a transaction to signal "end-of-data" to the master. Will this work on the ULP in general and with this fix?

[AbanoubSalah]

Hi, @BitsForPeople

Even if a slave device uses a non-standard NACK to signal an end-of-data during write, masking the hardware NACK flag (the legacy behavior) causes the driver to report a false success, leaving the peripheral in a faulty state, causing subsequent hang and eventually trigger the Watchdog timeout detailed in #18628.

Prioritizing the error bits makes sure that low-level driver deterministically passes the HW status to upper layers rather than silently masking a bus failure. If an application expects a terminal NACK from a unique slave, that logic is much better handled at the higher level rather than allowing the low-level ISR/wait loop to falsely report a successful transaction.

In-conclusion: Error checking is left to the application developer to decide what to do with the returned error code.