Skip to content

Conversation

@CV-Bowen
Copy link
Contributor

Summary

  • rpmsg_ping.c: fix msg data memset length
The range of memset exceeds the size of the buffer
  • rpmsg_ping.c: change msg cmd type for more compatible
Use bit mask method to represent the command, because it's more
convenient to express multiple characteristics simultaneously.
  • rpmsg_ping.c: improve log level to LOG_EMERG
Usually rpmsg ping logs are needed during testing,
but too many other module logs can easily overwrite it,
so improve the rpmsg ping log level, later we can setlogmask to
a higher log level to mask other modules' logs.
  • rpmsg_ping.c: change check data to fluctuation value
We can found the error happend packet more accurately based
on the data value.

Impact

rpmsg ping

Testing

sim:rpserver and rpproxy

wyr-7 added 4 commits October 10, 2024 12:35
We can found the error happend packet more accurately based
on the data value.

Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>
Usually rpmsg ping logs are needed during testing,
but too many other module logs can easily overwrite it,
so improve the rpmsg ping log level, later we can setlogmask to
a higher log level to mask other modules' logs.

Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>
Use bit mask method to represent the command, because it's more
convenient to express multiple characteristics simultaneously.

Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>
The range of memset exceeds the size of the buffer

Signed-off-by: Yongrong Wang <wangyongrong@xiaomi.com>
@github-actions github-actions bot added the Size: M The size of the change in this PR is medium label Oct 10, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 10, 2024

[Experimental Bot, please feedback here]

This pull request appears to meet the NuttX requirements. Here's a breakdown:

Meets Requirements:

  • Summary: Clearly describes the "why" and "what" of each change. References to related issues would strengthen it further.
  • Impact:
    • Specifies that changes affect the rpmsg_ping functionality.
    • While it implies "NO" for most impact categories, explicitly stating it would improve clarity.
  • Testing:
    • Indicates testing environments (sim:rpserver and rpproxy).
    • Providing actual testing logs, even if brief, would significantly strengthen the PR.

Recommendations for Improvement:

  • Summary: Add links to related NuttX issues if they exist.
  • Impact: Be explicit about the impact categories (e.g., "Impact on build: NO").
  • Testing: Include snippets of actual testing logs before and after the changes. This provides concrete evidence that the changes work as intended.

Overall: The PR provides a good starting point. Adding the recommended details will make it even stronger and easier for reviewers to assess.

@CV-Bowen
Copy link
Contributor Author

Need merge with PR: apache/nuttx-apps#2677

@GUIDINGLI GUIDINGLI merged commit 976c417 into apache:master Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants