nuttxpr commented on PR #15796: URL: https://github.com/apache/nuttx/pull/15796#issuecomment-2646571391
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) Yes, this PR summary meets the basic NuttX requirements, but it lacks crucial details. Here's why and how to improve it: **Missing Information & Suggested Improvements:** * **Summary:** * **Insufficient Detail on *Why*:** "Optimize" is vague. Explain *what* performance issues existed before and *why* these specific changes address them. Were there specific bottlenecks identified? Quantify the problem (e.g., "Low FPS in LVGL benchmarks due to slow SDRAM access"). * **Missing *What* (Code Changes):** Be specific. Which files were modified? Mention specific functions or data structures that were changed. Example: "Modified FMC configuration in `stm32_fmc.c` and display timings in `stm32_ltdc.c`." * **Missing *How* (Technical Explanation):** Briefly explain the technical approach. For example: "Increased SDRAM clock frequency, adjusted CAS latency, and optimized LTDC timings for reduced pixel clock cycles." * **Missing Issue References:** If this relates to any existing NuttX issues, link them here. * **Impact:** * **User Impact:** While you mention improved performance, clarify if any user configuration changes are necessary. If not, explicitly state "NO". * **Build Impact:** If only configuration changes within an existing board configuration are made, the build process *itself* likely doesn't change. So, likely "NO". However, if any new Kconfig options are introduced, explain that. * **Hardware Impact:** Specify *only* `linum-stm32h753bi`. State "NO" for all other architectures/boards. * **Documentation Impact:** If no documentation changes are needed, state "NO". If the improvements are significant, consider adding a brief note to the board's documentation. * **Security Impact:** Likely "NO". Explicitly state this. * **Compatibility Impact:** Likely "NO" unless the changes break compatibility with previous configurations or software that relies on specific timings. Be explicit. * **Testing:** * **Insufficient Build Host Details:** Provide the OS, compiler version (e.g., "Linux, GCC 12.2.1"). * **Insufficient Target Details:** You mention the board, but specify the configuration (e.g., `linum-stm32h753bi:lvgl`). * **Missing "Before" Logs:** Provide actual benchmark results *before* the changes. This is critical to demonstrate the improvement. * **Missing "After" Logs:** Provide benchmark results *after* the change. Quantify the improvement (e.g., "FPS increased from X to Y"). Don't just say "doubled" – give the actual numbers. **Example of an Improved Summary:** ``` ## Summary This PR addresses performance issues observed in LVGL applications on the linum-stm32h753bi board, specifically low FPS in benchmark demos due to inefficient memory and display timings. It optimizes the FMC configuration in `stm32_fmc.c` to improve SDRAM access speed by increasing the clock frequency and adjusting CAS latency. Additionally, it modifies the LTDC timings in `stm32_ltdc.c` to reduce pixel clock cycles. This aims to significantly improve LVGL rendering performance. ## Impact * User Impact: NO * Build Impact: NO * Hardware Impact: YES (linum-stm32h753bi board only) * Documentation Impact: NO (Consider adding a note about the performance improvements to the board documentation in a future PR). * Security Impact: NO * Compatibility Impact: NO * Anything else to consider: None ## Testing I confirm that changes are verified on local setup and works as intended: * Build Host(s): Linux, GCC 12.2.1 * Target(s): linum-stm32h753bi:lvgl Testing logs before change: ``` nsh> lvgl benchmark ... (Actual benchmark output before changes) ... ``` Testing logs after change: ``` nsh> lvgl benchmark ... (Actual benchmark output after changes, demonstrating improvement) ... ``` ``` By providing these details, reviewers can quickly understand the changes, assess their impact, and verify the testing results, leading to a smoother and faster review process. Remember to be concise but thorough. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@nuttx.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
