chore(color): reduce busy wait time on LCD refresh#7483
Conversation
philmoz
added
color
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe LCD refresh path now routes completion through ChangesLCD Flush Synchronization Refactor
Sequence Diagram(s)sequenceDiagram
participant startLcdRefresh
participant LTDC_IRQHandler
participant lvglFlushed
participant lv_disp_flush_ready
startLcdRefresh->>LTDC_IRQHandler: trigger LTDC reload
LTDC_IRQHandler->>LTDC_IRQHandler: BOOT or runtime branch
LTDC_IRQHandler->>lvglFlushed: runtime flush completion
lvglFlushed->>lv_disp_flush_ready: mark flush ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
tested on h7 hardware without issue I could spot |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
radio/src/targets/horus/lcd_driver.cpp (1)
169-193: 🩺 Stability & Availability | 🔴 CriticalDMA operations are asynchronous; data race when
lvglFlushed()is signaled mid-copy.
DMACopyBitmap()callsLL_DMA2D_Start()and returns immediately without waiting for the transfer to complete. In non-BOOT builds,_update_frame_buffer_addr()enablesLTDC_IT_LIbut skips the busy-wait, soLTDC_IRQHandlerfires at the next vertical blank—potentially while DMA2D is still writing to_back_buffer. When the IRQ callslvglFlushed(), it signals to LVGL that the refresh is complete before the DMA copy operations have actually finished, creating a data race on the buffer contents.Add
DMAWait()after the invalidation area loop (after line 192) to ensure all pending DMA transfers complete before the IRQ can fire.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@radio/src/targets/horus/lcd_driver.cpp` around lines 169 - 193, The issue is that `DMACopyBitmap()` initiates asynchronous DMA transfers but returns immediately, while the loop iterates through multiple invalidation areas and queues multiple DMA operations. The LTDC IRQ can fire before all DMA operations complete, causing `lvglFlushed()` to signal refresh completion while DMA is still writing to `_back_buffer`, creating a data race. After the for loop that processes `disp->inv_p` invalidation areas and calls `DMACopyBitmap()`, add a call to `DMAWait()` to ensure all pending DMA transfers finish before the IRQ handler fires and signals the buffer is ready.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@radio/src/targets/horus/lcd_driver.cpp`:
- Around line 169-193: The issue is that `DMACopyBitmap()` initiates
asynchronous DMA transfers but returns immediately, while the loop iterates
through multiple invalidation areas and queues multiple DMA operations. The LTDC
IRQ can fire before all DMA operations complete, causing `lvglFlushed()` to
signal refresh completion while DMA is still writing to `_back_buffer`, creating
a data race. After the for loop that processes `disp->inv_p` invalidation areas
and calls `DMACopyBitmap()`, add a call to `DMAWait()` to ensure all pending DMA
transfers finish before the IRQ handler fires and signals the buffer is ready.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 505ba5a8-533c-4f46-9efa-aa96027da32b
📒 Files selected for processing (18)
radio/src/boards/jumper-h750/lcd_driver.cppradio/src/boards/jumper-h750/lcd_driver.hradio/src/boards/rm-h750/lcd_driver_480.cppradio/src/boards/rm-h750/lcd_driver_480.hradio/src/boards/rm-h750/lcd_driver_800.cppradio/src/boards/rm-h750/lcd_driver_800.hradio/src/gui/colorlcd/lcd.cppradio/src/gui/colorlcd/lcd.hradio/src/gui/common/stdlcd/lcd_common.cppradio/src/gui/common/stdlcd/lcd_common.hradio/src/targets/horus/lcd_driver.cppradio/src/targets/horus/lcd_st7796s_driver.cppradio/src/targets/pa01/lcd_driver.cppradio/src/targets/pl18/lcd_driver.cppradio/src/targets/simu/simulib.cppradio/src/targets/st16/lcd_driver.cppradio/src/targets/stm32h7s78-dk/lcd_driver.cppradio/src/targets/taranis/board.h
💤 Files with no reviewable changes (2)
- radio/src/boards/rm-h750/lcd_driver_800.h
- radio/src/targets/taranis/board.h
|
Reverted to using busy wait loop for radios with inverted LCD. While the interrupt driven version works - it can result in some screen tearing while the inverted front buffer contents are being DMA copied to the back buffer. Interrupt version works on F4 radios with non-inverted LCD and H7 radios (except PA01). |
f80393d to
c83bfce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/targets/horus/lcd_driver.cpp`:
- Around line 129-133: The LTDC line interrupt can remain latched after the
handler disables it, so re-enabling LTDC_IT_LI in lcd_driver.cpp may trigger an
immediate interrupt instead of waiting for vertical blank. In the code path that
updates LTDC_Layer1->CFBAR and requests the shadow reload via LTDC->SRCR, clear
the latched line interrupt flag with the existing LTDC handle before calling
__HAL_LTDC_ENABLE_IT, using the same interrupt setup around
__HAL_LTDC_CLEAR_FLAG / __HAL_LTDC_ENABLE_IT to ensure the next interrupt fires
only after the intended reload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 26a6a050-1d6f-4187-814b-323ba66ad2fb
📒 Files selected for processing (20)
radio/src/boards/jumper-h750/lcd_driver.cppradio/src/boards/jumper-h750/lcd_driver.hradio/src/boards/rm-h750/lcd_driver_480.cppradio/src/boards/rm-h750/lcd_driver_480.hradio/src/boards/rm-h750/lcd_driver_800.cppradio/src/boards/rm-h750/lcd_driver_800.hradio/src/gui/colorlcd/boot_lcd.cppradio/src/gui/colorlcd/boot_menu.cppradio/src/gui/colorlcd/lcd.cppradio/src/gui/colorlcd/lcd.hradio/src/gui/common/stdlcd/lcd_common.cppradio/src/gui/common/stdlcd/lcd_common.hradio/src/targets/horus/lcd_driver.cppradio/src/targets/horus/lcd_st7796s_driver.cppradio/src/targets/pa01/lcd_driver.cppradio/src/targets/pl18/lcd_driver.cppradio/src/targets/simu/simulib.cppradio/src/targets/st16/lcd_driver.cppradio/src/targets/stm32h7s78-dk/lcd_driver.cppradio/src/targets/taranis/board.h
💤 Files with no reviewable changes (2)
- radio/src/targets/taranis/board.h
- radio/src/boards/rm-h750/lcd_driver_800.h
✅ Files skipped from review due to trivial changes (2)
- radio/src/gui/common/stdlcd/lcd_common.h
- radio/src/gui/common/stdlcd/lcd_common.cpp
🚧 Files skipped from review as they are similar to previous changes (11)
- radio/src/targets/stm32h7s78-dk/lcd_driver.cpp
- radio/src/targets/simu/simulib.cpp
- radio/src/targets/horus/lcd_st7796s_driver.cpp
- radio/src/boards/rm-h750/lcd_driver_480.h
- radio/src/boards/rm-h750/lcd_driver_480.cpp
- radio/src/targets/pa01/lcd_driver.cpp
- radio/src/gui/colorlcd/lcd.h
- radio/src/boards/rm-h750/lcd_driver_800.cpp
- radio/src/targets/st16/lcd_driver.cpp
- radio/src/boards/jumper-h750/lcd_driver.cpp
- radio/src/targets/pl18/lcd_driver.cpp
|
Busy wait loop removed from F4 radios with inverted LCD display. |
89822b0 to
e71bdef
Compare
| void lcdRefresh(); | ||
|
|
||
| void lcdFlushed(); | ||
| extern "C" void lvglFlushed(); |
There was a problem hiding this comment.
By the way, is there a specific reason for renaming this? That kind of "breaks" the naming convention and forces a few #if defined(SIMU).
There was a problem hiding this comment.
It wasn't renamed - 'lcdFlushed' has always been simulator only, now it's explicit.
There was a problem hiding this comment.
Ok, but that doesn't have to be something separate, right? (see #7483 (comment))
|
Separate from the LTDC discussion — a thought on the
Then extern "C" void lcdFlushed() { lv_disp_flush_ready(&disp_drv); }Net: removes |
What is the purpose of lcd_wait_cb / lcdSetWaitCb? As far as I can see it serves no real use, and removing it completely does not seem to affect the simulator at all. |
It's used to allow for a clean exit of the UI task when the simulator shuts down. Otherwise it could block on the refresh, causing the UI task to keep waiting. This is necessary in case the UI task is stopped after a frame flush but before it has been ACKed via
If you find a better way to allow for the UI task to finish cleanly, I have no problem with it. The alternatives Claude comes up with are not really better:
The implementation itself could however be made a bit simpler and better reflect the intention (UI task has been signalled to stop): static void simuLcdExitHandler(lv_disp_drv_t* disp_drv)
{
if (!task_running()) lv_disp_flush_ready(disp_drv);
} |
92cbc25 to
dc2a2e3
Compare
dc2a2e3 to
76db7b9
Compare
4 participants
Version 2 - replacement for #7448.
Moved the call to tell LVGL that it can swap buffers and continue into the LDTC vertical refresh interrupt.
Seems to solves all the issues.
Tested widgets using lcd and lvgl API's with no issues.
This removes the busy wait loop completely in the firmware (still used in bootloader).
Note: PA01 does not have a vertical refresh interrupt so still uses a busy wait loop.
Summary by CodeRabbit
#pragma once) and cleaned up conditional simulation vs firmware callback declarations.