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 LFS_DEFINES if available #15150

Merged
merged 3 commits into from
Dec 13, 2024
Merged

Use LFS_DEFINES if available #15150

merged 3 commits into from
Dec 13, 2024

Conversation

yamt
Copy link
Contributor

@yamt yamt commented Dec 12, 2024

Summary

Use LFS_DEFINES (a new lfs_util.h override mechanism from litllefs 2.10.0) if available

Bump littlefs version to 2.10.0 in a few configurations.

Impact

Testing

build tested a few combinations including

  • v2.9.3 + CONFIG_FS_LITTLEFS_HAS_LFS_DEFINES=n
  • v2.10.0 + CONFIG_FS_LITTLEFS_HAS_LFS_DEFINES=y

@github-actions github-actions bot added Area: File System File System issues Board: arm Board: xtensa Size: S The size of the change in this PR is small labels Dec 12, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 12, 2024

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

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

Missing Information in Summary:

  • Why is this change necessary? Just stating "Use LFS_DEFINES" doesn't explain the reason for the change. Is it for bug fixing, performance improvement, code cleanup, or enabling a new feature in littlefs 2.10.0? Be explicit. For example: "This change updates the littlefs integration to leverage the new LFS_DEFINES mechanism introduced in littlefs 2.10.0. This allows for more flexible configuration and improves code maintainability."
  • What functional part of the code is being changed? Specifically mention the affected files/modules. E.g., "Changes are made to the littlefs driver in fs/littlefs."
  • How does the change exactly work? Briefly explain the implementation details. For example, "This change introduces a Kconfig option CONFIG_FS_LITTLEFS_HAS_LFS_DEFINES to enable/disable the use of LFS_DEFINES. When enabled, the littlefs driver will utilize the new override mechanism; otherwise, it falls back to the previous behavior. "
  • Related Issues: If there's a corresponding NuttX issue prompting this change, link it here.

Missing Information in Impact:

Every "Impact" section needs a YES/NO answer and a description if the answer is YES. Currently, the entire Impact section is empty. Address each point specifically. For example:

  • New Feature? Possibly YES: "This allows utilizing the LFS_DEFINES feature of littlefs 2.10.0 for more flexible configuration."
  • User Impact? Likely NO, unless users were relying on specific undefined behavior that now changes. If NO, still explicitly state "NO. No changes to user API or expected behavior."
  • Build Impact? YES: "A new Kconfig option CONFIG_FS_LITTLEFS_HAS_LFS_DEFINES is introduced. Users can enable it when using littlefs 2.10.0 or later."
  • Hardware Impact? Likely NO. If so, explicitly state "NO."
  • Documentation Impact? Probably YES: "Documentation should be updated to explain the new Kconfig option and its usage."
  • Security Impact? Thoroughly analyze and justify your answer (hopefully NO).
  • Compatibility Impact? Potentially YES, depending on how the old and new mechanisms interact. Be specific about forward and backward compatibility. Does this change break compatibility with older littlefs versions? If YES "This change requires littlefs 2.10.0 or later. Previous versions are no longer supported with this option enabled."
  • Anything else? This is a good place to mention any special considerations.

Missing Information in Testing:

  • Build Host(s): Provide specifics. E.g., "Linux (Ubuntu 20.04), x86_64, GCC 9.4.0"
  • Target(s): Be specific. E.g., "qemu-x86_64:nsh", "STM32F4DISCOVERY:hello"
  • Testing Logs: The provided logs are insufficient. Show actual commands and outputs demonstrating the change's behavior. Include tests that show both the old and new behavior with the Kconfig option enabled and disabled. Show that using LFS_DEFINES works as expected and the fallback mechanism also functions correctly. Demonstrate how the change addresses the original issue (if there was one).

By providing the missing details and expanding on the existing information, the PR will be much clearer, easier to review, and more likely to be accepted.

@xiaoxiang781216 xiaoxiang781216 merged commit a3849af into apache:master Dec 13, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: File System File System issues Board: arm Board: xtensa 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