nuttxpr commented on PR #16315: URL: https://github.com/apache/nuttx/pull/16315#issuecomment-2851002262
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) The PR mostly meets the NuttX requirements, but could be improved in a few areas: **Strengths:** * **Clear Summary of Changes:** The summary provides a good overview of the changes made. * **Impact Assessment:** The impact section addresses most of the required points, although some could be more detailed (see below). * **Testing Procedure Provided:** The testing procedure is relatively detailed, including build commands, connection diagrams, and run commands. **Weaknesses and Suggestions for Improvement:** * **Missing Issue References:** The summary should include links to related NuttX or NuttX Apps issues if any exist. Even if there isn't a directly related issue, it's often good to link to a higher-level issue discussing the need for RS485 support if one exists. * **Impact Details:** While the impact section covers the required points, it lacks detail in some areas: * **Impact on Build:** Instead of "Current builds work fine, new option added for new feature," specify which Kconfig options were added and how they affect the build. For example, "Adds `CONFIG_ESPRESSIF_UART1_RS485` Kconfig option. Enabling this option will add RS485 support to UART1 on Espressif devices." * **Impact on Hardware:** Be more specific about *how* the serial peripheral is affected. Does it change pin mappings? Does it require different configuration settings? For instance, "This change allows the UART1 peripheral to be used in RS485 mode by controlling GPIO4 for DE and RE signals." * **Impact on Documentation:** While you say "No," consider adding a brief note to the relevant driver documentation about the new RS485 support and how to configure it. Even a small addition is valuable. * **Testing Logs:** The testing section says to include "testing logs," but you haven't provided any actual log output. Include the output from `minicom` or the logic analyzer showing the successful transmission and reception of data. This provides concrete evidence that the change works. If the logs are very long, consider summarizing them and providing a link to a full log file if necessary. * **Testing Clarity:** * The connection list is a little confusing. Instead of two separate transmitter and receiver side tables, combine them into one table showing the connections for both devices. Make it clear which GPIOs are used for TX, RX, DE, and RE on each device. * The MAX485 adapter connections section is very unclear. Explain how the two MAX485 adapters are connected to each other (A to A, B to B, etc.). A simple diagram would be very helpful here. * **"Before Change" Logs:** You've provided logs *after* the change, but you also need to provide logs *before* the change to demonstrate what the behavior was and how it has been improved. This is especially important for bug fixes. In this case, since it's a new feature, the "before" logs could simply demonstrate the lack of RS485 functionality or build errors related to its absence. By addressing these weaknesses, you'll make the PR much stronger and easier for reviewers to evaluate. A well-documented and thoroughly tested PR is more likely to be accepted quickly. -- 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
