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

i2c: busy wait in s_i2c_send_commands is not very power efficient. (IDFGH-12076) #13137

Closed
3 tasks done
KJ7LNW opened this issue Feb 8, 2024 · 3 comments
Closed
3 tasks done
Assignees
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally

Comments

@KJ7LNW
Copy link
Contributor

KJ7LNW commented Feb 8, 2024

Answers checklist.

  • I have read the documentation ESP-IDF Programming Guide and the issue is not addressed there.
  • I have updated my IDF branch (master or release) to the latest version and checked that the issue is present there.
  • I have searched the issue tracker for a similar issue and not found a similar issue.

General issue report

While researching #13136, I noticed that s_i2c_send_commands does a "busy wait" while waiting for the interrupt handler to (eventually) finish the i2c transfer.

Correct me if I'm wrong, but this isn't very power efficient; at low i2c clock rates, it isn't computationally efficient for other tasks, either, because it spins the CPU waiting for the transfer to complete:

while (i2c_master->trans_done == false) {};

Wouldn't this be better for power consumption?

while (i2c_master->trans_done == false) {
  esp_light_sleep_start();
};

or if FreeRTOS is involved, maybe this would be better to suspend the busy wait until the next systick?

while (i2c_master->trans_done == false) {
  taskYIELD();
};
@espressif-bot espressif-bot added the Status: Opened Issue is new label Feb 8, 2024
@github-actions github-actions bot changed the title i2c: busy wait in s_i2c_send_commands is not very power efficient. i2c: busy wait in s_i2c_send_commands is not very power efficient. (IDFGH-12076) Feb 8, 2024
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Feb 18, 2024

If FreeRTOS is always expected to be in the build for this driver, then maybe the synchronous version should suspend the task with task = xGetCurrentTaskHandle(); vTaskSuspend(NULL); and the callback should resume it with vTaskResume(task); return true; in the callback handler.

This provides not just better CPU sharing and power-usage efficiency, but an 8x performance improvement over 1kHz systicks and ~75x over 100Hz systicks. Here is a simple sync vs async benchmark:

  • async with task suspend/resume: 7506 samples/sec
  • sync with vTaskDelay(1); is limited by the systick rate, so the best it can do is ~995 samples/sec, or 99 samples/sec with the default of systick at 100Hz.

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

n.b.: I'm not sure if xGetCurrentTaskHandle() is a thing, but I found a reference to it here.

@nebkat
Copy link
Contributor

nebkat commented Mar 5, 2024

@mythbuster5 I believe this hasn't been sufficiently documented since the new driver was added.

The previous API used a FreeRTOS queue to receive messages once commands were completed, while now the API is truly CPU blocking.

I have had TWDT trigger because of this when device is offline.

KJ7LNW added a commit to KJ7LNW/esp-idf that referenced this issue Mar 6, 2024
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: espressif#13137

Signed-off-by: Eric Wheeler <[email protected]>
@KJ7LNW
Copy link
Contributor Author

KJ7LNW commented Mar 6, 2024

@nebkat, and @mythbuster5: Please see PR #13322 which would close this issue.

KJ7LNW added a commit to KJ7LNW/esp-idf that referenced this issue Mar 6, 2024
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: espressif#13137

Signed-off-by: Eric Wheeler <[email protected]>
nebkat pushed a commit to nebkat/esp-idf that referenced this issue Apr 9, 2024
@espressif-bot espressif-bot added Status: Done Issue is done internally Resolution: Done Issue is done internally and removed Status: Opened Issue is new labels Nov 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Done Issue is done internally Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants