Usb host msd block device #4230

jamike · 2017-04-26T13:37:09Z

Description

Since BlockDevice has been introduced USBHostMSD is no more supported.
With this patch USBhostMSD inherits from BlockDevice , and can be used with FATFileSystem class.

Migration

As USBHostMSD does not inherit from FATFilesystem any more, the code using the class has to be modified.

Status

READY

geky · 2017-04-26T15:32:05Z

With regard to the adoption of the block device api, this looks great!

geky · 2017-04-26T17:49:26Z

features/unsupported/tests/usb/host/mass_storage/main.cpp

+            printf("file system mounted\n");
+
+        if  (!msd.connect()) {
+            goto again;


Actually, could these gotos just be a continue statement inside this while loop?

agree, I will update.

geky · 2017-04-26T17:50:20Z

features/unsupported/tests/usb/host/mass_storage/main.cpp

+        while (msd.connected()) {
+            Thread::wait(500);
+        }
+        while (fs.unmount()==-1) {


This should really be checking for less than zero fs.unmount() < 0, since unmount may return a negative error code

agree, iI will update

jamike · 2017-04-27T09:39:52Z

Patch is updated : Comments taken into account and some more changes have been included in function USBHostMSD::read and USBHostMSD::program.

0xc0170 · 2017-05-02T11:54:24Z

features/unsupported/tests/usb/host/mass_storage/main.cpp

+        while(!msd.connect()) {
+            Thread::wait(500);
+        }
+        if (fs.mount(&msd) != 0) continue;


Can you fix the formatting for this if else?

https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/

Before pushing the patch I have used the astyle.exe command from page https://docs.mbed.com/docs/mbed-os-handbook/en/5.1/cont/code_style/
To fix the above indentation issue I added option -j .
AStyle.exe --style=kr --indent=spaces=4 --indent-switches -j
This solves the following requirement.
•One true brace style (1TBS) - use braces for statements of type if, else, while and for (exception from K&R).

-j is taking into account the above remarks.

adbridge · 2017-05-04T14:10:24Z

@0xc0170 are you happy with the review changes?

theotherjimmy · 2017-05-08T15:16:32Z

@0xc0170 Could you review please?

0xc0170 · 2017-05-08T15:19:07Z

features/unsupported/USBHost/USBHostMSD/USBHostMSD.cpp

@@ -29,13 +28,16 @@
 #define GET_MAX_LUN             (0xFE)
 #define BO_MASS_STORAGE_RESET   (0xFF)

-USBHostMSD::USBHostMSD(const char * rootdir) : FATFileSystem(rootdir)
+USBHostMSD::USBHostMSD()


for changes like this, please note in the commit and in the PR description that this is a breaking change? Any change of API should be emphasised and provided with a strong reason for doing so.

The BlockDevice was not existing when the initial version has been developped.
When BlockDevice and fatfilesystem evolution has been introduced, USBHostMSD was no more working (USB: MSD Device #4131) .
Since USB stick is exchanging block write , block read on USB.
To let application using fatfilesystem on this BlockDevice seems in line with the mbed filesystem evolution.

I understand. That's OK.

My intention for these type of changes is that a commit should contain information about breaking the compatibility - this warns all of us that this change is breaking, and any user pulling this commit should be warned. Something nice to have.

0xc0170 · 2017-05-08T15:19:57Z

features/unsupported/USBHost/USBHostMSD/USBHostMSD.cpp

 }

-void USBHostMSD::init() {
+void USBHostMSD::init_usb()


why is this renamed? Is it because of init method conflict somewhere in the hierarchy? Can it be resolved differently?

As USBhostMSD inherits from BlockDevice and block device as a virtual method init.
It is better to rename the method relative to USB init , as init_usb . Also this method is protected so the renaming does not modify the behaviour for the user of this class.

0xc0170 · 2017-05-08T15:21:34Z

features/unsupported/USBHost/USBHostMSD/USBHostMSD.cpp

    uint8_t buf[1], res;
    res = host->controlRead(    dev, USB_RECIPIENT_INTERFACE | USB_DEVICE_TO_HOST | USB_REQUEST_TYPE_CLASS,
                                0xfe, 0, msd_intf, buf, 1);
    USB_DBG("max lun: %d", buf[0]);
    return res;
 }

-int USBHostMSD::disk_initialize() {
+int USBHostMSD::init()


thanks for fixing the style. I would recommend sending this as separate PR, this is causing distraction from the actual changes in this PR - would be easier if its a separate patch

I ll take it account in my next PR. (the fact that USB is not in good style does not help ...)

0xc0170 · 2017-05-08T15:22:50Z

features/unsupported/USBHost/USBHostMSD/USBHostMSD.cpp

+bd_size_t USBHostMSD::size() const
+{
+    USB_DBG("FILESYSTEM: size ");
+    if (!disk_init)


if we are changing these lines, we shall update the style in here as it was done for the rest of the code?

agree , I ll update soon.

0xc0170 · 2017-05-08T15:26:31Z

tools/build.py

@@ -91,6 +91,12 @@
                      default=False,
                      help="Compile the DSP library")

+    parser.add_argument("-F", "--fat",


this is good for testing, but I would not send this to master now. The reason is that this fat file system is targeting mbed-os, and soon enough the new rtos will come that wont be supported in these scripts (use mbed-cli). Thus any tools/ changes in this PR shall be reverted. What do you think?

cc @sg-

0xc0170 · 2017-05-15T15:31:19Z

Can you please rebase ? And regarding that --fat change, please remove it as requested above

jamike · 2017-05-17T07:50:25Z

The patch is rebased and the Commit relative to mbed OS 2 test is removed.

0xc0170 · 2017-05-18T10:07:40Z

/morph test

adbridge · 2017-05-22T09:39:18Z

/morph test

mbed-bot · 2017-05-22T16:02:41Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 297

Test failed!

geky · 2017-05-22T17:01:23Z

@studavekar, could these be issues with the CI? The tests seem to not get past board flashing.

studavekar · 2017-05-22T20:05:12Z

Doesn't look like there was issue with CI, seems like it was device specific issues with ARCH_PRO and other failure related NUCLEO_F429Z one of failure.

Will re-trigger and will see how that goes

/morph test

mbed-bot · 2017-05-23T10:15:02Z

Result: FAILURE

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 313

Test failed!

adbridge · 2017-05-23T10:20:40Z

@studavekar looks like the NUCLEO_F429ZI became non-responsive, timeouts across the tests for 2 of the builds....

adbridge · 2017-05-24T13:21:57Z

/morph test

mbed-bot · 2017-05-24T20:58:23Z

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 333

All builds and test passed!

0xc0170 requested review from simonqhughes and geky April 26, 2017 14:19

0xc0170 added the needs: review label Apr 26, 2017

geky approved these changes Apr 26, 2017

View reviewed changes

geky reviewed Apr 26, 2017

View reviewed changes

chrissnow mentioned this pull request Apr 26, 2017

USB: MSD Device #4131

Closed

jamike force-pushed the USBHostMSD_BockDevice branch 2 times, most recently from 07ac34f to 3233031 Compare April 27, 2017 09:35

0xc0170 reviewed May 2, 2017

View reviewed changes

jamike force-pushed the USBHostMSD_BockDevice branch from 3233031 to d0df7fb Compare May 4, 2017 09:07

screamerbg changed the title ~~Usb host msd bock device~~ Usb host msd block device May 4, 2017

0xc0170 reviewed May 8, 2017

View reviewed changes

jamike force-pushed the USBHostMSD_BockDevice branch from d0df7fb to af7acf8 Compare May 9, 2017 13:32

0xc0170 added needs: work and removed needs: review labels May 15, 2017

jamike added 3 commits May 17, 2017 08:58

USHostMSD: rely on BlockDevice

d30a9a6

USBHostMSD : fix possible race condition during connection

edbba13

USBHost : modify USB_10 since USBHostMSD inherits BlockDevice class

6401d48

jamike force-pushed the USBHostMSD_BockDevice branch from af7acf8 to 6401d48 Compare May 17, 2017 07:46

0xc0170 added needs: CI and removed needs: work labels May 17, 2017

adbridge added ready for merge and removed needs: CI labels May 25, 2017

sg- merged commit 3122abe into ARMmbed:master May 26, 2017

sg- added release-version: 5.5.0-rc1 and removed ready for merge labels May 26, 2017

Usb host msd block device #4230

Usb host msd block device #4230

Uh oh!

Conversation

jamike commented Apr 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Migration

Status

Uh oh!

geky commented Apr 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamike commented Apr 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adbridge commented May 4, 2017

Uh oh!

theotherjimmy commented May 8, 2017

Uh oh!

0xc0170 May 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented May 15, 2017

Uh oh!

jamike commented May 17, 2017

Uh oh!

0xc0170 commented May 18, 2017

Uh oh!

adbridge commented May 22, 2017

Uh oh!

mbed-bot commented May 22, 2017

Output

Uh oh!

geky commented May 22, 2017

Uh oh!

studavekar commented May 22, 2017

Uh oh!

mbed-bot commented May 23, 2017

Output

Uh oh!

adbridge commented May 23, 2017

Uh oh!

adbridge commented May 24, 2017

Uh oh!

mbed-bot commented May 24, 2017

Output

Uh oh!

Uh oh!

jamike commented Apr 26, 2017 •

edited

Loading

0xc0170 May 8, 2017 •

edited

Loading