Skip to content

Conversation

@JianyuWang0623
Copy link
Contributor

@JianyuWang0623 JianyuWang0623 commented Dec 6, 2024

Summary

Update configs about system/usbmsc.
The Kconfig of system/usbmsc has updated to support setting paths that bind to LUN at runtime.
More details: apache/nuttx-apps#2876

Impact

The boards has enabled usbmsc.

Testing

  1. Selftest with esp32s3-devkit:usbmsc, lc823450-xgevk:audio and lc823450-xgevk:usb
  2. NuttX CI

@github-actions github-actions bot added Board: arm Board: avr Board: mips Board: xtensa Size: S The size of the change in this PR is small labels Dec 6, 2024
The Kconfig of system/usbmsc has updated to support setting paths that bind to LUN at runtime.
More details: apache/nuttx-apps#2876

Signed-off-by: wangjianyu3 <wangjianyu3@xiaomi.com>
@JianyuWang0623 JianyuWang0623 force-pushed the br_wjy_boards_usbmsc_kconfigupdated_241206 branch from e814205 to 33398d3 Compare December 6, 2024 02:17
@nuttxpr
Copy link

nuttxpr commented Dec 6, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the basic NuttX requirements, but it's missing some crucial details. While it addresses the summary and testing sections reasonably well, the impact section is insufficient.

Here's a breakdown of what's missing and how to improve it:

Missing Information/Improvements Needed:

  • Impact - More Detail: Simply stating "The boards has enabled usbmsc" isn't enough. You need to specify which boards are affected. Also, address all the impact points:

    • Impact on user: Will users need to change their configuration files or scripts to use this new feature? If not, explicitly state "NO". If yes, explain the changes required.
    • Impact on build: Does this change add any new dependencies or alter the build process in any way? Even if the answer is no, explicitly state "NO".
    • Impact on hardware: Be specific about the hardware affected (e.g., "This change affects the ESP32-S3 and LC823450 boards when configured with USB MSC support").
    • Impact on documentation: Does the documentation need to be updated to reflect this change? If yes, confirm whether the PR includes those updates or if a separate documentation PR will be needed. If no, explicitly state "NO".
    • Impact on security: Consider if allowing runtime path binding for LUNs introduces any security vulnerabilities. Even if the answer is no, explicitly state "NO" and briefly justify why (e.g., "NO - No new attack surface introduced as access controls remain unchanged").
    • Impact on compatibility: Does this change break backward compatibility? Will it work with older versions of NuttX or applications? Even if the answer is no, explicitly state "NO".
  • Testing - More Detail: While the testing description is a good start, more context would be helpful.

    • Build Host(s): Specify the OS, CPU architecture, and compiler used for your local testing.
    • Testing Logs: The placeholder "your testing logs here" needs to be replaced with actual log output. Focus on showing that the new functionality works as expected (e.g., successfully binding a path to a LUN at runtime). Demonstrate both successful and unsuccessful scenarios (e.g., trying to bind to an invalid path).

Example of Improved Impact Section:

## Impact
* Is new feature added? YES.  This adds the ability to set paths that bind to LUN at runtime for the USB MSC class driver.
* Impact on user: YES. Users will need to configure the new options in their defconfig or menuconfig to specify the paths for LUN binding. Example configuration will be added to the documentation.
* Impact on build: NO.
* Impact on hardware:  This change affects ESP32-S3 and LC823450 boards when configured with USB MSC support.
* Impact on documentation: YES. Documentation will be updated to explain the new configuration options and provide usage examples.  These updates are included in this PR.
* Impact on security: NO. No new attack surface introduced as access controls remain unchanged. Runtime binding is subject to the same permissions as static configuration.
* Impact on compatibility: NO. This change maintains backward compatibility.  Existing configurations without the new options will continue to function as before.

By providing more complete information, you'll make it much easier for reviewers to understand and approve your PR.

@JianyuWang0623 JianyuWang0623 changed the title boards: Update config about system/usbmsc boards: Update configs about system/usbmsc Dec 6, 2024
@JianyuWang0623
Copy link
Contributor Author

JianyuWang0623 commented Dec 6, 2024

Link: #15063
Unrelated error

====================================================================================
Configuration/Tool: esp32-devkitc/nxdiag
2024-12-06 03:10:44
------------------------------------------------------------------------------------
  Cleaning...
  Configuring...
  Building NuttX...
fatal: could not read Username for 'https://github.com/': No such device or address
make[3]: *** [Makefile:83: espressif_prepare] Error 128
make[3]: Target 'context' not remade because of errors.
make[2]: *** [Makefile:55: /github/workspace/sources/apps/system/nxdiag_context] Error 2
make[2]: Target 'context_all' not remade because of errors.
make[1]: *** [Makefile:185: context] Error 2
make: *** [tools/Unix.mk:457: /github/workspace/sources/apps/.context] Error 2
make: Target 'all' not remade because of errors.
/github/workspace/sources/nuttx/tools/testbuild.sh: line 385: /github/workspace/sources/nuttx/../nuttx/nuttx.manifest: No such file or directory
  Normalize esp32-devkitc/nxdiag

@xiaoxiang781216 xiaoxiang781216 merged commit 0481533 into apache:master Dec 6, 2024
24 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants