Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

usbus/msc: add CONFIG_USBUS_MSC_AUTO_MTD option to create LUNs on init #19356

Merged
merged 5 commits into from
Mar 9, 2023
Merged

usbus/msc: add CONFIG_USBUS_MSC_AUTO_MTD option to create LUNs on init #19356

merged 5 commits into from
Mar 9, 2023

Conversation

benpicco
Copy link
Contributor

@benpicco benpicco commented Mar 6, 2023

Contribution description

I had some trouble combining tests/usbus_msc with stdio_cdc_acm.
Turns out the test was creating it's own usbus and was therefore not compatible with other modules that make use of usbus.

Fix this by adding USB MSC to auto_init_usbus and add a config option CONFIG_USBUS_MSC_AUTO_MTD to export all MTD devices on startup.

Testing procedure

tests/usbus_msc now provides all MTD devices via USB without manual configutaiton.

More importantly, it now also works on boards that use stdio_cdc_acm like adafruit-itsybitsy-m4:

[ 5252.107261] usb 1-1.4: new full-speed USB device number 40 using ehci-pci
[ 5252.217906] usb 1-1.4: New USB device found, idVendor=1209, idProduct=7d01, bcdDevice= 1.00
[ 5252.217917] usb 1-1.4: New USB device strings: Mfr=3, Product=2, SerialNumber=4
[ 5252.217920] usb 1-1.4: Product: adafruit-itsybitsy-m4
[ 5252.217923] usb 1-1.4: Manufacturer: RIOT-os.org
[ 5252.217925] usb 1-1.4: SerialNumber: 6B1818B4AF9DA5EE
[ 5252.220677] cdc_acm 1-1.4:1.0: ttyACM0: USB ACM device
[ 5252.221092] usb-storage 1-1.4:1.2: USB Mass Storage device detected
[ 5252.221457] scsi host8: usb-storage 1-1.4:1.2
[ 5253.240488] scsi 8:0:0:0: Direct-Access     RIOT-OS  RIOT_MSC_DISK     1.0 PQ: 0 ANSI: 1
[ 5253.240895] sd 8:0:0:0: Attached scsi generic sg3 type 0
[ 5253.241621] sd 8:0:0:0: [sdc] 512 4096-byte logical blocks: (2.10 MB/2.00 MiB)
[ 5253.242220] sd 8:0:0:0: [sdc] Write Protect is off
[ 5253.242223] sd 8:0:0:0: [sdc] Mode Sense: 03 00 00 00
[ 5253.242855] sd 8:0:0:0: [sdc] No Caching mode page found
[ 5253.242858] sd 8:0:0:0: [sdc] Assuming drive cache: write through
[ 5253.266933] sd 8:0:0:0: [sdc] Attached SCSI removable disk

Issues/PRs references

@github-actions github-actions bot added Area: drivers Area: Device drivers Area: sys Area: System Area: tests Area: tests and testing framework Area: USB Area: Universal Serial Bus labels Mar 6, 2023
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 6, 2023
@riot-ci
Copy link

riot-ci commented Mar 7, 2023 •
edited
Loading

Murdock results

✔️ PASSED

73400cb tests/usbus_msc: don't use custom usbus

Success Failures Total Runtime
6882 0 6882 09m:19s

Artifacts

dylad
dylad reviewed Mar 7, 2023
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

I'm quite puzzled by this PR right now.
I'd like the idea to initialize USBUS through auto_init() but on the other side, I don't think we should export all MTD devices by default and I don't like the idea of keeping the MSC ON all the time.

As I said in #19242, there are some use cases for this but I still don't think it should be the default behavior.

Boards which provides STDIO through CDC-ACM are tricky because in the current state, users need to have USB attached to the host to have the shell and configure which MTD devices to be exported. But we need to configure those devices before attaching USB to the host so we're a bit stuck here unless we accept to quickly detach/attach the USB (and thus, lose the serial connection for a few ms).

tests/usbus_msc/Makefile
Comment on lines -21 to -22
# Purposely disable auto_attach for this application
CFLAGS += -DCONFIG_USBUS_AUTO_ATTACH=0
Copy link
Member

Choose a reason for hiding this comment

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

if CONFIG_USBUS_MSC_AUTO_MTD is set to 0, removing this will lead to issue in the application test.

Copy link
Contributor Author

@benpicco benpicco Mar 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

tbh I wonder why - attaching with no LUNs configured should be no problem if we then detach before adding LUNs, right?

I think I saw an empty (0 byte) device on the host instead, but that might be a bug.

Copy link
Member

Choose a reason for hiding this comment

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

If CONFIG_USBUS_MSC_AUTO_MTD is set to 0 and if we auto attach USB device. No LUNs will be available at the enumuration. Then host will asks the number of available LUNs to the device with an USB_MSC_SETUP_REQ_GML request and answering 0 is not possible by the spec.

I guess we can remove it for now as CONFIG_USBUS_MSC_AUTO_MTD is set by default.

I'll figured it out later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But can we simply not advertise the MSC endpoint if no LUNs are configured?

Copy link
Member

Choose a reason for hiding this comment

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

Yup, but enable MSC at runtime will require to reset the USB and this might be troublesome for CDC-ACM stdio devices especially on Windows.

drivers/include/mtd_default.h
* @return MTD_0 for @p idx 0 and so on
* NULL if no MTD device exists for the given index
*/
static inline mtd_dev_t *mtd_default_get_dev(unsigned idx)
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering here:
Why the default part ? mtd_get_dev() would fit well too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It only works if the MTD devices follow the default naming convention

@github-actions github-actions bot added the Area: Kconfig Area: Kconfig integration label Mar 9, 2023
@dylad
Copy link
Member

dylad commented Mar 9, 2023

Please squash.

@benpicco benpicco force-pushed the usbus_msc-auto_init branch from 6fc64d4 to 73400cb Compare March 9, 2023 15:57
dylad
dylad approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@dylad dylad left a comment

Choose a reason for hiding this comment

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

ACK.

@dylad
Copy link
Member

dylad commented Mar 9, 2023

bors merge

@bors
Copy link
Contributor

bors bot commented Mar 9, 2023

Build succeeded:

@bors bors bot merged commit bdecf57 into RIOT-OS:master Mar 9, 2023
@benpicco benpicco deleted the usbus_msc-auto_init branch March 9, 2023 22:02
@MrKevinWeiss MrKevinWeiss added this to the Release 2023.04 milestone Apr 25, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@dylad dylad dylad approved these changes

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@bergzand bergzand Awaiting requested review from bergzand bergzand is a code owner

@jia200x jia200x Awaiting requested review from jia200x jia200x is a code owner

@MrKevinWeiss MrKevinWeiss Awaiting requested review from MrKevinWeiss MrKevinWeiss is a code owner

Assignees
No one assigned
Labels
Area: drivers Area: Device drivers Area: Kconfig Area: Kconfig integration Area: sys Area: System Area: tests Area: tests and testing framework Area: USB Area: Universal Serial Bus CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Milestone
Release 2023.04
Development

Successfully merging this pull request may close these issues.
4 participants
@benpicco @riot-ci @dylad @MrKevinWeiss