chore(color): reduce busy wait time on LCD refresh#7483
Open
philmoz wants to merge 4 commits into
Open
Conversation
philmoz
added
color
📝 WalkthroughWalkthroughThe LCD refresh path now uses BOOT-only reload waiting and runtime 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)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Collaborator
|
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
Collaborator
Author
|
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). |
philmoz
force-pushed
the
philmoz/reduce-busy-wait
branch
from
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
Collaborator
Author
|
Busy wait loop removed from F4 radios with inverted LCD display. |
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
2 participants
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.
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 outdated declarations.