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

drivers/syslog: add cdcacm channel #15925

Merged
merged 1 commit into from
Mar 10, 2025
Merged

Conversation

yangsong8-a1
Copy link
Contributor

@yangsong8-a1 yangsong8-a1 commented Mar 3, 2025

Summary

add cdcacm syslog channel

Impact

syslog cdcacm

Testing

selftest

@nuttxpr
Copy link

nuttxpr commented Mar 3, 2025

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements. While it provides some information, it lacks crucial details and context required for proper review. Here's a breakdown:

  • Summary: "add cdcacm syslog channel" is insufficient. It needs to explain why this change is necessary. Is there a problem the current syslog doesn't address? What are the benefits of using CDCACM? How does it work? What code is changed? Are there related issues?

  • Impact: "syslog cdcacm" is vague. This section needs YES/NO answers for each item, followed by detailed explanations for any "YES" responses. For example:

    • Impact on user: YES (Users will now be able to redirect syslog output over USB CDCACM. This provides a new option for debugging and monitoring.)
    • Impact on build: Potentially YES (if any Kconfig options are added or modified, explain how).
    • Impact on hardware: Possibly YES (Which architectures and boards are affected? Does it require specific hardware support?)
    • Impact on documentation: YES (Documentation needs to be updated to explain how to configure and use the CDCACM syslog channel.)
  • Testing: "selftest" is inadequate. Specify the host OS, CPU architecture, compiler, and version used for testing. List the target architecture, board, and configuration. Provide actual logs demonstrating the functionality before and after the change, not just "selftest." The logs should show that the new CDCACM channel works as expected and that existing functionality is not broken.

In short, the PR needs substantially more detail and context for each section to be considered complete. It should clearly explain the motivation, implementation, impact, and testing of the change. Following the template more closely, with specific answers and details, is essential for a successful PR.

@github-actions github-actions bot added the Size: M The size of the change in this PR is medium label Mar 3, 2025
@jerpelea jerpelea changed the title syslog: add cdcacm channel driver/syslog: add cdcacm channel Mar 6, 2025
@jerpelea jerpelea changed the title driver/syslog: add cdcacm channel drivers/syslog: add cdcacm channel Mar 6, 2025
Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

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

please provide a commit message
ex

Use the CDCACM as a SYSLOG output device, send message to remote proc.
If there are more than one CDCACM devices, then a device minor number
may also need to be provided. Default: 0

Use the CDCACM as a SYSLOG output device, send message to remote proc.
If there are more than one CDCACM devices, then a device minor number
may also need to be provided. Default: 0

Signed-off-by: yangsong8 <[email protected]>
@yangsong8-a1
Copy link
Contributor Author

please provide a commit message ex

Use the CDCACM as a SYSLOG output device, send message to remote proc. If there are more than one CDCACM devices, then a device minor number may also need to be provided. Default: 0

@jerpelea done, thank you.

@lupyuen lupyuen merged commit 9c55a19 into apache:master Mar 10, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Drivers Drivers issues Area: USB 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.

6 participants