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

Enable flow control in Greentea #6660

Merged
merged 1 commit into from Apr 19, 2018

Conversation

Projects
None yet
5 participants
@marcuschangarm
Contributor

marcuschangarm commented Apr 17, 2018

Description

Flow control is enabled in Greentea for targets that has
console-uart-flow-control set.

Pull request type

[X] Fix
[ ] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change

Enable flow control in Greentea
Flow control is enabled in Greentea for targets that has
console-uart-flow-control set.
@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 17, 2018

@cmonr @0xc0170

Copy-pasted from mbed_retarget.cpp

@cmonr

cmonr approved these changes Apr 17, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 17, 2018

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 17, 2018

@marcuschangarm Confirming, this was needed because local testing for the #6547 wasn't working, correct?

@mbed-ci

This comment has been minimized.

mbed-ci commented Apr 17, 2018

Build : SUCCESS

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

Triggering tests

/morph test
/morph uvisor-test
/morph export-build
/morph mbed2-build

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 18, 2018

It's actually to take full advantage of #6603.

#6547 Depends on both.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 18, 2018

/morph export-build

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 18, 2018

This is pretty grim. Trying to use a serial port both through stdin/stdout, and through a separate Serial object is a hack, and this digs deeper

And this defeats buffering if set with platform.stdio-buffered.

Can't you just make greentea use stdout? putc/getc, rather than greentea_serial->putc/getc?

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 18, 2018

Also, we have platforms that don't use serial for the console, right? We've just done the console redirection for that, and this bypasses it. The greentea client code in principle should work on something other than serial ports.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 18, 2018

@kjbracey-arm This is not related to the patch, however good question! I tracked where this serial console was added, in the original greentea client ARMmbed/greentea-client@ff62371 - main drive to have it in interrupt context. This can be fixed

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 18, 2018

Okay, that looks like there was quite a specific requirement to do this raw IRQ-context output.

That's now possible as of mbed OS 5.8 by cooperating with mbed_retarget.cpp - you can now do

write(STDOUT_FILENO, buffer, len);

That is interrupt safe as long as the underlying console is, and the default non-buffered case (DirectSerial) is. Would currently fail on buffered UARTSerial, but thinking about improving that for fault and error prints.

@mbed-ci

This comment has been minimized.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 18, 2018

/morph export-build

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Apr 18, 2018

@studavekar Pipe closed failure , please review

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

This comment has been minimized.

Contributor

cmonr commented Apr 18, 2018

@marcuschangarm Could you answer @kjbracey-arm's questions?

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Apr 18, 2018

I think refactoring Greentea is out of scope of this PR which only adds flow control to the serial connection already present.

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Apr 19, 2018

I'd agree - this patch is safe enough as a fix, as it only activates with a newly-added flag. Messing with it further wouldn't be, so let's just raise an issue for the awkward serial port use.

@0xc0170 0xc0170 added ready for merge and removed needs: CI labels Apr 19, 2018

@0xc0170 0xc0170 merged commit 15cac12 into ARMmbed:master Apr 19, 2018

12 checks passed

AWS-CI uVisor Build & Test Success
Details
ci-morph-build build completed
Details
ci-morph-exporter build completed
Details
ci-morph-mbed2-build build completed
Details
ci-morph-test test completed
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 10483 cycles (+955 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/littlefs Passed, code size is 10092B (+0.00%)
Details
travis-ci/tools Local tools testing has passed
Details

@marcuschangarm marcuschangarm deleted the marcuschangarm:fix-greentea branch May 10, 2018

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