Skip to content

USB device cleanup #8125

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

Conversation

bmcdonnell-ionx
Copy link
Contributor

@bmcdonnell-ionx bmcdonnell-ionx commented Sep 13, 2018

Description

Attn: @c1728p9: a little cleanup in the feature-hal-spec-usb-device branch.

Pull request type

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

@bmcdonnell-ionx
Copy link
Contributor Author

I did a test build (mbed compile -t GCC_ARM) for three devices - one using each of the currently supported targets:

  • K22F
  • LPC4088
  • NUCLEO_F446ZE

@c1728p9 c1728p9 self-requested a review September 13, 2018 18:49
@c1728p9
Copy link
Contributor

c1728p9 commented Sep 13, 2018

Thanks for the enhancements @bmcdonnell-ionx

@c1728p9
Copy link
Contributor

c1728p9 commented Sep 13, 2018

@bmcdonnell-ionx it looks like there are some build errors. If you build locally for the K64F you should get the same errors.

@bmcdonnell-ionx bmcdonnell-ionx force-pushed the feature-hal-spec-usb-device branch from aee5873 to ff2ec5a Compare September 13, 2018 19:22
@bmcdonnell-ionx
Copy link
Contributor Author

@c1728p9, OK, I think I got it.

@cmonr
Copy link
Contributor

cmonr commented Sep 23, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Sep 23, 2018

Build : FAILURE

Build number : 3136
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8125/

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 24, 2018

Please review the build failures (related to the changes)

@bmcdonnell-ionx
Copy link
Contributor Author

@0xc0170, what changed? Didn't this pass the checks before?

I don't know where to begin. How do I see what's failed?

When I click on Details next to the failed "ci-morph-build", I get, "This site can’t be reached
"mbed-os.mbedcloudtesting.com took too long to respond."

When I click on "Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8125/", I get

2018-09-24-build-artifacts-log

@cmonr
Copy link
Contributor

cmonr commented Sep 25, 2018

@bmcdonnell-ionx All of the failure logs are located within the FAIL directory. Looking at a random device's log, it looks like this PR doesn't pass the TESTS-USB_DEVICE-BASIC test against any target.

I'd recommend locally verifying that this PR works against that (and all other) tests.

@bmcdonnell-ionx bmcdonnell-ionx force-pushed the feature-hal-spec-usb-device branch 2 times, most recently from eb215d6 to dabe924 Compare September 25, 2018 22:21
@bmcdonnell-ionx
Copy link
Contributor Author

I fixed some stuff and rebased. The test output is very verbose, I'm not sure what all to look for, and it looks like some things fail both locally for me with and without my changes here.

I can't see the details of the latest CI failure (site can't be reached), and I haven't seen a "Build : FAILURE" that I can look at yet, so I await further instruction.

@cmonr
Copy link
Contributor

cmonr commented Sep 26, 2018

Interesting.
@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-test This might be interesting feedback to take in.

@bmcdonnell-ionx It's a bit counter-intuitive, but the details link that the merge checks provide are internal-only. We generally point users to the log results that are sourced from AWS.

Unfortunately, it looks like these failures are in a test that I'm not use to navigating around.

@SeppoTakalo @JanneKiiskila Would y'all mind chceking why this PR failed the smoke test?

@JanneKiiskila
Copy link
Contributor

Multiple failure reasons, K64F w IAR fails to this:


Building project mbed-cloud-client-example (K64F, IAR)

Scan: mbed-cloud-client-example

Scan: env

[ERROR] Feature 'BOOTLOADER' is not a supported features

[mbed] ERROR: "/usr/bin/python" returned error.

       Code: 1

       Path: "/builds/ws/mbed-os-ci-cloud-client-test-job@3/mbed-cloud-client-example"

       Command: "/usr/bin/python -u /builds/ws/mbed-os-ci-cloud-client-test-job@3/mbed-cloud-client-example/mbed-os/tools/make.py -t IAR -m K64F --source . --build ./BUILD/K64F/IAR"

       Tip: You could retry the last command with "-v" flag for verbose output

Must be some setup issue? @OPpuolitaival , @teetak01 any clues? Some issue with the mbed_app.json used?

GCC ARM has the same issue.

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 26, 2018

Based on the failures, I suspect the usb branch lacks behind (BOOTLOADER is not yet feature there) and the example that builds uses it. We might want to update the usb feature branch (@c1728p9 Please rebase)

@JanneKiiskila
Copy link
Contributor

JanneKiiskila commented Sep 26, 2018

Right you are, BOOTLOADER is a 5.10 based feature and this branch is based on Mbed OS 5.9.3.
@0xc0170 - the PR guidance texts should be perhaps modified to advice people to rebase against the tip of the master?

@JanneKiiskila
Copy link
Contributor

Definitely compiles once it is rebased, though the list of commits I get by doing the git pull origin master --rebase is a lot longer than just 4 commits.

I especially like the commit USB devices: avoid unnecessary floats - I never understood why we would use floats for waits in Mbed OS? Using that one float will pull in the whole floating point library, which we probably will not need for anything in quite a few embedded apps.

@c1728p9 c1728p9 force-pushed the feature-hal-spec-usb-device branch from 9e06f07 to 05d77ab Compare October 3, 2018 20:15
@bmcdonnell-ionx bmcdonnell-ionx force-pushed the feature-hal-spec-usb-device branch from dabe924 to 10c666e Compare October 5, 2018 14:19
@cmonr
Copy link
Contributor

cmonr commented Oct 13, 2018

/morph build

@mbed-ci
Copy link

mbed-ci commented Oct 13, 2018

Build : FAILURE

Build number : 3343
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8125/

@cmonr
Copy link
Contributor

cmonr commented Oct 23, 2018

@bmcdonnell-ionx Please take a look at the build errors.

@cmonr
Copy link
Contributor

cmonr commented Oct 29, 2018

Closing due to inactivity.

@bmcdonnell-ionx Feel free to reopen once an update is available.

@cmonr cmonr closed this Oct 29, 2018
@cmonr cmonr removed the needs: work label Oct 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants