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

Replace calls to printf() with mbed-trace in netsocket tests #11861

Merged
merged 1 commit into from Nov 16, 2019

Conversation

@trowbridgec
Copy link
Contributor

trowbridgec commented Nov 13, 2019

Description (required)

Summary of change (What the change is for and why)

While troubleshooting why our cellular-enabled target (EP_AGORA) was failing some netsocket tests (DNS, TCP, UDP), we determined that the caused seemed to be related to serial output synchronization issues similar to what @ithinuel described in #9954 and #11202. Whenever the Greentea test would hit a printf() the test would hang and subsequent test cases would timeout. By replacing the calls to printf() with a new wrapper function called print_to_console() (which internally calls printf() if MBED_CONF_APP_DISABLE_LOGGING is not defined) in this PR, all of the tests passed without issue on our target.

For reference here is an example of an mbed_app.config file used with our target to pass the tests:

{
    "config": {
        "disable-logging": {
            "help": "Disable printf in print_to_console()",
            "value": null
        }
    },
    "target_overrides": {
        "EP_AGORA": {
            "nsapi.default-cellular-apn": "\"APN\"",
            "target.network-default-interface-type": "CELLULAR",
            "lwip.ipv4-enabled": true,
            "lwip.ipv6-enabled": true,
            "lwip.ethernet-enabled": false,
            "lwip.ppp-enabled": true,
            "lwip.tcp-enabled": true,
            "platform.stdio-convert-newlines": true,
            "platform.stdio-baud-rate": 9600,
            "platform.default-serial-baud-rate": 9600,
            "platform.stdio-buffered-serial": true,
            "lwip.mem-size": 22000,
            "nsapi.dns-response-wait-time": 60000,
            "disable-logging": true
        }
    }
}
Documentation (Details of any document updates required)

Pull request type (required)

[x] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results (required)

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers (optional)

@maclobdell
@ithinuel

Release Notes (required for feature/major PRs)

Summary of changes
Impact of changes
Migration actions required
@trowbridgec

This comment has been minimized.

Copy link
Contributor Author

trowbridgec commented Nov 13, 2019

@ciarmcom ciarmcom requested review from ithinuel, maclobdell and ARMmbed/mbed-os-maintainers Nov 13, 2019
@ciarmcom

This comment has been minimized.

Copy link
Member

ciarmcom commented Nov 13, 2019

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 14, 2019

Reading emac tests , we have there trace levels:

Test cases have different trace levels based on how much tracing can be done without affecting the performance of the test case. Configure tracing using SET\_TRACE\_LEVEL macro.

or using mbed_trace, rather than this custom wrapper?

@0xc0170 0xc0170 removed the request for review from ARMmbed/mbed-os-test Nov 14, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 14, 2019

@ARMmbed/mbed-os-ipcore Please review, this PR should be part of 5.14.2

@SeppoTakalo

This comment has been minimized.

Copy link
Contributor

SeppoTakalo commented Nov 14, 2019

@ARMmbed/mbed-os-maintainers What is the best practice regarding console outputs and printf() when running Greentea test?

Are we supposed to print out anything at all?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Nov 14, 2019

Printing should be okay as long as you only do it when the PC is waiting for a result. Otherwise it's a risk of breaking the protocol.

I think I'd advocate using mbed_trace, and having it off by default - people can turn it on to investigate failures. But others spend more time working with tests than me.

@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 14, 2019

This is one of the grey areas. Most of the time test does not print anything by default, should not.

I think I'd advocate using mbed_trace, and having it off by default - people can turn it on to investigate failures. But others spend more time working with tests than me.

+1. @trowbridgec can we update this to use trace as proposed?

@trowbridgec

This comment has been minimized.

Copy link
Contributor Author

trowbridgec commented Nov 14, 2019

@0xc0170 That sounds good to me. I've already tested that using trace also resolves this issue.

I'll commit the changes shortly for the tests we've been having issues with, however it occurs to me now that there may be other tests using printf() which should be converted to using trace.

@trowbridgec trowbridgec changed the title Replace calls to printf() with new wrapper function print_to_console() Replace calls to printf() with mbed-trace in netsocket tests Nov 14, 2019
Copy link
Contributor

maclobdell left a comment

LGTM

@adbridge adbridge added needs: CI and removed needs: review labels Nov 14, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 14, 2019

CI started. It might fail as there is master known issue

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Nov 14, 2019

Test run: FAILED

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

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-ARM
@maclobdell

This comment has been minimized.

Copy link
Contributor

maclobdell commented Nov 14, 2019

As this is critical, I hope this can be merged and released in 5.14.2 as is. @trowbridgec - in the future, in cases where you update your changes based on feedback, it is much preferred that you squash your commits down to one and force push back to your branch. This makes the git history cleaner. Since this is at a critical stage, I don't think that this is necessary right now, but it is good practice for later.

@trowbridgec trowbridgec force-pushed the trowbridgec:update-ep-agora-target branch from 600ea40 to 0592e95 Nov 14, 2019
@trowbridgec

This comment has been minimized.

Copy link
Contributor Author

trowbridgec commented Nov 14, 2019

@maclobdell I added similar fixes to the integration tests and squashed my commits.

@kivaisan

This comment has been minimized.

Copy link
Contributor

kivaisan commented Nov 15, 2019

I think this will make tracing those even worse and potentially break test results completely. Enabling mbed-traces will most probably trace much more info than just these test case prints and as by default traces are not synced between threads. This causes greentea's own prints to getting corrupted resulting in failures to be reported even though test itself passed ok.
This would be ok if greentea would also use mbed-trace and trace locks are set.

@40Grit

This comment has been minimized.

Copy link

40Grit commented Nov 15, 2019

@kivaisan Are saying that this change is not worth the effects that will be caused when trying to debug a test and trace is enabled?

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Nov 15, 2019

This isn't enabling the trace by default (I believe), so the output is prevented, so should be okay for CI.

For human testing, turning on the trace will also turn on any internal trace, but that should be addressable by setting the level appropriately. However, all the trace here is at "debug" level which would be problematic. Turning on that will by default give you all other internal traces. Most of it should be at least "info" level.

But you could still filter by the "GRNT" trace group.

@kivaisan

This comment has been minimized.

Copy link
Contributor

kivaisan commented Nov 15, 2019

Would it be feasible to change greentea itself to use mbed-trace when mbed-trace is enabled? Then it would enable use of mbed_trace_mutex_wait_function_set / mbed_trace_mutex_release_function_set to prevent output corruption?

@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Nov 15, 2019

Would it be feasible to change greentea itself to use mbed-trace when mbed-trace is enabled? Then it would enable use of mbed_trace_mutex_wait_function_set / mbed_trace_mutex_release_function_set to prevent output corruption?

In terms of this as a general idea I will leave others more knowledgeable in this area to comment. However, irrespective of that it is certainly not feasible within the current timescales. Technically code freeze for 5.14.2 was yesterday so if this PR is to come in , it needs to do so at the very latest by Monday....

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Nov 15, 2019

Would it be feasible to change greentea itself to use mbed-trace when mbed-trace is enabled?

It should be quite feasible for it to claim a mutex that is registered with mbed_trace_mutex_wait_function_set around its own comms output. Probably only a couple of adjustment points needed.

A small number of tests do output from interrupt though, which is why it's still using RawSerial generally (#6674), but most tests could use a mutex.

If this PR is leaving trace off, so the reduction in output fixes a CI, then it could go to 5.14.2 as-is, and a follow-up could add that mutex to make turning trace on work better.

@trowbridgec

This comment has been minimized.

Copy link
Contributor Author

trowbridgec commented Nov 15, 2019

