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

unmount function called during mount, and the mount function called during unmount (IEC-203) #78

Open
3 tasks done
lijunru-hub opened this issue Oct 15, 2024 · 5 comments
Assignees
Labels
Status: Reviewing Issue is being reviewed Type: Bug Bug in esp-usb

Comments

@lijunru-hub
Copy link
Contributor

Answers checklist.

  • I have read the component documentation ESP-IDF Components and the issue is not addressed there.
  • I am using target and esp-idf version as defined in component's idf_component.yml
  • I have searched the issue tracker for a similar issue and not found any related issue.

Which component are you using? If you choose Other, provide details in More Information.

device/esp_tinyusb

ESP-IDF version.

IDF release/v5.2

Development Kit.

ESP32S3

Used Component version.

1.4.5

More Information.

The function calls are opposite to the intended logic.

@lijunru-hub lijunru-hub added the Type: Bug Bug in esp-usb label Oct 15, 2024
@lijunru-hub
Copy link
Contributor Author

@espressif-bot espressif-bot added the Status: Opened Issue is new label Oct 15, 2024
@github-actions github-actions bot changed the title unmount function called during mount, and the mount function called during unmount unmount function called during mount, and the mount function called during unmount (IEC-203) Oct 15, 2024
@peter-marcisovsky
Copy link
Collaborator

It works like this:
tud_umount_cb is called when a USB device is unmounted from a USB host -> a storage is mounted on the application FW tinyusb_msc_storage_mount. Now the application FW owns the storage.

tud_mount_cb is called when a USB device is mounted to a USB host -> a storage is unmounted from the application FW tinyusb_msc_storage_unmount. Now the USB Host owns the storage.

https://github.com/espressif/esp-usb/blob/master/device/esp_tinyusb/include/tusb_msc_storage.h#L127

@DamienEspitallier
Copy link

Is calling tinyusb_msc_storage_unmount in tud_mount_cb really correct ?

It only does something if the firmware has previously mounted the storage (and probably do something with it). It will remove the access for the firmware with no way to refuse and can happen at any moment (user can plug the cable when he wants).

May be a good way would be to add a way to refuse the unmont with a custom return value of the TINYUSB_MSC_EVENT_PREMOUNT_CHANGED callback fired in tinyusb_msc_storage_unmount ? I can do a PR if you think this is a good fix.

@lijunru-hub
Copy link
Contributor Author

lijunru-hub commented Oct 28, 2024

There's another issue: tusb_msc_storage.c occupies the tud_mount and tud_unmount functions.

Although two callback interfaces are provided:

  • tusb_msc_callback_t callback_mount_changed
  • tusb_msc_callback_t callback_premount_changed

there is a logical bug during actual use. The callback is only triggered the first time, and subsequent plug-in or unplug events do not trigger the callback.

I believe a better approach would be to register a TinyUSB event and return the tud_mount and tud_unmount functions for user control.

@peter-marcisovsky peter-marcisovsky self-assigned this Jan 17, 2025
@espressif-bot espressif-bot added Status: In Progress Issue is being worked on and removed Status: Opened Issue is new labels Jan 17, 2025
@peter-marcisovsky
Copy link
Collaborator

Hi @lijunru-hub sorry for a late response:

To my understanding, the mount and unmount works as expected.

The following screenshot is an output log of the esp-idf/examples/peripherals/usb/device/tusb_msc example.

Vbus monitor implemented (Correct example functionality)

Image

The example works as expected. However, it is added with a Vbus monitor

Now, the tusb_msc example looks like this without the Vbus monitor:

Vbus monitor not implemented (Incorrect example functionalit)

Image

Without the Vbus monitor, it is not possible to catch tud_umount callback when plugging the MSC device out of the USB host. Thus the current implementation of the tusb_msc example is not able to change an ownership of the storage upon the tud_umount_cb callback.

Does it explain the issues you are having wit the tud_mount_cb and tud_umount_cb callbacks?
We can add some notes about the Vbus monitor to the tud_msc example, as this is not the first time we have seen an issue connected with Vbus monitor.

@DamienEspitallier

May be a good way would be to add a way to refuse the unmont with a custom return value of the TINYUSB_MSC_EVENT_PREMOUNT_CHANGED callback fired in tinyusb_msc_storage_unmount ? I can do a PR if you think this is a good fix.

Thanks for pointing it out, we have been thinking about adding this functionality, so the user can actually control whether to change the storage ownership upon the USB Cable connection event to the USB Host. I will take a look at the implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Reviewing Issue is being reviewed Type: Bug Bug in esp-usb
Projects
None yet
Development

No branches or pull requests

4 participants