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: remove implement of syslog_putc() #14810

Merged
merged 1 commit into from
Nov 15, 2024

Conversation

anchao
Copy link
Contributor

@anchao anchao commented Nov 15, 2024

Summary

drivers/syslog: remove implement of syslog_putc()

syslog_putc() have a lot of duplicate logic with syslog_write().
remove syslog_putc() and reuse syslog_write() to simplify syslog printing.

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

Impact

N/A

Testing

ci-check

syslog_putc() have a lot of duplicate logic with syslog_write().
remove syslog_putc() and reuse syslog_write() to simplify syslog printing.

Signed-off-by: chao an <[email protected]>
@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Arch: arm Issues related to ARM (32-bit) architecture Area: Drivers Drivers issues Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Nov 15, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 15, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details.

Here's what's missing:

  • Summary: While it states what is being changed, it doesn't fully explain why. What are the benefits of removing syslog_putc() besides "simplifying"? Does it improve performance, reduce code size, or fix a bug? This is the most important part of the summary and needs to be strengthened. It also needs links to related issues if applicable.
  • Impact: Simply stating "N/A" is insufficient. Each impact item needs to be explicitly addressed with either "NO" or "YES (with explanation)". Even if there's no impact, stating "NO" for each item demonstrates that the impact has been considered. For example, are you sure there's no impact on compatibility? Could code relying on syslog_putc() break?
  • Testing: "ci-check" is not sufficient. While CI is important, the requirements specifically request testing logs before and after the change. These logs should demonstrate that the change works as intended and doesn't introduce regressions. What specific tests were run? Provide concrete examples, even if simple. Also, specify the build host and target details as requested.

Therefore, while the PR provides a starting point, it needs more information before it can be considered complete according to the provided guidelines.

@xiaoxiang781216 xiaoxiang781216 merged commit 238cddd into apache:master Nov 15, 2024
28 checks passed
@@ -84,7 +84,7 @@ const uintptr_t g_idle_topstack = HEAP_BASE;

#ifdef CONFIG_DEBUG_FEATURES
# if defined(CONFIG_ARMV7M_ITMSYSLOG)
# define showprogress(c) syslog_putc(c)
# define showprogress(c) up_putc(c)
Copy link
Contributor

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

showprocess() is only called during the startup phase, up_putc is more suitable

Copy link
Contributor

Choose a reason for hiding this comment

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

what's the point of this ifdef block then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Area: Documentation Improvements or additions to documentation Area: Drivers Drivers issues Area: OS Components OS Components 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