@kivaisan This PR is not turning on mbed-trace by default for greentea tests; it is merely converting calls to printf() within greentea tests to use mbed-trace functions instead. By default, mbed-trace is disabled (https://github.com/ARMmbed/mbed-os/blob/master/features/frameworks/mbed-trace/mbed_lib.json#L6):

{
    "name": "mbed-trace",
    "config": {
        "enable": {
            "help": "Used to globally enable traces.",
            "value": null
        },
        "max-level": {
            "help": "This flag is used to optimize the code size. For example, setting trace optimization level to TRACE_LEVEL_INFO will define all tr_debug() macros empty, which reduces the binary size. The possible optimization levels are TRACE_LEVEL_DEBUG, TRACE_LEVEL_INFO, TRACE_LEVEL_WARN, TRACE_LEVEL_ERROR and TRACE_LEVEL_CMD. To set the output tracing level, please use mbed_trace_config_set(TRACE_ACTIVE_LEVEL_INFO). The possible tracing levels for mbed_trace_config_set() are TRACE_ACTIVE_LEVEL_ALL, TRACE_ACTIVE_LEVEL_DEBUG (same as ALL), TRACE_ACTIVE_LEVEL_INFO, TRACE_ACTIVE_LEVEL_WARN, TRACE_ACTIVE_LEVEL_ERROR, TRACE_ACTIVE_LEVEL_CMD and TRACE_LEVEL_NONE.",
            "value": null,
            "macro_name": "MBED_TRACE_MAX_LEVEL"

        },
        "fea-ipv6": {
            "help": "Used to globally disable ipv6 tracing features.",
            "value": null
        },
        "allocator": {
            "value": "malloc",
            "macro_name": "MEM_ALLOC"
        },
        "deallocator": {
            "value": "free",
            "macro_name": "MEM_FREE"
        }

    }
}

For human testing, turning on the trace will also turn on any internal trace, but that should be addressable by setting the level appropriately. However, all the trace here is at "debug" level which would be problematic. Turning on that will by default give you all other internal traces. Most of it should be at least "info" level.

But you could still filter by the "GRNT" trace group.

@kjbracey-arm That's a good point...I can switch to using tr_info()/tr_error() when appropriate.

@kjbracey-arm

This comment has been minimized.

Copy link
Contributor

kjbracey-arm commented Nov 15, 2019

That's a good point...I can switch to using tr_info()/tr_error() when appropriate.

As a general rule, debug level should not normally (need to) be shown except when you really are debugging - I believe IceTea defaults to showing info and above.

Anything vaguely interesting for a test run should be at least "info". Ideally we would have had the extra "notice" level between "info" and "warning" to be even more selective, but we don't.

(In the Linux kernel you can't even turn on "debug" globally IIRC - you'd turn it on per module).

@trowbridgec trowbridgec force-pushed the trowbridgec:update-ep-agora-target branch from 0592e95 to eac84ad Nov 15, 2019
@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Nov 15, 2019

@kjbracey-arm could you please re-review update? I will put into CI in the meantime

@trowbridgec trowbridgec force-pushed the trowbridgec:update-ep-agora-target branch from eac84ad to e59fbe5 Nov 15, 2019
@trowbridgec

This comment has been minimized.

Copy link
Contributor Author

trowbridgec commented Nov 15, 2019

Sorry, I had to fix an astyle issue...please re-review.

@adbridge

This comment has been minimized.

Copy link
Contributor

adbridge commented Nov 15, 2019

CI started

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Nov 15, 2019

Test run: FAILED

Summary: 1 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-GCC_ARM
@trowbridgec trowbridgec force-pushed the trowbridgec:update-ep-agora-target branch from e59fbe5 to a1edbb9 Nov 15, 2019
@0xc0170

This comment has been minimized.

Copy link
Member

0xc0170 commented Nov 15, 2019

CI restarted

@mbed-ci

This comment has been minimized.

Copy link

mbed-ci commented Nov 15, 2019

Test run: SUCCESS

Summary: 5 of 5 test jobs passed
Build number : 3
Build artifacts

@0xc0170 0xc0170 removed the needs: CI label Nov 16, 2019
@0xc0170 0xc0170 merged commit ee46d03 into ARMmbed:master Nov 16, 2019
20 checks passed
20 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-ARM Success
Details
jenkins-ci/build-GCC_ARM Success
Details
jenkins-ci/build-IAR Success
Details
jenkins-ci/greentea-test Success
Details
jenkins-ci/unittests Success
Details
travis-ci/astyle Success!
Details
travis-ci/docs Success!
Details
travis-ci/doxy-spellcheck Success!
Details
travis-ci/events Success! Runtime is 8817 cycles.
Details
travis-ci/gitattributestest Success!
Details
travis-ci/include_check Success!
Details
travis-ci/licence_check Success!
Details
travis-ci/littlefs Success! Code size is 8420B.
Details
travis-ci/psa-autogen Success!
Details
travis-ci/tools-py2.7 Success!
Details
travis-ci/tools-py3.5 Success!
Details
travis-ci/tools-py3.6 Success!
Details
travis-ci/tools-py3.7 Success!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.