You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "anchao (via GitHub)" <gi...@apache.org> on 2024/09/27 11:56:06 UTC
[PR] syslog/ramlog: remove magic flag [nuttx]
anchao opened a new pull request, #13686:
URL: https://github.com/apache/nuttx/pull/13686
## Summary
syslog/ramlog: remove magic flag
1. remove magic flag
2. move g_sysdev to bss
3. clear ramlog buffer only if in custom section
Signed-off-by: chao an <an...@lixiang.com>
## Impact
N/A
## Testing
ci-check
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] syslog/ramlog: remove magic flag [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #13686:
URL: https://github.com/apache/nuttx/pull/13686#discussion_r1779435589
##########
drivers/syslog/ramlog.c:
##########
@@ -290,10 +274,16 @@ static ssize_t ramlog_addbuf(FAR struct ramlog_dev_s *priv,
flags = enter_critical_section();
#ifdef CONFIG_RAMLOG_SYSLOG
- if (header->rl_magic != RAMLOG_MAGIC_NUMBER && priv == &g_sysdev)
+ if (priv == &g_sysdev && header == NULL)
{
+ priv->rl_header = (FAR void *)g_sysbuffer;
Review Comment:
what's the benefit to change the static data initialization to the runtime initialization?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] syslog/ramlog: remove magic flag [nuttx]
Posted by "anchao (via GitHub)" <gi...@apache.org>.
anchao commented on PR #13686:
URL: https://github.com/apache/nuttx/pull/13686#issuecomment-2379609681
@acassis @patacongo This PR will use the `header `pointer to check if it is hot/cold reset, so removing the magic will give the same functionality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] syslog/ramlog: remove magic flag [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #13686:
URL: https://github.com/apache/nuttx/pull/13686#issuecomment-2379119346
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
This PR appears to **mostly** meet the NuttX requirements, but is missing some information:
**What's good:**
* **Summary:** Provides a high-level overview of the changes being made. However, it could be more descriptive. What is the magic flag and why should it be removed?
* **Testing:** Indicates that CI checks were performed, which is good. However, providing actual log snippets (even if shortened) would be even better.
**What needs improvement:**
* **Impact:** While marking everything as N/A might be technically correct, it's unlikely that *every* category is truly unaffected. For example:
* **Impact on build:** Even small code changes can alter build times or memory usage. Consider if this is relevant.
* **Impact on compatibility:** Does removing this magic flag potentially break any existing configurations or use cases?
* **Testing:**
* Be specific about the build host and target(s) used for testing.
* Provide snippets of the testing logs before and after the change. This helps reviewers quickly understand the impact of the changes.
**Recommendation:**
Before merging, expand on the Impact section, even if briefly, to confirm that the potential implications have been considered. Additionally, enhance the Testing section with specifics about the test environment and relevant log snippets.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] syslog/ramlog: remove magic flag [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #13686:
URL: https://github.com/apache/nuttx/pull/13686#issuecomment-2494488015
ping @anchao
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] syslog/ramlog: remove magic flag [nuttx]
Posted by "acassis (via GitHub)" <gi...@apache.org>.
acassis commented on PR #13686:
URL: https://github.com/apache/nuttx/pull/13686#issuecomment-2379358741
@anchao I think the magic flag is useful to search where the ramlog starts in the memory in case where someone is debug NuttX bringup and doesn't have serial console working.
Maybe @patacongo can confirm the reason he included it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] syslog/ramlog: remove magic flag [nuttx]
Posted by "patacongo (via GitHub)" <gi...@apache.org>.
patacongo commented on PR #13686:
URL: https://github.com/apache/nuttx/pull/13686#issuecomment-2379493177
The flag was used to distinguish between a cold start and a reset. See
```
commit 0da8755fbce536da81894694c47905430d38848b
Author: yuanyongjian <yu...@xiaomi.com>
Date: Thu Nov 23 13:02:47 2023 +0800
syslog/ramlog:Add tags to distinguish between hot start and cold start
Signed-off-by: yuanyongjian <yu...@xiaomi.com>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] syslog/ramlog: remove magic flag [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on PR #13686:
URL: https://github.com/apache/nuttx/pull/13686#issuecomment-2380549660
> @acassis @patacongo This PR will use the `header `pointer to check if it is hot/cold reset, so removing the magic will give the same functionality.
>
> 
but the check doesn't work since g_sysdev is initialized to zero by boot every time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org