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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
