Skip to content

Conversation

@CV-Bowen
Copy link
Contributor

Summary

  1. struct rpmsg_ping_s ack change to cmd;
  2. Update the help descrption based on the new cmd meaning;

Impact

rpmsg command

Testing

sim rpserver and rpproxy

@nuttxpr
Copy link

nuttxpr commented Oct 11, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Missing Information:

  • Summary:
    • Why is the change from ack to cmd necessary? What problem does it solve?
    • How does the change to cmd work? What are the possible values of cmd and what do they signify?
  • Impact:
    • The impact section is too vague. Be specific about which features are impacted.
    • User Impact: Will users need to change their applications due to the renaming from ack to cmd?
    • Documentation: Does the documentation need to be updated to reflect the change from ack to cmd?
  • Testing:
    • Insufficient Detail: "sim rpserver and rpproxy" is not enough information. Specify the simulator used (e.g., QEMU), the architecture (e.g., ARMv7-M), and the NuttX configuration.
    • No Logs: Provide actual testing logs before and after the change. This helps demonstrate the problem being solved and that the change works as intended.

Recommendations:

  1. Elaborate on the "Why" and "How": Explain the rationale for renaming ack to cmd. Provide details about the new structure and how the cmd field is used.
  2. Specify Impact: Clearly state which features are affected and how. For example, "This change affects the rpmsg ping example application. Users will need to update their applications to use 'cmd' instead of 'ack'."
  3. Provide Detailed Testing Information: Specify the simulator, architecture, NuttX configuration, and include actual log output demonstrating the change's effectiveness.

By providing this missing information, you can significantly improve the quality of your PR and make it easier for reviewers to understand and accept your changes.

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @CV-Bowen :-)

wyr-7 and others added 2 commits October 11, 2024 11:08
1. struct rpmsg_ping_s ack change to cmd;
2. Update the help descrption based on the new cmd
   meaning;

Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>
Signed-off-by: Bowen Wang <wangbowen6@xiaomi.com>
nsh_syscmds.c: In function ‘cmd_rpmsg_once’:
nsh_syscmds.c:567:13: error: ‘RPMSGIOC_PANIC’ undeclared (first use in this function)
  567 |       cmd = RPMSGIOC_PANIC;
      |             ^~~~~~~~~~~~~~
nsh_syscmds.c:567:13: note: each undeclared identifier is reported only once for each function it appears in
nsh_syscmds.c:571:13: error: ‘RPMSGIOC_DUMP’ undeclared (first use in this function); did you mean ‘FIOC_DUMP’?
  571 |       cmd = RPMSGIOC_DUMP;

Signed-off-by: Bowen Wang <wangbowen6@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants