Experimental AMP Mode (IDFGH-11693)

[FL0WL0W]

Changes needed to make AMP mode work.

Because the ESP port needs to run on 2 cores and the FREERTOS on 1 core, there became a conflict with the current usage of portNUM_PROCESSORS and configNUM_CORES. Because of this, I changed all areas looking at these definitions in relation to FREERTOS to use configNUM_CORES, and all areas of the ESP port to use portNUM_PROCESSORS. This is what i understand to be the correct ussage of these definitions.

The second core entry point is app_main1() and defaults into a delay loop. This code is found in app_startup.c. The check for making sure CONFIG_FREERTOS_UNICORE == CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE was removed since AMP is now experimentally supported.

The esp_ipc was modified in order to use crosscore_int to trigger the second core handling of ipc functions when in AMP mode. The first core still handles ipc functions through the RTOS task. Some code was rearranged to make this cohesive and readable. This consists of the bulk of the changes in this PR.

Some more CONFIG_FREERTOS_UNICORE checks were changed to CONFIG_ESP_SYSTEM_SINGLE_CORE_MODE

I tested this by building my interrupt example and have been able to demonstrate a marked improvement on Interrupt Jitter. It is now within 1us.
https://github.com/FL0WL0W/ESP32InterruptExample/tree/AMP

Relevant issues
#10410
#11830

[github-actions]

	Warnings
⚠️	Some issues found for the commit messages in this PR: the commit message "AMP Changes": summary looks empty type/action looks empty the commit message "Use configNUM_CORES for FreeRTOS configuration and portNUM_PROCESSORS for port": summary looks empty type/action looks empty the commit message "fix: Invalid configuration ESP_SYSTEM_SINGLE_CORE_MODE=y and CONFIG_FREERTOS_UNICORE=n": summary appears to be too long the commit message "fix: pre commit error with line being too long for ESP_SYSYEM_SINGLE_CORE_MODE": summary appears to be too long the commit message "fix: precommit errors": summary looks too short Please fix these commit messages - here are some basic tips: follow Conventional Commits style correct format of commit message should be: <type/action>(<scope/component>): <summary>, for example fix(esp32): Fixed startup timeout issue allowed types are: change,ci,docs,feat,fix,refactor,remove,revert,test sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter avoid Jira references in commit messages (unavailable/irrelevant for our customers) TIP: Install pre-commit hooks and run this check when committing (uses the Conventional Precommit Linter).
⚠️	Please consider squashing your 6 commits (simplifying branch history).
⚠️	The source branch "AMP" incorrect format: contains uppercase letters. This can cause troubles on case-insensitive file systems (macOS). Please rename your branch.

👋 Hello FL0WL0W, 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 ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

This GitHub project is public mirror of our internal git repository

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.

Generated by 🚫 dangerJS against 04f1c29

[mickeyl]

This is amazing. I'd really love to see this option publicly available in ESP-IDF. It would come in handy especially for situations where you have SPI-drivers and an INT line to indicate that something did happen on the peripheral.

Since it's not really advisable to use SPI from an ISR, the traditional way to handle this seems to be a high priority task that does the SPI communication which then gets woken up using direct-to-task-notification like xTaskNotifyFromISR. This way though, it can take several hundred µsec until the task actually gets woken up ­– which might be a tad bit slow for high speed peripherals.

With a bare metal loop being woken up by the IRQ (and an assorted bit-banging SPI driver), I would hope to eliminate this delay. The question then is how to communicate the payloads between the FreeRTOS core and the bare metal core.

[sudeep-mohanty]

@FL0WL0W Thank you for the contribution! The changes done for updating portNUM_PROCESSORS to configNUM_CORES LGTM. I shall be pulling it to our internal repository and running CI tests.

[FL0WL0W]

Awesome! Let me know if you need anything from me.
Thank you @sudeep-mohanty for the update!

@mickeyl The built-in ESP-IDF driver for SPI should work on the bare metal second core, but I have not tested that. It would be great if you could test it from the bare metal core. You should also be able to notify tasks in FreeRTOS from the second core.

[sudeep-mohanty]

Hello @FL0WL0W, The changes for configNUM_CORES macro replacement LGTM and I shall be pulling it in to our internal repository for tests. Unfortunately though, we won't be able to incorporate the IPC related changes as currently we don't have a mandate of maintaining an AMP compatible IDF.

[sudeep-mohanty]

sha=9cdb09c5e990ae45e0af1eaf887d3fa1a0d2dc16

[FL0WL0W]

@sudeep-mohanty I guess having some of my changes pulled is better than nothing. It is still unfortunate my other changes to the ipc will not get pulled. I will open a new pull request for some other changes that can hopefully be pulled in.

[sudeep-mohanty]

Hi @FL0WL0W,
After some more deliberations, we've decided to introduce a hidden Kconfig option, viz., CONFIG_FREERTOS_NUMBER_OF_CORES which should control the number of cores on which FreeRTOS is running. The intention is to use this new macro to iterate over SMP entities., instead of using configNUM_CORES. At the moment, this option will be tied to the value of CONFIG_FREERTOS_UNICORE but it will be made the default option for selecting the number of cores for FreeRTOS by the next major ESP-IDF release. This should not affect your efforts to enable AMP with ESP-IDF.

[mickeyl]

@sudeep-mohanty That's very good news, thanks Espressif for reconsidering!

[sudeep-mohanty]

PR updated and merged at 9605f9b. Closing this PR.

[mickeyl]

Thanks again, @FL0WL0W and @sudeep-mohanty. Going to do more experiments with this mode soon(ish). This is what puts Espressif apart from many other silicon vendors in the industry. Not only is your responsivity superb, but you're also open to 3rd party contributions when they make sense.

[FL0WL0W]

@mickeyl careful, they did not add the AMP functionality. They only updated their C definitions to match closer to what they should be. All the changes needed to the cpu_start and the ipc_task were not accepted.

@sudeep-mohanty I will look over the changes and make another PR if I see anything wrong. Thanks.

[mickeyl]

@FL0WL0W Oops, thanks for the warning. I'll stick with your fork then. The smaller the diff is the better though… and perhaps you can get more upstream eventually.