Fix I2C clock stretching issues on SF32LB5 #718
Conversation
Optimize GPIO interrupt handlers to prevent interrupt storms: - Change GPIO1/GPIO2 IRQ handlers to only process pending interrupts instead of looping through all 78/36 pins, which caused interrupt storms that blocked other tasks like I2C - Move NVIC configuration to exti_configure_pin (one-time setup) instead of exti_enable (called repeatedly) - Use direct register writes for enable/disable instead of |= which could have race conditions - Clear pending interrupts on disable to prevent spurious triggers Signed-off-by: Joshua Jun <joshuajun@proton.me>
The display driver performs in-place RGB222<->RGB332 pixel format conversion to save 44KB of RAM. These conversions run synchronously in the app task (priority idle+2), which has higher priority than the system task (priority idle+1) where I2C operations run. During full-screen updates (~200 rows), the conversion takes ~27ms of continuous CPU time, preventing the system task from running and causing I2C transactions to clock-stretch as the hardware times out waiting for the CPU to service interrupts. Fix by implementing chunked conversion with periodic priority drops: - Process rows in chunks of 32 - After each chunk: briefly lower app task priority to system level, yield to scheduler, then restore original priority - This creates ~6 yield points during a full conversion, giving I2C regular opportunities to run The app task is only vulnerable to preemption during brief yield points (microseconds) rather than the entire 27ms conversion time, maintaining display update performance while preventing I2C issues. Applies to both pre-conversion (222->332 before DMA) and post- conversion (332->222 after DMA) phases. Works on both Obelix and Getafix platforms which share this driver. Signed-off-by: Joshua Jun <joshuajun@proton.me> (cherry picked from commit e6aa1aa86e4985be4b00adc0d275d3991265596d) Signed-off-by: Joshua Jun <joshuajun@proton.me>
jplexer
changed the title
| break; | ||
| } | ||
|
|
||
| // Configure NVIC once during pin setup |
There was a problem hiding this comment.
open a separate PR for GPIO changes, and drop any GPIO2 code as it's not needed/used
| } | ||
|
|
||
| static void prv_display_update_terminate(void *data) { | ||
| TaskHandle_t app_task = xTaskGetCurrentTaskHandle(); |
There was a problem hiding this comment.
It looks like drivers should not be dealing with system task settings (dependency inversion).
My proposal would be:
- Best: How difficult is it to use RGB332? We're doing conversions unnecessarily just because of legacy.
- Tweak priorities in the compositor layer as a temporary solution, but (1) is the right way
I still wonder why this is a problem for I2C transfers. UI may use a lot of CPU, but I2C IRQs will still be serviced, and so I2C transfers will finish. Under which condition/sequence of events is clock stretching happening?
There was a problem hiding this comment.
Good points! Agreed on the dependency inversion issue. The display driver shouldn't be manipulating task priorities. The problem is that the driver does the conversion in-place to save RAM, so it's the only component that knows it's blocking lower-priority tasks.
On RGB332: it's difficult and breaking. We'd lose the alpha channel going from ARGB2222 to RGB332, which breaks all existing SDK apps and watchfaces since we can't recompile them. It also requires rewriting graphics blending and converting all resources. Only worth it if we wouldn't want to keep backwards compatibility.
On why clock stretching happens: the critical detail is that I2C runs in system task context at priority idle+1, not at IRQ priority. The sequence is system task calls i2c_read and blocks waiting, then app task at priority idle+2 does 27ms of pixel conversion. FreeRTOS won't preempt the higher-priority app task, so the I2C hardware times out waiting for the CPU, causing clock stretching. The I2C interrupt does fire but it just wakes the system task which can't run due to app task priority. If I2C was pure IRQ-driven you'd be right, but the FreeRTOS driver uses task context for blocking operations.
There was a problem hiding this comment.
Good points! Agreed on the dependency inversion issue. The display driver shouldn't be manipulating task priorities. The problem is that the driver does the conversion in-place to save RAM, so it's the only component that knows it's blocking lower-priority tasks.
Could compositor just swap priorities (with full update granularity)?
On RGB332: it's difficult and breaking. We'd lose the alpha channel going from ARGB2222 to RGB332, which breaks all existing SDK apps and watchfaces since we can't recompile them. It also requires rewriting graphics blending and converting all resources. Only worth it if we wouldn't want to keep backwards compatibility.
We could use e.g. RGB232, making it compatible with RGB332 (upper bit would be for alpha), or, just use RGB332 with a color that means mask/alpha. I think it's ok to have inefficiencies to run legacy apps, but, on new apps, we should be free to do whatever we want (also because Emery is in practice a new platform, more Gabbro). I'd be in favor of using a color format that HW supports, or in general, making apps agnostic of the underlying format (!= available colors), except those manipulating the framebuffer.
On why clock stretching happens: the critical detail is that I2C runs in system task context at priority idle+1, not at IRQ priority. The sequence is system task calls i2c_read and blocks waiting, then app task at priority idle+2 does 27ms of pixel conversion. FreeRTOS won't preempt the higher-priority app task, so the I2C hardware times out waiting for the CPU, causing clock stretching. The I2C interrupt does fire but it just wakes the system task which can't run due to app task priority. If I2C was pure IRQ-driven you'd be right, but the FreeRTOS driver uses task context for blocking operations.
As far as I read the code, I2C transfer is handled via IRQ. What IRQ does is just signal a ready flag so tasks waiting for the transfer to finish can be woken up and process the data. So, it should not cause clock stretching? Clock stretching happens if slave cannot keep up with master's clock speed.
2 participants
Fix two related issues causing I2C delays and watchdog warnings during heavy App activity (e.g., launcher scrolling):
Optimize GPIO IRQ handlers
Fix display updates blocking I2C