Skip to content

add LINE_MAX and modify lib_pathbuffer#15344

Merged
anchao merged 2 commits into
apache:masterfrom
Zhangshoukui:line_
Dec 26, 2024
Merged

add LINE_MAX and modify lib_pathbuffer#15344
anchao merged 2 commits into
apache:masterfrom
Zhangshoukui:line_

Conversation

@Zhangshoukui

Copy link
Copy Markdown
Contributor

Summary

add LINE_MAX and modify lib_pathbuffer

  1. Due to the continuous expansion of functions under the app, the 2k stack of many programs can no longer meet the requirements. If malloc is used frequently, memory fragmentation may occur, and we need an interface to optimize the use of stack and avoid memory fragmentation.

  2. There are many similar configurations in the code:
    config SYSTEM_CLE_CMD_HISTORY_LEN
    config SYSTEM_CLE_CMD_HISTORY_LINELEN
    CONFIG_NSH_LINELEN
    HELP_LINELEN
    PROCFS_LINELEN
    The future plan is to remove these configurations and use LINE_MAX

Impact

Optimize the use of stacks

Testing

local test

Signed-off-by: zhangshoukui <zhangshoukui@xiaomi.com>
…CONFIG_LINE_MAX

Signed-off-by: zhangshoukui <zhangshoukui@xiaomi.com>
@github-actions github-actions Bot added Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Dec 26, 2024
@nuttxpr

nuttxpr commented Dec 26, 2024

Copy link
Copy Markdown

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements as described. Here's why:

  • Summary: While it explains the why, it lacks clarity on what part of lib_pathbuffer is changed and how it works. Simply stating "modify lib_pathbuffer" isn't sufficient. Be explicit. Mention the specific functions or data structures modified. How does it optimize stack usage? Does it introduce a new buffer management scheme?
  • Impact: While it mentions stack optimization, it needs to address all the other points explicitly with "YES" or "NO" and provide details if "YES." For example:
    • Impact on user: YES (Users might need to adjust code that relied on previous lib_pathbuffer behavior or the removed configurations. Describe the potential changes required).
    • Impact on build: Potentially YES (if config options are removed). Describe the build changes.
    • Impact on hardware: Likely NO (but state it explicitly).
    • Impact on documentation: YES (Documentation needs updating to reflect the lib_pathbuffer changes and the removal of config options). Describe what documentation needs to be added/changed.
    • Impact on security: NO (or YES if there are any security implications).
    • Impact on compatibility: Potentially YES (if the API changes or config options are removed, it could break existing code). Describe the compatibility impact.
  • Testing: "local test" is insufficient. Provide details about your build host (OS, compiler, etc.) and target(s) (architecture, board, configuration). Include actual testing logs (before and after) demonstrating the improvement in stack usage or the correct functionality of the modified lib_pathbuffer. Show how you measured the stack usage.

In short, the PR description needs to be much more thorough and specific to meet the NuttX requirements. Provide concrete details and evidence to support your claims.

@anchao anchao merged commit ebf1ab5 into apache:master Dec 26, 2024
JianyuWang0623 added a commit to JianyuWang0623/nuttx-apps that referenced this pull request Jan 14, 2025
LINE_MAX: apache/nuttx#15344

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
xiaoxiang781216 pushed a commit to apache/nuttx-apps that referenced this pull request Jan 14, 2025
LINE_MAX: apache/nuttx#15344

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
xiaoxiang781216 pushed a commit to open-vela/nuttx-apps that referenced this pull request Jan 16, 2025
LINE_MAX: apache/nuttx#15344

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

3 participants