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

syslog/rpmsg_server: fix build break if enable SYSLOG_RPMSG/SYSLOG_RPMSG_SERVER #15903

Merged
merged 1 commit into from
Feb 25, 2025

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Feb 25, 2025

Summary

syslog/rpmsg_server: fix build break if enable SYSLOG_RPMSG/SYSLOG_RPMSG_SERVER

syslog/syslog_rpmsg_server.c:66:13: error: conflicting types for 'syslog_rpmsg_write';
      have 'void(const char *, size_t,  const char *, size_t)' {aka 'void(const char *, unsigned int,  const char *, unsigned int)'}
   66 | static void syslog_rpmsg_write(FAR const char *buf1, size_t len1,
      |             ^~~~~~~~~~~~~~~~~~
In file included from syslog/syslog_rpmsg_server.c:36:
nuttx/include/nuttx/syslog/syslog_rpmsg.h:51:9: note: previous declaration of
  'syslog_rpmsg_write' with type 'ssize_t(const syslog_channel_t *, const char *, size_t)' {aka 'int(const struct syslog_channel_s *, const char *, unsigned int)'}
   51 | ssize_t syslog_rpmsg_write(FAR syslog_channel_t *channel,
      |         ^~~~~~~~~~~~~~~~~~

Signed-off-by: chao an [email protected]

Impact

N/A

Testing

enable SYSLOG_RPMSG/SYSLOG_RPMSG_SERVER

…MSG_SERVER

syslog/syslog_rpmsg_server.c:66:13: error: conflicting types for 'syslog_rpmsg_write';
      have 'void(const char *, size_t,  const char *, size_t)' {aka 'void(const char *, unsigned int,  const char *, unsigned int)'}
   66 | static void syslog_rpmsg_write(FAR const char *buf1, size_t len1,
      |             ^~~~~~~~~~~~~~~~~~
In file included from syslog/syslog_rpmsg_server.c:36:
nuttx/include/nuttx/syslog/syslog_rpmsg.h:51:9: note: previous declaration of
  'syslog_rpmsg_write' with type 'ssize_t(const syslog_channel_t *, const char *, size_t)' {aka 'int(const struct syslog_channel_s *, const char *, unsigned int)'}
   51 | ssize_t syslog_rpmsg_write(FAR syslog_channel_t *channel,
      |         ^~~~~~~~~~~~~~~~~~

Signed-off-by: chao an <[email protected]>
@github-actions github-actions bot added Area: Drivers Drivers issues Size: S The size of the change in this PR is small labels Feb 25, 2025
@nuttxpr
Copy link

nuttxpr commented Feb 25, 2025

[Experimental Bot, please feedback here]

This PR appears to mostly meet the NuttX requirements, but is missing some crucial information. Here's a breakdown:

Strengths:

  • Clear Summary of the Problem: The summary clearly identifies the issue (build break) and its cause (enabling SYSLOG_RPMSG/SYSLOG_RPMSG_SERVER). The error message provides further context.
  • Signed-off-by: Includes the Signed-off-by line, which is important for contribution acceptance.

Weaknesses:

  • Missing "How the Change Works": The summary lacks a description of how the fix addresses the build break. What code was modified and how does it resolve the conflicting types? This is critical for reviewers to understand the change.
  • Incomplete Impact Assessment: While stating "N/A" might seem sufficient if the change is truly isolated, it's better to explicitly confirm the lack of impact in each category (user, build, hardware, documentation, security, compatibility). For example, even if there's no user impact, explicitly stating "Impact on user: NO" adds clarity. The build impact is affected (it was broken, now it's fixed), so that should be stated.
  • Insufficient Testing Information: The testing section is far too brief. It only states what was enabled, not the results. The "Testing logs before change" and "Testing logs after change" sections are empty. Include actual build logs (or at least snippets showing the error before and the successful build after), and ideally also show the syslog functionality working as expected. Specify the host and target details as requested.

Recommendations for Improvement:

  1. Explain the Fix: Describe the code changes made to resolve the conflicting types. For example, "Modified syslog_rpmsg_write function signature in syslog_rpmsg_server.c to match the declaration in syslog_rpmsg.h..."
  2. Complete the Impact Assessment: Explicitly state "NO" or provide details for each impact category. Specifically mention the positive impact on the build process (it now works when these options are enabled).
  3. Provide Detailed Testing Information:
    • Fill in the "Build Host(s)" details: OS, CPU architecture, compiler version.
    • Fill in the "Target(s)" details: Architecture (e.g., simulator, real hardware), board, and configuration.
    • Include relevant snippets of the build logs before and after the change, demonstrating the error and the successful build.
    • Show evidence that syslog messages are successfully sent/received over RPMSG after the fix.

By addressing these weaknesses, the PR will be much more complete and easier for reviewers to evaluate.

@jerpelea jerpelea merged commit c782131 into apache:master Feb 25, 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 Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants