Optimize memory usage and add shell cmd switch #396
Conversation
📝 WalkthroughWalkthroughAdds a new Kconfig symbol to gate the USB host serial shell command and conditions compilation and export of the command and its help function on that symbol across Kconfig variants, headers, source, platform registration, and a test config header. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧬 Code graph analysis (1)class/serial/usbh_serial.h (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖
In @Kconfig.rtt:
- Around line 456-460: The dependency for CONFIG_USBHOST_SERIAL_CMD incorrectly
references USBHOST_SERIAL; update the depends on line to use the actual defined
symbol CONFIG_USBHOST_SERIAL so the dependency is evaluated correctly (i.e.,
change the dependency from USBHOST_SERIAL to CONFIG_USBHOST_SERIAL for
CONFIG_USBHOST_SERIAL_CMD).
In @Kconfig.rttpkg:
- Around line 455-459: The dependency for CONFIG_USBHOST_SERIAL_CMD is
incorrect: change the depends on symbol from USBHOST_SERIAL to
CONFIG_USBHOST_SERIAL so it matches the actual config symbol
(CONFIG_USBHOST_SERIAL) defined elsewhere; update the depends on line in the
CONFIG_USBHOST_SERIAL_CMD stanza accordingly to restore the correct dependency.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
class/serial/usbh_serial.c (1)
681-682: Remove redundantmemcpybeforesnprintf.The
memcpyon line 681 is immediately overwritten by thesnprintfon line 682, making it redundant. Thesnprintfalready copiesargv[3]and appends"\r\n", so thememcpyserves no purpose.🔎 Proposed fix
- memcpy(g_serial_testbuffer, argv[3], MIN(strlen(argv[3]), sizeof(g_serial_testbuffer))); uint32_t len = snprintf((char *)g_serial_testbuffer, sizeof(g_serial_testbuffer), "%s\r\n", argv[3]);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
KconfigKconfig.rttKconfig.rttpkgclass/serial/usbh_serial.cclass/serial/usbh_serial.hplatform/rtthread/usb_msh.c
🧰 Additional context used
🧬 Code graph analysis (1)
class/serial/usbh_serial.h (1)
class/serial/usbh_serial.c (1)
usbh_serial(637-705)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_espressif (latest)
- GitHub Check: build_bouffalolab
🔇 Additional comments (6)
Kconfig (1)
439-443: LGTM! Configuration option properly structured.The new
USBHOST_SERIAL_CMDoption is correctly defined with appropriate dependency onUSBHOST_SERIALand defaults to disabled for memory optimization.class/serial/usbh_serial.h (1)
176-178: LGTM! API visibility correctly gated.The function prototype is properly conditionally compiled based on
CONFIG_USBHOST_SERIAL_CMD, ensuring the API is only exposed when the feature is enabled.platform/rtthread/usb_msh.c (1)
48-51: LGTM! Command export correctly gated.The MSH command export now correctly requires both
CONFIG_USBHOST_SERIALandCONFIG_USBHOST_SERIAL_CMDto be enabled, properly implementing the conditional feature exposure.class/serial/usbh_serial.c (3)
621-633: LGTM! Clean conditional compilation.The conditional compilation guard and help function are well-implemented. The help text clearly documents the command-line interface.
635-635: LGTM! Proper buffer declaration.The test buffer is correctly declared with appropriate memory attributes for USB transfers.
637-705: Command implementation is solid (aside from the redundant memcpy).The shell command implementation correctly handles multiple options and maintains a persistent connection using a static variable. The validity checks on lines 647-651 appropriately handle device disconnection scenarios.
MDLZCOOL
force-pushed
the
usbh_serial_cmd_onoff
branch
from
527c968 to
53cedab
Compare
Includes: 1. Add `CONFIG_USBHOST_SERIAL_CMD` in Kconfig to allow disable/enable the `usbh_serial` command. 2. Wrap the `usbh_serial` command and `g_serial_testbuffer` with the macro to save resources when not needed. Result: - Flash: Reduced by ~0.9KB (text) + potential stdlib removal (snprintf/atoi). - RAM: Reduced by 512 Bytes (NoCache Data).
MDLZCOOL
force-pushed
the
usbh_serial_cmd_onoff
branch
from
53cedab to
762df4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @tests/hpmicro/inc/usb_config.h:
- Around line 271-274: The preprocessor check currently tests
CONFIG_USBHOST_BLUETOOTH_RX_SIZE (already defined earlier), making the block
unreachable so CONFIG_USBHOST_SERIAL_CMD is never set; update the condition to
check CONFIG_USBHOST_SERIAL_RX_SIZE instead (or remove the #ifndef so
CONFIG_USBHOST_SERIAL_CMD is always defined for CI/CD) so the macro
CONFIG_USBHOST_SERIAL_CMD gets defined as intended.
🧹 Nitpick comments (1)
class/serial/usbh_serial.c (1)
681-682: Remove redundant memcpy before snprintf.Line 681 copies
argv[3]intog_serial_testbuffer, but line 682 immediately overwrites the buffer withsnprintf. Thememcpyis unnecessary and should be removed.🔎 Proposed fix
} else if (strncmp(argv[2], "-w", 2) == 0 && argc >= 4) { - memcpy(g_serial_testbuffer, argv[3], MIN(strlen(argv[3]), sizeof(g_serial_testbuffer))); uint32_t len = snprintf((char *)g_serial_testbuffer, sizeof(g_serial_testbuffer), "%s\r\n", argv[3]); ret = usbh_serial_write(serial, g_serial_testbuffer, len);
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
KconfigKconfig.rttKconfig.rttpkgclass/serial/usbh_serial.cclass/serial/usbh_serial.hplatform/rtthread/usb_msh.ctests/hpmicro/inc/usb_config.h
🚧 Files skipped from review as they are similar to previous changes (4)
- platform/rtthread/usb_msh.c
- Kconfig
- class/serial/usbh_serial.h
- Kconfig.rtt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build_bouffalolab
- GitHub Check: build_espressif (latest)
🔇 Additional comments (2)
Kconfig.rttpkg (1)
455-460: LGTM! Dependency is correct.The dependency on
CONFIG_USBHOST_SERIALcorrectly references the symbol defined at line 412. The Kconfig syntax and structure look appropriate for gating the shell command feature.class/serial/usbh_serial.c (1)
621-706: LGTM! Command gating is properly implemented.The shell command and its help function are correctly wrapped with
CONFIG_USBHOST_SERIAL_CMD, achieving the stated goal of making this feature optional to save flash and RAM.
|
感觉不是特别有必要。没调用的话是不占用内存的 |
|
@sakumisu 同意，这个问题主要影响开启了 whole_archive 的特定构建系统，而这些平台通常 Flash 资源比较充裕，因此这个 Kconfig 开关的必要性确实不大。我先关闭这个 PR 了。 |
2 participants
Includes:
CONFIG_USBHOST_SERIAL_CMDin Kconfig to allow disable/enable theusbh_serialcommand.usbh_serialcommand andg_serial_testbufferwith the macro to save resources when not needed.Result:
Summary by CodeRabbit
New Features
Behavior Change
Tests / Platform
✏️ Tip: You can customize this high-level summary in your review settings.