Repository navigation
Conversation
pthread_recv_esp32() initialised its select() timeout once, before the
for(;;) loop. On Linux, select() overwrites the timeval with the time
remaining, so after the first 100 ms with no byte from the backpack the
timeout is {0, 0} and every subsequent select() returns immediately.
The thread then spins forever, burning a whole Cortex-A7 core whenever
the ELRS backpack option is enabled (the backpack only talks on events,
so the first timeout happens right after boot).
Measured on HDZero Goggles 2 (Linux 4.9, idle main menu, elrs.enable=1):
- reader thread: 98% of one core, 85% in kernel, 0 voluntary sleeps
- whole app: 1.45 cores -> 0.54 core after the fix
- cpufreq: pinned at 1104 MHz 97% of the time -> 816 MHz 93% of the time
Re-arm the timeout on every iteration.
esp32_handler_timeout() fires the delayed DVR start/stop (record_time).
It only ran when select() timed out; the spinning loop hid that, as it
ran the handler continuously. With the thread sleeping again, a backpack
sending bytes less than 100 ms apart would hold the DVR command off, so
call the handler on every wake-up. It only compares record_time with
time().
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Problem
pthread_recv_esp32()initializes theselect()timeout only once, before its receive loop. On Linux,select()updates that timeout with the time remaining. Once the initial 100 ms has elapsed, the loop keeps passing a zero timeout and spins instead of sleeping, consuming a CPU core while the ESP32 reader is enabled.This was reproduced on HDZero Goggles 2 with the ELRS backpack enabled. The affected reader is shared by the builds in this repository.
Fix
select().esp32_handler_timeout()after each successful wake-up, including after receiving data.The second change preserves delayed DVR start/stop handling: checking the deadline only after a timeout would defer the command indefinitely while UART data arrives less than 100 ms apart. The original busy loop masked this problem.
One commit, one file; no changes to the UART protocol.
Validation
Goggles 2 hardware: an A/B measurement in our downstream firmware, at the idle main menu with
elrs.enable=1, showed the ESP32 reader falling from approximately 98% of one CPU core to approximately 0%, with about 10 wake-ups per second. In that same downstream test, whole-process CPU was observed to fall from about 1.45 cores to 0.54 core; these figures are specific to that setup and do not imply a corresponding gain in frame rate. Per-thread CPU was sampled from/proc/<pid>/task/*/statover five seconds; 375 of 400 program-counter samples before the fix were inside_newselectcalled by this reader.Those hardware measurements cover the timeout reset alone, in a firmware containing other downstream patches. They do not establish temperature or battery-life gains.
Current revision:
liblog.sowas also present on the unmodified base.clang-formatandgit diff --checkpassed during review.The exact revision including both changes has not had a dedicated on-device test of delayed DVR commands under sustained backpack traffic.