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

Use lib_get_tempbuffer Saving stack #2946

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Zhangshoukui
Copy link
Contributor

Summary

Use lib_get_tempbuffer Saving stack

Impact

Saves the consumption of stack space

Testing

./tools/configure.sh -l sim:nsh
make -j5

@nuttxpr
Copy link

nuttxpr commented Jan 15, 2025

[Experimental Bot, please feedback here]

No, this PR description does not fully meet the NuttX requirements. Here's why and how to fix it:

Missing Information in Summary:

  • Why is the change necessary? Simply stating "Saving stack" isn't enough. Explain the problem the change solves. Is there a stack overflow issue? Is this a preventative measure for potential future issues? Quantify the savings if possible.
  • What functional part of the code is being changed? Be specific. Which files/functions are using lib_get_tempbuffer now that weren't before?
  • How does the change exactly work? Explain the mechanism. Does it replace statically allocated buffers? How is memory managed?

Missing Information in Impact:

  • Is new feature added? Is existing feature changed? Answer these explicitly.
  • While you mention saving stack space, you need to answer all the other impact questions (build, hardware, documentation, security, compatibility) with explicit YES/NO and descriptions where applicable. Even if the answer is NO, state it explicitly.

Missing Information in Testing:

  • Build Host(s): Provide details about your build host environment. e.g., "Linux, x86_64, GCC 12.1"
  • Target(s): Be more specific than "sim:nsh." Which simulator? Which configuration within nsh?
  • Testing logs before change: You've provided no logs. Show the output demonstrating the problem before your change. For example, perhaps there's a stack overflow error message or you've used a tool to measure stack usage.
  • Testing logs after change: You've provided no logs. Show the output after your change demonstrating the improvement. Again, an error message disappearing or reduced stack usage metrics would be helpful. Show the output of the same test commands you used before the change to allow for direct comparison.

Example of an improved PR description:

Summary

This change addresses potential stack overflow issues by utilizing lib_get_tempbuffer for temporary buffers in the xyz and abc functions within the drivers/networking/foo directory. Previously, these functions used large, statically allocated buffers on the stack, consuming up to N bytes. This change moves the buffer allocation to the heap, reducing stack usage and improving system stability. This addresses NuttX Issue #XXX (if applicable).

Impact

  • Is new feature added? NO
  • Is existing feature changed? YES. The memory management strategy for temporary buffers in the foo driver is changed.
  • Impact on user: NO
  • Impact on build: NO
  • Impact on hardware: NO
  • Impact on documentation: YES. The drivers/networking/foo/README.md file has been updated to reflect the new memory management approach.
  • Impact on security: NO
  • Impact on compatibility: NO
  • Anything else to consider? This change might slightly increase heap memory usage.

Testing

I confirm that changes are verified on local setup and works as intended:

  • Build Host(s): Linux, x86_64, GCC 12.1
  • Target(s): sim:qemu-x86_64, nsh configuration xyz

Testing logs before change:

$ ./my_stack_test_app
Stack usage: 95%
ERROR: Stack overflow detected!

Testing logs after change:

$ ./my_stack_test_app
Stack usage: 20%
Test passed.

examples/lp503x/lp503x_main.c Outdated Show resolved Hide resolved
nshlib/nsh_parse.c Outdated Show resolved Hide resolved
system/taskset/taskset.c Outdated Show resolved Hide resolved
examples/lp503x/lp503x_main.c Outdated Show resolved Hide resolved
nshlib/nsh_timcmds.c Outdated Show resolved Hide resolved
system/nxrecorder/nxrecorder_main.c Outdated Show resolved Hide resolved
system/nxplayer/nxplayer_main.c Outdated Show resolved Hide resolved
system/nxlooper/nxlooper_main.c Outdated Show resolved Hide resolved
system/nxcamera/nxcamera_main.c Outdated Show resolved Hide resolved
netutils/rexec/rexec.c Outdated Show resolved Hide resolved
system/trace/trace.c Outdated Show resolved Hide resolved
nshlib/nsh_mmcmds.c Show resolved Hide resolved
nshlib/nsh_mmcmds.c Show resolved Hide resolved
nshlib/nsh_mmcmds.c Show resolved Hide resolved
nshlib/nsh_mmcmds.c Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants