fix(i2c_master): Do not busy wait on sync transactions (IDFGH-12280)

[KJ7LNW]

This commit introduces a private member i2c_master_bus_t::in_progress_semphr which allows i2c_master_transmit and related functions to give up their timeslice so other tasks can proceed.

Without this commit, one must delay at least one tick via vTaskDelay(1) to prevent trigging the idle watchdog, thus limiting the maximum I2C transaction rate to about the speed of CONFIG_FREERTOS_HZ.

With this commit, over 6600 samples/sec are possible and the tick delay is unnecessary.

CPU usage below was measured using vTaskGetRunTimeStats(). The FreeRTOS %CPU numbers seem to increase over time and level out after a time, so CPU usage was sampled after 5 seconds across each test for consistency across measurements. Measurements were taken with CONFIG_FREERTOS_HZ set to 100Hz and 1000Hz:

Before:

• 100Hz: samples/sec= 99 with 1% CPU ( 99.0 samples/%cpu)
• 1000Hz: samples/sec= 995 with 10% CPU ( 99.5 samples/%cpu)

After:

• 100Hz: samples/sec=6637 with 23% CPU (288.6 samples/%cpu)
• 1000Hz: samples/sec=6610 with 24% CPU (275.4 samples/%cpu)

Closes: #13137

[CLAassistant]

All committers have signed the CLA.

[github-actions]

Warnings

⚠️

Some issues found for the commit messages in this PR: the commit message "i2c_master: use notifications to wait on sync tranasctions": type/action should be one of [change, ci, docs, feat, fix, refactor, remove, revert, test] 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).

👋 Hello KJ7LNW, 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 d132c79

[KJ7LNW]

We used this simple project to test the PR change:

• https://github.com/KJ7LNW/esp32-i2c-test/tree/master

If you try the test project, be sure to comment the vTaskDelay(1) line here, or the samples/sec rate will be limited by your CONFIG_FREERTOS_HZ rate:

https://github.com/KJ7LNW/esp32-i2c-test/blob/master/main/i2c-test.c#L152

[KJ7LNW]

The force-push is a minor change: It simply sets HPTaskAwoken to true when i2c_master->trans_done == true to wake the caller sooner.

This reduces latency and increases the i2c sample rates to 6612 (1000Hz) and 6648 (100Hz).

[nebkat]

I think the original commit was better. If I2C is running on a medium priority task and the interrupt happens on a high priority task, we shouldn't yield it just to complete the I2C transaction - we must respect the priorities chosen. Especially not for a ~0.2% increase in performance.

[KJ7LNW]

@nebkat that makes sense. Reverted and force-pushed.

[mythbuster5]

Thanks a lot for this contribution. I am going to create an event queue instead of semaphore. Which can be used to store more information.

[jrahlf]

@nebkat that does not make sense to me.
Interrupts do not run on any task, they interrupt tasks. If the I2C ISR yields at the end, the scheduler runs at the end of the ISR, instead of at the end of the current time slice. Of course FreeRTOS respects the priorities of the tasks here.

[nebkat]

@jrahlf You're right my explanation was inaccurate, it would jump back to the higher priority task anyway. But we would have run the scheduler unnecessarily, so still better this way.

I have also learned pxHigherPriorityTaskWoken will only ever be set to true (it is or-ing), so there is no need for the HPTaskAwokenInProgress here

[KJ7LNW]

Out of curiosity I wondered how task notifications perform, which FreeRTOS claims to be up to 45% faster than semaphores:

Notifications:

• 100Hz: samples/sec= 6654 with 18% CPU (efficiency: 369.7 samples/%cpu)
• 1000Hz: samples/sec= 6636 with 18% CPU (efficiency: 368.7 samples/%cpu)

Semaphores: (copied from above)

• 100Hz: samples/sec=6637 with 23% CPU (efficiency: 288.6 samples/%cpu)
• 1000Hz: samples/sec=6610 with 24% CPU (efficiency: 275.4 samples/%cpu)

As you can see, notifications are faster and CPU efficiency is much higher with the task notification implementation.

I've pushed a commit converting to notifications on this PR so you can see the difference.