Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sensor_rpmsg.c:Fix that "stub" will be created when local core only has subscribers. #13539

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Otpvondoiats
Copy link

@Otpvondoiats Otpvondoiats commented Sep 19, 2024

Summary

Bug fix, After the ap core subscribes to the 25hz batch 120ms data of the sensor core, it switches to the cp core and subscribes to the 50hz batch 120ms sensor core data. At this time, the data subscribed by the AP will be abnormally lost (12.5hz). Mainly because ap subscribes to sensor core data earlier than cp. The subscription of cp will be broadcast to the ap core at the same time, and the corresponding "path" already exists under "/dev/uorb" of ap. Thus the ap will create a "stub".
There are some restrictions on the creation of "stub" to avoid this situation.

Impact

Cross-core communication and persistent topics

Testing

  • build
    PC:ubuntu2004, X86,SIM
    rpserver and rpproxy open uorb config
./build.sh nuttx/boards/sim/sim/sim/configs/rpserver -j16
./build.sh nuttx/boards/sim/sim/sim/configs/rpproxy -j16
  • order:
  1. rpserver
  2. rpproxy

@lupyuen
Copy link
Member

lupyuen commented Sep 19, 2024

[Experimental Bot, please feedback here]

Yes, the PR generally meets the NuttX requirements, but could use some improvements:

Strengths:

  • Summary: Clearly describes the bug, its root cause, and the proposed fix.
  • Impact: Identifies the relevant areas affected (cross-core communication, persistent topics).
  • Testing: Provides build environment details and test procedure.

Areas for Improvement:

  • Impact:
    • User Impact: Will this fix change any user-facing behavior? Be explicit (YES/NO) and describe if necessary.
    • Build Impact: Be explicit (YES/NO). If YES, explain any required build process changes.
    • Hardware Impact: Be explicit (YES/NO). If YES, specify affected architectures, boards, or drivers.
    • Documentation Impact: Be explicit (YES/NO). If YES, clarify what documentation needs updating and whether it's included in the PR.
    • Security Impact: Be explicit (YES/NO). Briefly explain any security implications.
    • Compatibility Impact: Be explicit (YES/NO) for backward, forward, and interoperability. Explain any potential issues.
  • Testing:
    • Logs: The "before" and "after" logs are placeholders. Provide actual log snippets that demonstrate the bug and its resolution. This helps reviewers understand the impact and validate the fix.

Recommendations:

  1. Expand the "Impact" section with explicit YES/NO answers and detailed descriptions where necessary.
  2. Replace the placeholder logs in the "Testing" section with real log output showing the problem before the fix and the correct behavior after.

By addressing these points, you'll provide a comprehensive and informative PR that's easier for reviewers to assess and approve.

@github-actions github-actions bot added the Size: XS The size of the change in this PR is very small label Sep 19, 2024
…has subscribers.

After the ap core subscribes to the 25hz batch 120ms data of the sensor core, it switches
to the cp core and subscribes to the 50hz batch 120ms sensor core data. At this time,
the data subscribed by the AP will be abnormally lost (12.5hz). Mainly because ap
subscribes to sensor core data earlier than cp. The subscription of cp will be broadcast
to the ap core at the same time, and the corresponding "path" already exists
under "/dev/uorb" of ap. Thus the ap will create a "stub".

There are some restrictions on the creation of "stub" to avoid this situation.

Signed-off-by: likun17 <likun17@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Size: XS The size of the change in this PR is very small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants