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

Added interface version information to mbed detect command. #5077

Merged
merged 11 commits into from Oct 5, 2017

Conversation

Projects
None yet
4 participants
@dhwalters423
Contributor

dhwalters423 commented Sep 12, 2017

Description

This adds the additional information of the interface version (IE DAPlink, OpenSDA, etc) to the command mbed detect

This has proved useful when debugging applications, particularly when encountering interface issues. Not knowing about mbedls, I found it difficult to debug my application, where the USB driver and board was crashing when dragging the .bin to the disk for deployment in Windows 10. This information is a "nice to have" for a quick reference for mbed developers.

Example output:

...Desktop\mbed Workspace\mbed-os>mbed detect

[mbed] Detected K64F, port COM4, mounted D:, interface version 0243
[mbed] Supported toolchains for K64F
+--------+-----------+-----------+-----------+-----------+-----------+
| Target | mbed OS 2 | mbed OS 5 |    ARM    |  GCC_ARM  |    IAR    |
+--------+-----------+-----------+-----------+-----------+-----------+
| K64F   | Supported | Supported | Supported | Supported | Supported |
+--------+-----------+-----------+-----------+-----------+-----------+
Supported targets: 1
Supported toolchains: 3

Status

Built on mbed OS 5.5, will be tested against mbed OS 5.6 when an rc is ready.

Migrations

Added one method to test_api.py def get_mounted_details_txt(mount_point) to pull the details.txt for a specific target given it's mount point. This could prove useful later as additional information may want to be propagated to other tools using the test API.

Related Issues

Submitted issue relating to this here:
ARMmbed/mbed-cli#541

The approach was changed slightly from the original issue request. This was because the mut matrix PrettyTable for the command mbed detect displayed specifically MUT information. In order to not change the format of the MUT JSON, I decided to create the additional function def get_mounted_details_txt(mount_point) which created another mbed ls tools resource and display the information separately from the mut matrix.

Steps to test or reproduce

Run the mbed detect command.

Expected output should be similar to:

[mbed] Detected K64F, port COM4, mounted D:, interface version 0243
[mbed] Supported toolchains for K64F
+--------+-----------+-----------+-----------+-----------+-----------+
| Target | mbed OS 2 | mbed OS 5 |    ARM    |  GCC_ARM  |    IAR    |
+--------+-----------+-----------+-----------+-----------+-----------+
| K64F   | Supported | Supported | Supported | Supported | Supported |
+--------+-----------+-----------+-----------+-----------+-----------+
Supported targets: 1
Supported toolchains: 3
@dhwalters423

This comment has been minimized.

Contributor

dhwalters423 commented Sep 12, 2017

Additional tests performed on Ubuntu 17.04 from VirtualBox using USB Extensions:

dwalters-VirtualBox:~/workspace/mbed-os$ mbed detect

[mbed] Detected K64F, port /dev/ttyACM0, mounted /media/dwalters/DAPLINK, interface version 0243
[mbed] Supported toolchains for K64F
+--------+-----------+-----------+-----------+-----------+-----------+
| Target | mbed OS 2 | mbed OS 5 |    ARM    |  GCC_ARM  |    IAR    |
+--------+-----------+-----------+-----------+-----------+-----------+
| K64F   | Supported | Supported | Supported | Supported | Supported |
+--------+-----------+-----------+-----------+-----------+-----------+
Supported targets: 1
Supported toolchains: 3
@@ -1647,6 +1647,12 @@ def get_module_avail(module_name):
"""
return module_name in sys.modules.keys()
def get_mounted_details_txt(mount_point):

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 14, 2017

Contributor

Why is this function in test_api.py? I don't think it logically fits here.

This comment has been minimized.

@dhwalters423

dhwalters423 Sep 14, 2017

Contributor

@theotherjimmy Happy to move it- my original thought process was to add it into test_api.get_autodetected_MUTS_list and return it as a mut object. After realising it would break the mut .json format, I decided to put it above. Any suggestions on where it may fit in better?

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 14, 2017

Contributor

Since it's only used in detect_targets.py let's put it there.

Removed get_mounted_details_txt from test_api.py
Fixed minor typos in test_api.py
Added get_interface_version to detect_targets
@dhwalters423

This comment has been minimized.

Contributor

dhwalters423 commented Sep 15, 2017

@theotherjimmy I removed get_mounted_details_txt from test_api.py and added get_interface_version to detect_targets, also fixed some minor typos I noticed.

@theotherjimmy

Thanks for the typo fixes. The changes look good.

def get_interface_version(mount_point):
""" Function returns interface version from the target mounted on the specified mount point
mount_point = mut['port']

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 15, 2017

Contributor

What is this line of the function description supposed to tell me?

This comment has been minimized.

@dhwalters423

dhwalters423 Sep 15, 2017

Contributor

Example usage. Though I just found one mistake- let me push an update there. An accurate usage should be mount_point = mut['disk']

@theotherjimmy theotherjimmy added needs: CI and removed needs: work labels Sep 15, 2017

@theotherjimmy

This looks great!

def get_interface_version(mount_point):
""" Function returns interface version from the target mounted on the specified mount point
Example of mount_point:
mount_point = mut['disk']

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 15, 2017

Contributor

I'm still not clear to me why I need an example of what to do with the parameter here. Are you suggesting that a user might be able to acquire a mount_point with the example code? If so, could you state that?

This comment has been minimized.

@dhwalters423

dhwalters423 Sep 15, 2017

Contributor

Pushed a new commit with a bit more clarity, apologies for the confusion ;)

@theotherjimmy

THAT IS SO CLEAR 👏

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 15, 2017

/morph test

Let's get this in.

@dhwalters423

This comment has been minimized.

Contributor

dhwalters423 commented Sep 15, 2017

Apologies @theotherjimmy but I had to push one more emergency commit. I found an edge case on a NRF52840_DK where there was no details.txt and thus details_txt was None. This was causing a boom, so I added in a safety check there. Thanks for the quick reviews

@theotherjimmy

Given the aforementioned "explosion", I think it might be good to have a unit test for this new function.

details_txt = mbeds.get_details_txt(mount_point)
if (details_txt is None):
details_txt = {}

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 15, 2017

Contributor

Could you just

return "unkown"

here?

This comment has been minimized.

@dhwalters423

dhwalters423 Sep 15, 2017

Contributor

Makes sense for a unit test, I hadn't realised how many variations of details.txt there could be until now. I'll work on this in the coming few days.

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 15, 2017

Result: SUCCESS

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

/morph test

Let's get this in.

Output

mbed Build Number: 1317

All builds and test passed!

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Sep 20, 2017

I restarted jenkins CI

@dhwalters423 Is this complete ?

@dhwalters423

This comment has been minimized.

Contributor

dhwalters423 commented Sep 20, 2017

Hi @0xc0170 it's not quite ready as I discovered a breaking issue that will require a bit of tweaking and some test coverage. The issue is when a device does not have an interface version in the details.txt listed, which was causing the function to blow up when it tried to return None.

I'm out of the office this week so I've been a bit slow to work on this but I will make another commit addressing these issues in the next coming days.

@0xc0170 0xc0170 added needs: work and removed needs: CI labels Sep 20, 2017

@dhwalters423

This comment has been minimized.

Contributor

dhwalters423 commented Sep 25, 2017

@theotherjimmy Added in tests as requested, though I think wrapping the entire thing in a try/except covers all cases, therefore I think there is an argument that the unit tests are redundant. Either way, it's a nice double coverage there

count += 1
self.valid_mount_point = mut['disk']
break

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 25, 2017

Contributor

This looks very out of place. Count is only ever going to be 1 or 0.

# Function should handle failure gracefully regardless of a mount point being present.
# Therefore it is safe to assume a valid mount point.
if count is 0:
self.valid_mount_point = "D:"

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 25, 2017

Contributor

This is needed by the test_interface_version_valid_mount_point function, so it should be assigned unconditionally.

This comment has been minimized.

@dhwalters423

dhwalters423 Sep 25, 2017

Contributor

How could we handle the case where there was no mount point (no device connected to the host), and across cases where host OS differs.. IE a valid mount point on Win 10 could be D: but a valid mount point in Linux is something like /media/{username}/DAPLink? There would need to be some condition that has to make an assumption at a certain point.

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 25, 2017

Contributor

How could we handle the case where there was no mount point (no device connected to the host), and across cases where host OS differs.

Test 'em all?

There would need to be some condition that has to make an assumption at a certain point.

Yes, but that assumption can be guaranteed with mocking.

:return:
"""
# Gather a valid mount point from the host machine
muts = get_autodetected_MUTS_list()

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 25, 2017

Contributor

This should be mocked so that we don't depend on this test machine having some connected devices.

def test_interface_version_valid_mount_point(self):
interface_version = get_interface_version(self.valid_mount_point)
assert len(interface_version) > 0

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 25, 2017

Contributor

This is not a strong guarantee. I would like to see an assertion like the others in this commit: asserting that the interface version is a particular string/number.

@theotherjimmy

Please use mocks in your testing to avoid hardware/environment requirements during testing.

@theotherjimmy

I like the tests. It's very clear what mbed LS should provide and what the mbed OS tools do.

class MbedLsToolsMock():
def __init__(self, type):
self.interface_test_type = type

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 29, 2017

Contributor

Could you change the name of this parameter to not shadow a builtin?

This comment has been minimized.

@dhwalters423

dhwalters423 Sep 29, 2017

Contributor

Done 👍

This comment has been minimized.

@theotherjimmy

theotherjimmy Sep 29, 2017

Contributor

💯

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Sep 29, 2017

It's been a long road. Let's get to the destination.

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 30, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 30, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1455

All builds and test passed!

@mbed-bot

This comment has been minimized.

mbed-bot commented Sep 30, 2017

Result: FAILURE

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

/morph test

Output

mbed Build Number: 1455

All builds and test passed!

Output

mbed Build Number: 1461

Build failed!

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 1, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1455

All builds and test passed!

Output

mbed Build Number: 1461

Build failed!

Output

mbed Build Number: 1466

All builds and test passed!

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 1, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1455

All builds and test passed!

Output

mbed Build Number: 1461

Build failed!

Output

mbed Build Number: 1466

All builds and test passed!

Output

mbed Build Number: 1471

All builds and test passed!

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 1, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1455

All builds and test passed!

Output

mbed Build Number: 1461

Build failed!

Output

mbed Build Number: 1466

All builds and test passed!

Output

mbed Build Number: 1471

All builds and test passed!

Output

mbed Build Number: 1477

All builds and test passed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 2, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 3, 2017

Result: ABORTED

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

/morph test

Output

mbed Build Number: 1498

Test failed!

@theotherjimmy

This comment has been minimized.

Contributor

theotherjimmy commented Oct 3, 2017

/morph test

@mbed-bot

This comment has been minimized.

mbed-bot commented Oct 3, 2017

Result: SUCCESS

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

/morph test

Output

mbed Build Number: 1514

All builds and test passed!

@theotherjimmy theotherjimmy merged commit da78647 into ARMmbed:master Oct 5, 2017

4 checks passed

Cam-CI uvisor Build & Test Success
Details
ci/morph-test Job has completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@dhwalters423 dhwalters423 deleted the dhwalters423:dhwalters423-add-detect-info branch Oct 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment