nuttxpr commented on PR #16012: URL: https://github.com/apache/nuttx/pull/16012#issuecomment-2732286424
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR mostly meets the NuttX requirements but needs some improvements: **Strengths:** * **Clear Summary:** The summary explains the "why" (preventing pin conflicts between board versions) and the "what" (changing default pins and adding version support). * **Impact Assessment:** The impact assessment addresses all the required points. * **Testing Provided:** Testing logs are replaced with build and run instructions, which is acceptable, especially given the hardware-specific nature of the change. The added instructions for testing with older configurations are helpful. * **Addresses a Real Problem:** Managing different hardware revisions within a single board configuration is a common challenge, and this PR seems to address it directly. **Weaknesses and Suggestions:** * **Summary (Functional Changes):** The summary should be more precise about *how* the changes work. Instead of just saying "Add version support," describe the mechanism used (e.g., Kconfig options, board-specific files). * **Impact (Hardware):** While it mentions new board version support, it should list *specifically* which pins are changed and why these changes are necessary for the new revision. Be explicit about the differences between v1.4 and v1.5. * **Testing (Reproducibility):** The testing instructions are good, but there's still room for improvement: * **Clarity:** The build command is quite long and complex. Break it down into smaller, more manageable steps with explanations for each part. Explain why `kconfig-tweak` is used instead of menuconfig. * **"Works as intended":** The "Results" section states that the LVGL demo opens and runs. This is too vague. What specific aspects of the demo were tested to verify the pin changes are correct? Did you visually inspect the output on the LCD? Did you interact with the touchscreen (if applicable)? Provide concrete evidence that the changes work *correctly*. * **Older Configurations:** The comparison of `.config` files is helpful, but explain *what* the differences mean in terms of functionality. Are there any potential regressions? * **Simplify Test for Older Boards:** Instead of `git checkout master`, consider creating a separate configuration directory for v1.4 and building against that. This would avoid checking out different branches and make the testing process cleaner. * **Testing (Logs):** While full logs aren't strictly necessary in this case, consider including snippets of relevant output (e.g., boot messages showing the pin assignments, output from the LVGL demo indicating successful initialization). **Revised Testing Section Example (Partial):** ``` ## Testing ### Building and Flashing v1.5 1. **Clean Build:** ```bash make -j distclean ``` 2. **Configure for esp32s3-lcd-ev with LVGL:** ```bash ./tools/configure.sh esp32s3-lcd-ev:lvgl ``` 3. **Select ESP32S3WROOM1N16R16V Chip:** The default configuration might select a different chip. We change it using `kconfig-tweak` because [explanation of why kconfig-tweak is preferred]. ```bash kconfig-tweak -d CONFIG_ARCH_CHIP_ESP32S3WROOM2N16R8V kconfig-tweak -e CONFIG_ARCH_CHIP_ESP32S3WROOM1N16R16V ``` 4. **Set v1.5 Pin Configuration (using kconfig-tweak):** ```bash kconfig-tweak --set-val CONFIG_ESP32S3_I2C0_SCLPIN 48 kconfig-tweak --set-val CONFIG_ESP32S3_I2C0_SDAPIN 47 kconfig-tweak --set-val CONFIG_ESP32S3_LCD_DATA6_PIN 8 kconfig-tweak --set-val CONFIG_ESP32S3_LCD_DATA7_PIN 18 ``` 5. **Update Configuration and Build:** ```bash make olddefconfig make flash ESPTOOL_BINDIR=./ ESPTOOL_PORT=/dev/ttyUSB0 -s -j$(nproc) ``` ### Running and Verifying v1.5 1. **Connect via minicom:** ```bash minicom ``` 2. **Run lvgldemo:** ``` lvgldemo ``` 3. **Verification:** * Observed [specific visual element on LCD at expected location]. This confirms that the LCD data pins are configured correctly. * [Description of interaction with touchscreen and expected response]. This confirms correct touch functionality. [Include a photo or short video if possible]. * [Other specific tests performed and observed results]. ### Building and Verifying v1.4 [Similar steps as above, but using a separate configuration directory for v1.4 to avoid branch switching. Describe specific tests and expected results. Highlight any differences in behavior compared to v1.5.] ``` By addressing these points, the PR will be significantly stronger and easier for reviewers to evaluate. Remember to always focus on clarity, reproducibility, and demonstrating that the changes achieve the intended goal without introducing regressions. -- 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
