Skip to content
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

Fix ULP FSM register macros with addr[9:0] > 0xFF (IDFGH-10397) #11652

Merged
merged 1 commit into from
Jun 16, 2023

Conversation

boarchuz
Copy link
Contributor

The following was prematurely constraining reg to 8 bits, effectively masking off addr[7:6]:
.addr = (reg & 0xff) / sizeof(uint32_t),
This was an issue when reading from or writing to registers with bits[9:8] != 0.

For example, on S3 operations with the following register:
#define RTC_CNTL_TOUCH_DAC1_REG (DR_REG_RTCCNTL_BASE + 0x150)
MIstakenly interact with the following register instead (as 0x150 & 0xFF == 0x50):
#define RTC_CNTL_STORE0_REG (DR_REG_RTCCNTL_BASE + 0x50)

Fixed by applying the bitmask after the division, instead of before.

@CLAassistant
Copy link

CLAassistant commented Jun 12, 2023

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title Fix ULP FSM register macros with addr[9:0] > 0xFF Fix ULP FSM register macros with addr[9:0] > 0xFF (IDFGH-10397) Jun 12, 2023
@espressif-bot espressif-bot added the Status: Opened Issue is new label Jun 12, 2023
@sudeep-mohanty sudeep-mohanty self-assigned this Jun 14, 2023
@espressif-bot espressif-bot added Status: Selected for Development Issue is selected for development and removed Status: Opened Issue is new labels Jun 14, 2023
@sudeep-mohanty
Copy link
Collaborator

Hello @boarchuz,
Thank you for noticing the bug and fixing it. Changes LGTM! I shall be proceeding to merge the PR to our internal repository for further sanity tests. Thanks!

@sudeep-mohanty
Copy link
Collaborator

sha=07332abbaad63a74b22a6641488d9f6db000a2e2

@sudeep-mohanty sudeep-mohanty self-requested a review June 14, 2023 15:34
@sudeep-mohanty sudeep-mohanty removed their assignment Jun 14, 2023
@sudeep-mohanty sudeep-mohanty added the PR-Sync-Merge Pull request sync as merge commit label Jun 14, 2023
@sudeep-mohanty sudeep-mohanty self-assigned this Jun 14, 2023
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: NA Issue resolution is unavailable and removed Status: Selected for Development Issue is selected for development labels Jun 15, 2023
@espressif-bot espressif-bot merged commit 98bc3d7 into espressif:master Jun 16, 2023
wnienhaus added a commit to wnienhaus/micropython-esp32-ulp that referenced this pull request Aug 30, 2023
This was already incorrect in the original ESP32 implementation
but was discovered while testing the new S2/S3 implementation.

This was also wrong within the ESP-IDF, that we based the translation
logic on. Espressif fixed the issue in this pull request:
espressif/esp-idf#11652

We now also have unit tests and compat (integration) tests, that
compare our binary output against that of binutils-gdb/esp32-ulp-as,
which already did this translation correctly, but we didnt have a
test for the specific cases we handled incorrectly, so we didn't
notice this bug.

This fix has also been tested on a real device, because S2/S3 devices
need the IOMUX clock enabled in order to be able to read GPIO input
from the ULP, and enabling that clock required writing to a register
in the SENS address range, which didnt work correctly before this fix.
wnienhaus added a commit to wnienhaus/micropython-esp32-ulp that referenced this pull request Aug 30, 2023
This was already incorrect in the original ESP32 implementation
but was discovered while testing the new S2/S3 implementation.

This was also wrong within the ESP-IDF, that we based the translation
logic on. Espressif fixed the issue in this pull request:
espressif/esp-idf#11652

We now also have unit tests and compat (integration) tests, that
compare our binary output against that of binutils-gdb/esp32-ulp-as,
which already did this translation correctly, but we didnt have a
test for the specific cases we handled incorrectly, so we didn't
notice this bug.

This fix has also been tested on a real device, because S2/S3 devices
need the IOMUX clock enabled in order to be able to read GPIO input
from the ULP, and enabling that clock required writing to a register
in the SENS address range, which didnt work correctly before this fix.
wnienhaus added a commit to wnienhaus/micropython-esp32-ulp that referenced this pull request Aug 30, 2023
This was already incorrect in the original ESP32 implementation
but was discovered while testing the new S2/S3 implementation.

This was also wrong within the ESP-IDF, that we based the translation
logic on. Espressif fixed the issue in this pull request:
espressif/esp-idf#11652

We now also have unit tests and compat (integration) tests, that
compare our binary output against that of binutils-gdb/esp32-ulp-as,
which already did this translation correctly, but we didnt have a
test for the specific cases we handled incorrectly, so we didn't
notice this bug.

This fix has also been tested on a real device, because S2/S3 devices
need the IOMUX clock enabled in order to be able to read GPIO input
from the ULP, and enabling that clock required writing to a register
in the SENS address range, which didnt work correctly before this fix.
wnienhaus added a commit to wnienhaus/micropython-esp32-ulp that referenced this pull request Aug 31, 2023
This was already incorrect in the original ESP32 implementation
but was discovered while testing the new S2/S3 implementation.

This was also wrong within the ESP-IDF, that we based the translation
logic on. Espressif fixed the issue in this pull request:
espressif/esp-idf#11652

We now also have unit tests and compat (integration) tests, that
compare our binary output against that of binutils-gdb/esp32-ulp-as,
which already did this translation correctly, but we didnt have a
test for the specific cases we handled incorrectly, so we didn't
notice this bug.

This fix has also been tested on a real device, because S2/S3 devices
need the IOMUX clock enabled in order to be able to read GPIO input
from the ULP, and enabling that clock required writing to a register
in the SENS address range, which didnt work correctly before this fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-Sync-Merge Pull request sync as merge commit Resolution: NA Issue resolution is unavailable Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants