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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b6d2209
sys/auto_init: add support for USB MSC
benpicco Mar 6, 2023
092009e
drivers/mtd_default: add helper function for default MTD devices
benpicco Mar 6, 2023
f3dc90c
tests/mtd_raw: make use of mtd_default.h
benpicco Mar 6, 2023
bbda852
usbus/msc: add CONFIG_USBUS_MSC_AUTO_MTD option to create LUNs on init
benpicco Mar 6, 2023
73400cb
tests/usbus_msc: don't use custom usbus
benpicco Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 77 additions & 0 deletions drivers/include/mtd_default.h
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/*
* Copyright (C) 2023 ML!PA Consulting GmbH
*
* This file is subject to the terms and conditions of the GNU Lesser
* General Public License v2.1. See the file LICENSE in the top level
* directory for more details.
*/
/**
* @ingroup drivers_mtd
* @{
* @brief Default MTD device configuration
*
* Helpers for generic MTD use.
*
* @author Benjamin Valentin <benjamin.valentin@ml-pa.com>
*/
#ifndef MTD_DEFAULT_H
#define MTD_DEFAULT_H

#include "board.h"
#include "mtd.h"

#ifdef __cplusplus
extern "C" {
#endif

/**
* @brief Number of MTD devices
*/
#ifndef MTD_NUMOF
#if defined(MTD_3)
#define MTD_NUMOF 4
#elif defined(MTD_2)
#define MTD_NUMOF 3
#elif defined(MTD_1)
#define MTD_NUMOF 2
#elif defined(MTD_0)
#define MTD_NUMOF 1
#else
#define MTD_NUMOF 0
#endif
#endif

/**
* @brief Get the default MTD device by index
*
* @param[in] idx Index of the MTD device
*
* @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

{
switch (idx) {
#ifdef MTD_0
case 0: return MTD_0;
#endif
#ifdef MTD_1
case 1: return MTD_1;
#endif
#ifdef MTD_2
case 2: return MTD_2;
#endif
#ifdef MTD_3
case 3: return MTD_3;
#endif
}

return NULL;
}

#ifdef __cplusplus
}
#endif

#endif /* MTD_DEFAULT_H */
/** @} */
14 changes: 14 additions & 0 deletions sys/auto_init/usb/auto_init_usb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,10 @@ usbus_cdcecm_device_t cdcecm;
#include "usb/usbus/dfu.h"
static usbus_dfu_device_t dfu;
#endif
#ifdef MODULE_USBUS_MSC
#include "usb/usbus/msc.h"
static usbus_msc_device_t msc;
#endif

static char _stack[USBUS_STACKSIZE];
static usbus_t usbus;
Expand Down Expand Up @@ -66,6 +70,16 @@ void auto_init_usb(void)
usbus_dfu_init(&usbus, &dfu, USB_DFU_PROTOCOL_RUNTIME_MODE);
#endif

#ifdef MODULE_USBUS_MSC
/* Initialize Mass Storage Class */
usbus_msc_init(&usbus, &msc);
#endif

/* Finally initialize USBUS thread */
usbus_create(_stack, USBUS_STACKSIZE, USBUS_PRIO, USBUS_TNAME, &usbus);
}

usbus_t *usbus_auto_init_get(void)
{
return &usbus;
}
10 changes: 10 additions & 0 deletions sys/include/usb/usbus.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ extern "C" {
#endif
#endif

/**
* @brief USBUS MSC auto MTD setting
*
* When set to 1, the USBUS MSC module will automatically create a LUN for
* each MTD device defined in `board.h`.
*/
#ifndef CONFIG_USBUS_MSC_AUTO_MTD
#define CONFIG_USBUS_MSC_AUTO_MTD 1
#endif

/**
* @brief USBUS endpoint 0 buffer size
*
Expand Down
2 changes: 1 addition & 1 deletion sys/include/usb/usbus/msc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
#include <stdint.h>
#include "usb/usbus.h"
#include "usb/usbus/msc/scsi.h"
#include "mtd.h"
#include "mtd_default.h"

#ifdef __cplusplus
extern "C" {
Expand Down
7 changes: 7 additions & 0 deletions sys/usb/usbus/msc/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,13 @@ menuconfig MODULE_USBUS_MSC

if MODULE_USBUS_MSC

config USBUS_MSC_AUTO_MTD
bool "Automatically export all MTD devices via USB"
default true
help
This will automatically export all MTD devices that follow
the default naming scheme on startup.

config USBUS_MSC_VENDOR_ID
string "MSC Vendor ID"
default "RIOT-OS"
Expand Down
7 changes: 7 additions & 0 deletions sys/usb/usbus/msc/msc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,13 @@ static void _init(usbus_t *usbus, usbus_handler_t *handler)

/* Prepare to receive first bytes from Host */
usbdev_ep_xmit(msc->ep_out->ep, msc->out_buf, CONFIG_USBUS_EP0_SIZE);

/* Auto-configure all MTD devices */
if (CONFIG_USBUS_MSC_AUTO_MTD) {
for (int i = 0; i < USBUS_MSC_EXPORTED_NUMOF; i++) {
usbus_msc_add_lun(usbus, mtd_default_get_dev(i));
}
}
}

