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

USB Host MSC: Add request sense feature #34

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

peter-marcisovsky
Copy link
Collaborator

@peter-marcisovsky peter-marcisovsky commented May 21, 2024

Closing IDF-9890

Added request sense feature for MCS driver.

After each BOT command execution a CSW is checked for possible errors. If an error is present in the MSC device a bCSWStatus is returned to the Host as non zero. Ref: USB Mass Storage Class – Bulk Only Transport table 5.3. But a current MSC Host driver does not allow the user to find out what is causing the error.

This MR adds a feature to the MSC Host to issue a request sense to the MSC device each time, after checking CSW for errors. The request sense command returns specific error code. Ref: USB Mass Storage Class – UFI Command Specification Chapter 4.11 and Table 51

@peter-marcisovsky peter-marcisovsky force-pushed the feature/msc_host_request_sense branch 3 times, most recently from 85198b0 to 1033e50 Compare May 21, 2024 21:17
Copy link
Collaborator

@tore-espressif tore-espressif left a comment

Choose a reason for hiding this comment

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

LGTM @roma-jam PTAL too

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.

One nitpick, otherwise LGTM!

host/class/msc/usb_host_msc/src/msc_scsi_bot.c Outdated Show resolved Hide resolved
host/class/msc/usb_host_msc/src/msc_scsi_bot.c Outdated Show resolved Hide resolved
@peter-marcisovsky peter-marcisovsky force-pushed the feature/msc_host_request_sense branch from 1033e50 to 282194b Compare June 4, 2024 18:06
@peter-marcisovsky
Copy link
Collaborator Author

@roma-jam Coul you please take a look at the failing CI?
It looks like it's related to the recent enumeration refactoring. I tried running the pytest locally on esp-idf master and reproduced the same error as in the CI.

2024-06-05 13:21:44 [dut-1] D (1418) ENUM: [0:0] CHECK_FULL_PROD_STR_DESC OK
2024-06-05 13:21:44 [dut-1] D (1428E (1428) USBH: Dev 1 EP 0 STALL
2024-06-05 13:21:44 [dut-1] ) ENUM: [0:0] GET_SHORT_SER_STR_DESC OK
2024-06-05 13:21:44 [dut-0] W (12908) tusb_desc: String index (3) points to NULL, check your string descriptor
2024-06-05 13:21:44 [dut-1] D (1428) USBH: Processing actions 0xc
2024-06-05 13:21:44 [dut-1] D (1438) USBH: Default pipe device 1
2024-06-05 13:21:44 [dut-1] E (1438) ENUM: [0:0] Control transfer failed, status=4
2024-06-05 13:21:44 [dut-1] D (1448) ENUM: [0:0] CANCEL OK

@peter-marcisovsky peter-marcisovsky force-pushed the feature/msc_host_request_sense branch from 282194b to e98477d Compare June 5, 2024 20:25
@tore-espressif
Copy link
Collaborator

@peter-marcisovsky Please let me know if we should wait for the CI pass or whether I can force merge 🥇

@peter-marcisovsky
Copy link
Collaborator Author

@tore-espressif we are only waiting for GitLab master branch sync to GH, to pass the CI. So This MR is ready to be merged

@roma-jam
Copy link
Collaborator

@peter-marcisovsky @tore-espressif
CI unlocked, feel free to merge 🚀

@peter-marcisovsky peter-marcisovsky force-pushed the feature/msc_host_request_sense branch 2 times, most recently from dc45521 to e08deae Compare June 17, 2024 08:42
@peter-marcisovsky peter-marcisovsky force-pushed the feature/msc_host_request_sense branch from e08deae to 07d9107 Compare June 17, 2024 08:53
@peter-marcisovsky peter-marcisovsky merged commit b93f8cc into master Jun 17, 2024
17 checks passed
@peter-marcisovsky peter-marcisovsky deleted the feature/msc_host_request_sense branch December 20, 2024 13:48
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.

4 participants