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 msd tests #9444

Merged
merged 2 commits into from Mar 21, 2019

Conversation

Projects
None yet
9 participants
@maciejbocianski
Copy link
Member

commented Jan 21, 2019

Description

Test for USB MSD calss
Tested on Windows and Linux. On Mac should also work
Supported targets see TESTS/usb_device/msd/mbed_app.json
Targets with small RAM (less then 70kB) and without flash memory (e.g ARCH_PRO) require ci-sheild + SD card to enable testing

TODO:

  • update README.md

Known issues:

  • sometimes fs get corrupted while mounting on Windows machine
  • sometimes test hungs due to that greentea_parse_kv is preempted by usb msd communication task (it doesn't get message from host despite is was send and is visible on serial console)

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[X] Test update
[ ] Breaking change

Reviewers

@c1728p9

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:usb_msd_tests branch Jan 21, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

Please review astyle travis job

@ciarmcom ciarmcom requested review from c1728p9 and ARMmbed/mbed-os-maintainers Jan 21, 2019

@ciarmcom

This comment has been minimized.

Copy link
Member

commented Jan 21, 2019

@maciejbocianski, thank you for your changes.
@c1728p9 @ARMmbed/mbed-os-maintainers please review.

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:usb_msd_tests branch Jan 21, 2019

@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Jan 21, 2019

astyle fixed

TESTS/host_tests/pyusb_msd.py Outdated
@@ -0,0 +1,242 @@
"""
mbed SDK

This comment has been minimized.

Copy link
@0xc0170

0xc0170 Jan 22, 2019

Member

can you use updated license header

/*
 * Copyright (c) 2019, Arm Limited and affiliates.
 * SPDX-License-Identifier: Apache-2.0
 *
 * Licensed under the Apache License, Version 2.0 (the "License");
 * you may not use this file except in compliance with the License.
 * You may obtain a copy of the License at
 *
 *     http://www.apache.org/licenses/LICENSE-2.0
 *
 * Unless required by applicable law or agreed to in writing, software
 * distributed under the License is distributed on an "AS IS" BASIS,
 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jan 23, 2019

Author Member

license headers fixed

@cmonr cmonr added needs: work and removed needs: review labels Jan 22, 2019

@0xc0170 0xc0170 added needs: review and removed needs: work labels Jan 23, 2019

@c1728p9
Copy link
Contributor

left a comment

This looks great so far. I left some minor feedback. Once the stability problems you mentioned above are fixed then this will be ready to go.

TESTS/usb_device/msd/mbed_app.json Outdated
@@ -0,0 +1,39 @@
{

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Jan 23, 2019

Contributor

Per-tests configs aren't allowed. You may be able to use the test configs at https://github.com/ARMmbed/mbed-os/tree/master/tools/test_configs. @bridadan or @theotherjimmy may have more details around this.

This comment has been minimized.

Copy link
@bridadan

bridadan Jan 24, 2019

Contributor

Unfortunately that mechanism isn't really documented. It seems like you can add an entry to the "default_test_configuration" but I haven't used it personally.

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jan 25, 2019

Author Member

What about --test-config <TEST_CONFIG> ?
It uses configs from https://github.com/ARMmbed/mbed-os/tree/master/tools/test_configs?

This comment has been minimized.

Copy link
@bridadan

bridadan Jan 25, 2019

Contributor

I believe so yes. If you don't specify a --test-config or an --app-config it seems to go with the default one.

#define MIN_HEAP_SIZE (HEAP_BLOCK_DEVICE_SIZE + 6144)


/* TODO:

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Jan 23, 2019

Contributor

Did you want to take action on this TODO?

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jan 24, 2019

Author Member

Sure, as soon as it will be supported in USBMSD class

TESTS/usb_device/msd/mbed_app.json Outdated
"target.features_add": ["STORAGE"]
},
"K64F": {
"target.components_remove": ["SD"]

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Jan 23, 2019

Contributor

Why does SD need to be removed from the K64F?

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jan 24, 2019

Author Member

My assumption was to test embedded flash BD in the first place. In case of k64f it has components SD and FLASHAPI

"components_add": ["SD", "FLASHIAP"],

Because the BlockDevice::get_default_instance() implementation uses SD before FLASHAPI
(see code below) we have to remove SD to let BlockDevice::get_default_instance() use FLASHAPI
MBED_WEAK BlockDevice *BlockDevice::get_default_instance()
{
#if COMPONENT_SPIF
static SPIFBlockDevice default_bd(
MBED_CONF_SPIF_DRIVER_SPI_MOSI,
MBED_CONF_SPIF_DRIVER_SPI_MISO,
MBED_CONF_SPIF_DRIVER_SPI_CLK,
MBED_CONF_SPIF_DRIVER_SPI_CS,
MBED_CONF_SPIF_DRIVER_SPI_FREQ
);
return &default_bd;
#elif COMPONENT_QSPIF
static QSPIFBlockDevice default_bd(
MBED_CONF_QSPIF_QSPI_IO0,
MBED_CONF_QSPIF_QSPI_IO1,
MBED_CONF_QSPIF_QSPI_IO2,
MBED_CONF_QSPIF_QSPI_IO3,
MBED_CONF_QSPIF_QSPI_SCK,
MBED_CONF_QSPIF_QSPI_CSN,
MBED_CONF_QSPIF_QSPI_POLARITY_MODE,
MBED_CONF_QSPIF_QSPI_FREQ
);
return &default_bd;
#elif COMPONENT_DATAFLASH
static DataFlashBlockDevice default_bd(
MBED_CONF_DATAFLASH_SPI_MOSI,
MBED_CONF_DATAFLASH_SPI_MISO,
MBED_CONF_DATAFLASH_SPI_CLK,
MBED_CONF_DATAFLASH_SPI_CS
);
return &default_bd;
#elif COMPONENT_SD
static SDBlockDevice default_bd(
MBED_CONF_SD_SPI_MOSI,
MBED_CONF_SD_SPI_MISO,
MBED_CONF_SD_SPI_CLK,
MBED_CONF_SD_SPI_CS
);
return &default_bd;
#elif COMPONENT_FLASHIAP
#if (MBED_CONF_FLASHIAP_BLOCK_DEVICE_SIZE == 0) && (MBED_CONF_FLASHIAP_BLOCK_DEVICE_BASE_ADDRESS == 0xFFFFFFFF)
size_t flash_size;
uint32_t start_address;
uint32_t bottom_address;
FlashIAP flash;
int ret = flash.init();
if (ret != 0) {
return 0;
}
//Find the start of first sector after text area
bottom_address = align_up(FLASHIAP_APP_ROM_END_ADDR, flash.get_sector_size(FLASHIAP_APP_ROM_END_ADDR));
start_address = flash.get_flash_start();
flash_size = flash.get_flash_size();
ret = flash.deinit();
static FlashIAPBlockDevice default_bd(bottom_address, start_address + flash_size - bottom_address);
#else
static FlashIAPBlockDevice default_bd;
#endif
return &default_bd;
#else
return NULL;
#endif
}

This comment has been minimized.

Copy link
@bridadan

bridadan Jan 24, 2019

Contributor

Perhaps it'd be possible to remove the dependency on the physical device all together? Could you use a HeapBlockDevice instead? https://github.com/ARMmbed/mbed-os/blob/master/features/storage/blockdevice/HeapBlockDevice.h

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jan 25, 2019

Author Member

Not really. I'm already using heap block device.
The idea was to test more than one type of block device. The other problem is that many devices has not enough RAM to create 64kB heap block device (it's min size to mount FAT32 fs on it)

This comment has been minimized.

Copy link
@bridadan

bridadan Jan 25, 2019

Contributor

The idea was to test more than one type of block device

I get ya. Though I think the main thing you're trying to test is the USB stack's interaction with the FileSystem API, not the BlockDevice API, correct?

The other problem is that many devices has not enough RAM to create 64kB heap block device (it's min size to mount FAT32 fs on it)

Dang that's a pain. Crazy idea: maybe there's a way to simulate having a large block device? Especially if you don't actually have to store everything and just need to say the data moving. Almost like a streaming block device or something. (I may be going too far off topic now)

targets/TARGET_NXP/TARGET_LPC176X/TARGET_ARCH_PRO/PinNames.h Outdated
SPI_MISO = P0_8,
SPI_SCK = P0_7,
SPI_CS = P0_6,

This comment has been minimized.

Copy link
@c1728p9

c1728p9 Jan 23, 2019

Contributor

Can this change be upstreamed into master directly

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jan 24, 2019

Author Member

Sure, I will do

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jan 24, 2019

Author Member

Moved to master #9482

TESTS/host_tests/pyusb_msd.py Outdated

It will be used to find new disks mounted during test.
"""
self.initial_disk_list = set(MSDUtils.disks())

This comment has been minimized.

Copy link
@jupe

jupe Jan 25, 2019

Contributor

how test would behave if we run multiple ones in same hosts parallel? test looks a bit problematic in ci where >>2 devices are connected in same host and devices appears and disappeared all the time.

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Jan 28, 2019

Author Member

In current version it will not work.
There is a plan to enhance host side msd device discovery/unmount mechanism similar as it's done in usb serial tests (https://github.com/ARMmbed/mbed-os/blob/feature-hal-spec-usb-device/TESTS/usb_device/serial distinguished by serial number)

This comment has been minimized.

Copy link
@maciejbocianski

maciejbocianski Feb 5, 2019

Author Member

@jupe serial number based host side storage detection was added 275d124
Serial number is generated so each test instance will have different one

@0xc0170 0xc0170 added needs: work and removed needs: review labels Jan 28, 2019

@c1728p9 c1728p9 force-pushed the ARMmbed:feature-hal-spec-usb-device branch from 091b0e7 to e23e57d Jan 29, 2019

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:usb_msd_tests branch 2 times, most recently to 90c443a Jan 30, 2019

Please re-review. Comments appear to be addressed

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@jupe @bridadan @c1728p9 please re-review latest changes.

@bridadan

This comment has been minimized.

Copy link
Contributor

commented Feb 6, 2019

@NirSonnenschein I left some previous comments but I don't believe I need to approve this PR. Let me know if that's not the case, though.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Feb 25, 2019

CI was started a bit ago.

@mbed-ci

This comment has been minimized.

Copy link

commented Feb 25, 2019

Test run: FAILED

Summary: 1 of 12 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@cmonr cmonr added needs: work and removed needs: CI labels Feb 25, 2019

@maciejbocianski

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2019

features-storage-tests-blockdevice-general_block_device failed:

[1551119791.78][CONN][INF] found KV pair in stream: {{__testcase_name;Testing get type functionality}}, queued... 
[1551119791.88][CONN][RXD] >>> Running case #1: 'Testing BlockDevice erase functionality'... [1551119791.88][CONN][RXD] [1551119791.88][CONN][INF] found KV pair in stream: {{__testcase_start;Testing BlockDevice erase functionality}}, queued... 
[1551119791.98][CONN][RXD] Test BlockDevice::get_erase_value().. 
[1551119841.99][CONN][RXD] :229::FAIL: Expected 0 Was -5005 
[1551119841.99][CONN][RXD] block_device->get_erase_value()=-1 
[1551119842.19][CONN][RXD] :234::SKIP: Erase not supported in this block device. Test skipped. [1551119842.19][CONN][INF] found KV pair in stream: {{__testcase_finish;Testing BlockDevice erase functionality;0;1}}, queued... 
[1551119842.29][CONN][RXD] >>> 'Testing BlockDevice erase functionality': 0 passed, 1 failed with reason 'Test Cases Failed'

@maciejbocianski maciejbocianski force-pushed the maciejbocianski:usb_msd_tests branch to 7ff689b Feb 27, 2019

@maciejbocianski maciejbocianski changed the base branch from feature-hal-spec-usb-device to master Feb 27, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 19, 2019

Test run: SUCCESS

Summary: 13 of 13 test jobs passed
Build number : 2
Build artifacts

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

One moment, doing a thing.

@cmonr cmonr closed this Mar 19, 2019

@cmonr cmonr removed the needs: CI label Mar 19, 2019

@cmonr cmonr reopened this Mar 19, 2019

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

continuous-integration/travis-ci/pr Pending — The Travis CI build is in progres

Now we're cooking! 😁

@cmonr cmonr added the needs: CI label Mar 19, 2019

@NirSonnenschein

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Starting CI

@mbed-ci

This comment has been minimized.

Copy link

commented Mar 20, 2019

Test run: FAILED

Summary: 2 of 13 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_cloud-client-test
  • jenkins-ci/mbed-os-ci_exporter
@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

CI job restarted: jenkins-ci/exporter

armclang: error: Failed to check out a license.

@cmonr

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

CI job restarted: jenkins-ci/cloud-client-test

WebSocketProtocolException: rsv is not implemented, yet

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Mar 21, 2019

@0xc0170 0xc0170 merged commit f99431f into ARMmbed:master Mar 21, 2019

28 checks passed

continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jenkins-ci/build-ARMC5 Success
Details
jenkins-ci/build-ARMC6 Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR8 Success
Details
jenkins-ci/cloud-client-test Success
Details
jenkins-ci/dynamic-memory-usage RTOS ROM(+0 bytes) RAM(+0 bytes)
Details
jenkins-ci/exporter Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/mbed2-build-ARMC5 Success
Details
jenkins-ci/mbed2-build-ARMC6 Success
Details
jenkins-ci/mbed2-build-GCC_ARM Success
Details
jenkins-ci/mbed2-build-IAR8 Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Local astyle testing has passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/doxy-spellcheck Local doxy-spellcheck testing has passed
Details
travis-ci/events Passed, runtime is 9817 cycles (-238 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/include_check Local include_check testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 8408B (+0.00%)
Details
travis-ci/psa-autogen Local psa-autogen testing has passed
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details
travis-ci/tools-py3.5 Local tools-py3.5 testing has passed
Details
travis-ci/tools-py3.6 Local tools-py3.6 testing has passed
Details
travis-ci/tools-py3.7 Local tools-py3.7 testing has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.