static int _control_handler(usbus_t *usbus, usbus_handler_t *handler,
Expand Down
36 changes: 4 additions & 32 deletions tests/mtd_raw/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,40 +25,12 @@
#include <string.h>

#include "od.h"
#include "mtd.h"
#include "mtd_default.h"
#include "shell.h"
#include "board.h"
#include "macros/units.h"
#include "test_utils/expect.h"

#ifndef MTD_NUMOF
#ifdef MTD_0
#define MTD_NUMOF 1
#else
#define MTD_NUMOF 0
#endif
#endif

static mtd_dev_t *_get_mtd_dev(unsigned idx)
{
switch (idx) {
#ifdef MTD_0
case 0: return MTD_0;
#endif
#ifdef MTD_1
case 1: return MTD_1;
#endif
#ifdef MTD_2
case 2: return MTD_2;
#endif
#ifdef MTD_3
case 3: return MTD_3;
#endif
}

return NULL;
}

static mtd_dev_t *_get_dev(int argc, char **argv)
{
if (argc < 2) {
Expand All @@ -73,7 +45,7 @@ static mtd_dev_t *_get_dev(int argc, char **argv)
return NULL;
}

return _get_mtd_dev(idx);
return mtd_default_get_dev(idx);
}

static uint64_t _get_size(mtd_dev_t *dev)
Expand Down Expand Up @@ -311,7 +283,7 @@ static int cmd_info(int argc, char **argv)

for (int i = 0; i < MTD_NUMOF; ++i) {
printf(" -=[ MTD_%d ]=-\n", i);
_print_info(_get_mtd_dev(i));
_print_info(mtd_default_get_dev(i));
}
return 0;
}
Expand Down Expand Up @@ -481,7 +453,7 @@ int main(void)
for (int i = 0; i < MTD_NUMOF; ++i) {
printf("init MTD_%d… ", i);

mtd_dev_t *dev = _get_mtd_dev(i);
mtd_dev_t *dev = mtd_default_get_dev(i);
int res = mtd_init(dev);
if (res) {
printf("error: %d\n", res);
Expand Down
2 changes: 0 additions & 2 deletions tests/usbus_msc/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@ USEMODULE += ps
USEMODULE += shell
USEMODULE += usbus_msc
USEMODULE += ztimer_msec
# Purposely disable auto_attach for this application
CFLAGS += -DCONFIG_USBUS_AUTO_ATTACH=0
Comment on lines -21 to -22
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.


# Change this to 0 show compiler invocation lines by default:
QUIET ?= 1
Expand Down
51 changes: 10 additions & 41 deletions tests/usbus_msc/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,39 +22,13 @@
#include <stdio.h>
#include <string.h>

#include "mtd.h"
#include "mtd_default.h"
#include "shell.h"
#include "usb/usbus.h"
#include "usb/usbus/msc.h"
#include "ztimer.h"

#ifndef MTD_NUMOF
#define MTD_NUMOF 0
#endif

static char _stack[USBUS_STACKSIZE];
static usbus_t usbus;
static usbus_msc_device_t msc;

static mtd_dev_t *_get_mtd_dev(unsigned idx)
{
switch (idx) {
#ifdef MTD_0
case 0: return MTD_0;
#endif
#ifdef MTD_1
case 1: return MTD_1;
#endif
#ifdef MTD_2
case 2: return MTD_2;
#endif
#ifdef MTD_3
case 3: return MTD_3;
#endif
}

return NULL;
}
static usbus_t *usbus;

static int _cmd_add_lun(int argc, char **argv)
{
Expand All @@ -76,8 +50,8 @@ static int _cmd_add_lun(int argc, char **argv)
puts("error: invalid MTD device specified");
return -2;
}
mtd_dev = _get_mtd_dev(dev);
ret = usbus_msc_add_lun(&usbus, mtd_dev);
mtd_dev = mtd_default_get_dev(dev);
ret = usbus_msc_add_lun(usbus, mtd_dev);
if (ret != 0) {
printf("Cannot add LUN device (error:%d %s)\n", ret, strerror(-ret));
}
Expand All @@ -104,8 +78,8 @@ static int _cmd_remove_lun(int argc, char **argv)
puts("error: invalid MTD device specified");
return -2;
}
mtd_dev = _get_mtd_dev(dev);
ret = usbus_msc_remove_lun(&usbus, mtd_dev);
mtd_dev = mtd_default_get_dev(dev);
ret = usbus_msc_remove_lun(usbus, mtd_dev);
if (ret == -EAGAIN) {
printf("MTD device was not registered\n");
}
Expand All @@ -118,7 +92,7 @@ static int _cmd_usb_attach(int argc, char **argv)
(void)argv;
static const usbopt_enable_t _enable = USBOPT_ENABLE;

usbdev_set(usbus.dev, USBOPT_ATTACH, &_enable,
usbdev_set(usbus->dev, USBOPT_ATTACH, &_enable,
sizeof(usbopt_enable_t));
return 0;
}
Expand All @@ -129,7 +103,7 @@ static int _cmd_usb_detach(int argc, char **argv)
(void)argv;
static const usbopt_enable_t _enable = USBOPT_DISABLE;

usbdev_set(usbus.dev, USBOPT_ATTACH, &_enable,
usbdev_set(usbus->dev, USBOPT_ATTACH, &_enable,
sizeof(usbopt_enable_t));
return 0;
}
Expand Down Expand Up @@ -160,16 +134,11 @@ int main(void)

/* Get driver context */
usbdev_t *usbdev = usbdev_get_ctx(0);

assert(usbdev);

/* Initialize basic usbus struct, don't start the thread yet */
usbus_init(&usbus, usbdev);
usbus_t *usbus_auto_init_get(void);
usbus = usbus_auto_init_get();

/* Initialize Mass Storage Class */
usbus_msc_init(&usbus, &msc);
/* Create USBUS thread */
usbus_create(_stack, USBUS_STACKSIZE, USBUS_PRIO, USBUS_TNAME, &usbus);
/* start shell */
puts("All up, running the shell now");
char line_buf[SHELL_DEFAULT_BUFSIZE];
Expand Down