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

feat(msc_host): Add format MSC support #101

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

Conversation

igi540
Copy link
Collaborator

@igi540 igi540 commented Dec 9, 2024

Requirements

Provide the possibility to format connected flash drive

Description

Currently the flash drive should be formatted in advance, otherwise it can be formatted by ESP when mount error is present. To have the possibility to format connected flash drives in the USB Host MSC class driver explicitly, the new API call has been added. Now, the format process of the flash drive can be done not only during the mounting failure (format_if_mount_failed and FR_NO_FILESYSTEM) but explicitly from the application logic.

Related

Testing

Added test case for testing public API.


Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

Copy link
Collaborator

@roma-jam roma-jam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two small nitpicks, otherwise LGTM.

UPD: two small nitpicks about the code itself.

Couple of notes regarding the PR itself:

  • Usually we squash the commit history to make the PR more precise and not flood commit history with the commits. It could be done before merging and PR could have several commits, if the PR is complex enough and need to be reviewed in several steps (like this one: USB Host UVC driver version 2 #84)
  • PR description. Here I just prefer to reflect all the things that could help myself to understand why I did that commit (for me in the future). So, usually, I have:
    • Requirements section. This is a small info, which helps us to answer the question "why I am doing that?" or "why do we need that?" For example: Provide the possibility to format connected flash drive
    • Description section. This is a bit deeper description of the changes in the PR. Like, some technically related note, reasons or general description of the changes. Or might be the part of the issue description. For example: Currently the flash drive should be formatted in advance, otherwise it can be formatted by ESP when mount error is present. To have the possibility to format connected flash drives in the USB Host MSC class driver explicitly, the new API call has been added. Now, the format process of the flash drive can be done not only during the mounting failure (format_if_mount_failed and FR_NO_FILESYSTEM) but explicitly from the application logic.
    • Related section. Here we could have all related tickets. For example: - Closes https://github.com/espressif/esp-idf/issues/14890

Other fields are optional and may be added or kept clean.
It is not that strict as we have them in GitLab, but I thing that general idea to have the description here and there as close to each other as possible will help us.

Copy link
Collaborator

@peter-marcisovsky peter-marcisovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also update our tests, and use this new function.

Here, we have test case called can_be_formatted where we force fail mounting of the partition, to trigger the formatting.

Thus, we could either use this new public API function, to format the partition in this existing test case, instead of force failing the mounting.

Or to create a new, similar test case, where we create a file, format the partition and check if the file exists.

Otherwise LGTM

Added test case for public API format

Closes espressif/esp-idf#14890
@igi540 igi540 force-pushed the add-format-MSC-host branch from ffffead to ad575c0 Compare December 11, 2024 16:36
@igi540
Copy link
Collaborator Author

igi540 commented Dec 11, 2024

@peter-marcisovsky ok, thank you for suggesting about adding test case. I added another test case. While updating test I found a line which I am not sure whether it is correct. Why does it erase memory before formatting? PTAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Missing option for USB MSC drive format (IDFGH-14076)
4 participants