-
Notifications
You must be signed in to change notification settings - Fork 7.7k
added support for enabling I2C HAL locks even when system wide HAL locks are disabled #11941
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
Open
+81
−61
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
@AlmirKadric
AlmirKadric
requested review from
a team,
SuGlider and
me-no-dev
as code owners
October 23, 2025 13:06
CLAassistant
commented
Oct 23, 2025
CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.
👋 Hello AlmirKadric, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
Test Results
76 files 76 suites 16m 43s ⏱️
38 tests 38 ✅ 0 💤 0 ❌
241 runs 241 ✅ 0 💤 0 ❌
Results for commit efdf2bf.
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Why are changes in kconfig and CmakeList.txt? This changes how the Arduino libs are compiled!
This can introduce breaking changes.
Why are changes in kconfig and CmakeList.txt? This changes how the Arduino libs are compiled! This can introduce breaking changes.
@Jason2866 because its needed to make the option configurable. Also it will not break anything as it is completely optional just like the existing system wide HAL lock disable flag.
set(srcs ${CORE_SRCS} ${ARDUINO_LIBRARIES_SRCS})
set(priv_includes cores/esp32/libb64)
set(requires spi_flash esp_partition mbedtls wpa_supplicant esp_adc esp_eth http_parser esp_ringbuf esp_driver_gptimer esp_driver_usb_serial_jtag driver esp_http_client esp_https_ota)
set(requires spi_flash esp_partition wear_levelling mbedtls wpa_supplicant esp_adc esp_eth http_parser esp_ringbuf esp_driver_gptimer esp_driver_usb_serial_jtag driver esp_http_client esp_https_ota)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is irrelevant change that warrants its own PR with explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry i recently push up some other fixes to this branch by accident, will rebase it
and put those in a separate PR
conf.pull_down_en = GPIO_PULLDOWN_ENABLE;
}
}
if (gpio_reset_pin(pin) != ESP_OK) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is irrelevant change that warrants its own PR with explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry i recently push up some other fixes to this branch by accident, will rebase it
and put those in a separate PR
and code implemented only in setup/loop and Arduino callbacks
config FORCE_I2C_HAL_LOCKS
bool "Enable mutex locks for I2C HAL even if HAL locks are disbled"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to depend on hal locks being off
#include "esp32-hal-i2c.h"
#define I2C_HAL_LOCKS_ENABLED (!CONFIG_DISABLE_HAL_LOCKS || CONFIG_FORCE_I2C_HAL_LOCKS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be defined in esp32-hal-i2c.h and propagate here and all relevant files (esp32-hal-i2c.c, Wire.h and Wire.cpp)
@me-no-dev
me-no-dev
added
the
Status: Pending CLA ⚠️
Contributor is required to sign the CLA
label
Oct 27, 2025
@AlmirKadric you need to sign the CLA for this to go further
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By completing this PR sufficiently, you help us to review this Pull Request quicker and also help improve the quality of Release Notes
Checklist
This entire section above can be deleted if all items are checked.
Description of Change
I had a use case where I want to disable HAL locks for UART and SPI busses (self-managed for performance reasons), but needed them for I2C. So implemented a way to override the behaviour for I2C (we should create something similar for other HAL lock categories as well, I'll open other PRs for this when I have the time).
Test Scenarios
Tested on ESP32S3 with arduino-esp32 v3.3.2 with multiple sensors connected over I2C and
CONFIG_DISABLE_HAL_LOCKS=y, withoutCONFIG_FORCE_I2C_HAL_LOCKS=yI get bad data over from I2C sensors. However withCONFIG_FORCE_I2C_HAL_LOCKS=ythis issue is resolved.Related links
Please provide links to related issue, PRs etc.