syslog/ramlog: add support of ramlog flush worker #13599
+184
−12
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
Area: Documentation
|
[Experimental Bot, please feedback here] The PR does not fully meet the NuttX requirements, specifically in the following areas: Summary:
Impact:
Testing:
Recommendations for Improvement:
By addressing these points, the PR will better adhere to the NuttX requirements and provide reviewers with the necessary context for a comprehensive evaluation. |
1. add RAMLOG_FLUSH options to support flush ramlog buffer to ARCH_LOWPUTC device 2. add RAMLOG_FLUSH_WORKER to support background ramlog flush from workqueue in timeout RAMLOG_FLUSH_WORKER_TIMEOUT_MS Signed-off-by: chao an <[email protected]>
github-actions
bot
removed
Area: Documentation
|
|
||
| while (pos != (header->rl_buffer + end) && *pos++ != '\n'); | ||
|
|
||
| up_nputs(start, pos - start); |
There was a problem hiding this comment.
why need dup to up_nputs? which may generate the infinite recursion.
| ---help--- | ||
| ROMLOG lower flush worker to flush the ramlog to lower half driver. | ||
|
|
||
| if RAMLOG_FLUSH_WORKER |
There was a problem hiding this comment.
change RAMLOG_FLUSH_WORKER_TIMEOUT_MS to depend on RAMLOG_FLUSH_WORKER
| priv->rl_tail = header->rl_head - priv->rl_bufsize; | ||
| } | ||
|
|
||
| begin = priv->rl_tail % priv->rl_bufsize; |
There was a problem hiding this comment.
change begin/end to tail/head
| start = header->rl_buffer + begin; | ||
| pos = start; | ||
|
|
||
| while (pos != (header->rl_buffer + end) && *pos++ != '\n'); |
There was a problem hiding this comment.
why need find \n? let's up_nputs handle multiple \n to \r\n byself.
|
|
||
| flags = enter_critical_section(); | ||
|
|
||
| if (header->rl_head != priv->rl_tail) |
There was a problem hiding this comment.
why not flush all buffer in once
| } | ||
| else | ||
| { | ||
| if (work_available(&priv->rl_work)) |
There was a problem hiding this comment.
merge with line 454
| #ifdef CONFIG_RAMLOG_FLUSH | ||
| int ramlog_flush(FAR syslog_channel_t *channel) | ||
| { | ||
| ramlog_flush_internal(&g_sysdev, true); |
There was a problem hiding this comment.
why not merge ramlog_flush_internal into ramlog_flush?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
syslog/ramlog: add support of ramlog flush worker
RAMLOG_FLUSHoptions to support flush ramlog buffer toARCH_LOWPUTCdeviceRAMLOG_FLUSH_WORKERto support background ramlog flush from workqueuein timeout
RAMLOG_FLUSH_WORKER_TIMEOUT_MSSigned-off-by: chao an [email protected]
Impact
N/A
Testing
ci-check