added support for enabling I2C HAL locks even when system wide HAL locks are disabled #11941
+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
requested review from
a team,
SuGlider and
me-no-dev
as code owners
|
|
👋 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 ⏱️ 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! |
@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. |
me-no-dev
reviewed
Oct 27, 2025
| 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.
this is irrelevant change that warrants its own PR with explanation
There was a problem hiding this comment.
sorry i recently push up some other fixes to this branch by accident, will rebase it
and put those in a separate PR
me-no-dev
reviewed
Oct 27, 2025
| conf.pull_down_en = GPIO_PULLDOWN_ENABLE; | ||
| } | ||
| } | ||
| if (gpio_reset_pin(pin) != ESP_OK) { |
There was a problem hiding this comment.
this is irrelevant change that warrants its own PR with explanation
There was a problem hiding this comment.
sorry i recently push up some other fixes to this branch by accident, will rebase it
and put those in a separate PR
me-no-dev
reviewed
Oct 27, 2025
| 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.
This needs to depend on hal locks being off
me-no-dev
reviewed
Oct 27, 2025
|
|
||
| #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.
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
added
the
Status: Pending CLA ⚠️
|
@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
4 participants
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.