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

Improve efficiency and formatting of ITM output #7371

Merged
merged 1 commit into from Jul 5, 2018

Conversation

Projects
None yet
6 participants
@kjbracey-arm
Contributor

kjbracey-arm commented Jun 29, 2018

Description

SerialWireOutput was outputting 1 character per 32-bit write to the ITM stimulus port. This is inefficient, and causes processing problems with some viewers due to them receiving 3 NUL bytes between each desired character.

Rework to allow us to be more efficient, and eliminate those NUL bytes:

  • Retain existing mbed_itm_send() and clarify it's a single 32-bit write.
  • Add new mbed_itm_send_block() that is appropriate for sending character data, and modify SerialWireOutput to use it.
  • Move "wait for FIFO ready" check to before the write, rather than after.

One minor correction - FIFOREADY is a single bit of the register read. Don't interpret reserved bits.

Fixes #7373

Pull request type

[ ] Fix
[X] Refactor
[ ] New target
[ ] Feature
[ ] Breaking change
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 29, 2018

@RobMeades - something for you to test. I don't have any ITM boards handy.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:itm_block branch 2 times, most recently from b88cbca to 0129895 Jun 29, 2018

@RobMeades RobMeades referenced this pull request Jun 29, 2018

Closed

NULLs in SWO output #7373

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 29, 2018

Revised a couple of times - now compiles, at least.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:itm_block branch from 0129895 to 6d04224 Jun 29, 2018

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jun 29, 2018

Sweet. Performing printf("Hello world %d.\n", x); in a loop now produces the following output in JLink SWO Viewer and in GDB's SWO console:

Hello world 0.
Hello world 1.
Hello world 2.
Hello world 3.
Hello world 4.
@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 29, 2018

I'd be interested to see if you could actually measure a performance boost. Or is the port so fast it doesn't really matter?

It may be that it's not really worth the extra code size of doing the 32-bit writes for the block operation.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:itm_block branch from 6d04224 to acdcf42 Jun 29, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 29, 2018

Fixed doxygen - no functional change.

RobMeades added a commit to u-blox/infinite-iot that referenced this pull request Jun 29, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jun 29, 2018

On efficiency, it's possible the C library doesn't end up doing anything more than single-character write() calls anyway. I believe IAR's C library can't be persuaded to do otherwise, and the other libraries may default to non-buffered and need to be explicitly switched to line- or fully-buffered.

You could perhaps test theoretical throughtput more directly with some manual write(STDOUT_FILENO).

@RobMeades

This comment has been minimized.

Contributor

RobMeades commented Jun 29, 2018

For pure printf() output it appears to work quite fast enough: it didn't really work at all in any of the clients available to me before so I can't easily see what the performance improvement might be. If I sit in a tight loop printf()ing 10 characters at a time I notice no loss in GDB and lines go past way faster than I can read; at a very rough estimate 150 kbytes/s.

@0xc0170 0xc0170 requested a review from marcuschangarm Jun 29, 2018

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:itm_block branch from f71b85f to ca45797 Jun 29, 2018

@cmonr

Excited for the changes!

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 30, 2018

/morph build

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jun 30, 2018

@ARMmbed/mbed-os-maintainers Heads up that this PR adds a new function to HAL.
However, I've marked this as for a patch because 1) this is fixing behavior that should be working, and 2) the API addition does not regress user functionality.

@mbed-ci

This comment has been minimized.

mbed-ci commented Jun 30, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added ready for merge and removed needs: CI labels Jun 30, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jul 2, 2018

Sorry - need to correct this. The last commit ca45797 I added was wrong - we do still need the "enabled" checks.

Rereading documentation indicates that writing to the stimulus register is safe when disabled, but we will be getting "not ready" indications when we read, causing us to block indefinitely if disabled.

Removed that commit, leaving just the commit @RobMeades tested.

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jul 2, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 2, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr added needs: work and removed needs: CI labels Jul 3, 2018

@cmonr

This comment has been minimized.

Contributor

cmonr commented Jul 3, 2018

Rebase is needed.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:itm_block branch from acdcf42 to 32141f7 Jul 3, 2018

@cmonr

cmonr approved these changes Jul 3, 2018

/* Check if ITM and port is enabled */
if (((ITM->TCR & ITM_TCR_ITMENA_Msk) != 0UL) && /* ITM enabled */
((ITM->TER & (1UL << port)) != 0UL)) { /* ITM Port enabled */
{

This comment has been minimized.

@cmonr

cmonr Jul 3, 2018

Contributor

Extra curly brace.

Improve efficiency and formatting of ITM output
SerialWireOutput was outputting 1 character per 32-bit write to the
ITM stimulus port. This is inefficient, and causes processing problems
with some viewers due to them receiving 3 NUL bytes between each
desired character.

Rework to allow us to be more efficient, and eliminate those NUL bytes:

* Retain existing mbed_itm_send() and clarify it's a single 32-bit write.
* Add new mbed_itm_send_block() that is appropriate for sending
  character data, and modify SerialWireOutput to use it.
* Move "wait for FIFO ready" check to before the write, rather than
  after.

One minor correction - FIFOREADY is a single bit of the register read.
Don't interpret reserved bits.

@kjbracey-arm kjbracey-arm force-pushed the kjbracey-arm:itm_block branch from 32141f7 to 0f98338 Jul 4, 2018

@kjbracey-arm kjbracey-arm added needs: CI and removed needs: work labels Jul 5, 2018

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Jul 5, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Jul 5, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr

cmonr approved these changes Jul 5, 2018

@cmonr cmonr merged commit c72da66 into ARMmbed:master Jul 5, 2018

14 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/astyle Passed, 791 files
Details
travis-ci/docs Local docs testing has passed
Details
travis-ci/events Passed, runtime is 9578 cycles (-637 cycles)
Details
travis-ci/gitattributestest Local gitattributestest testing has passed
Details
travis-ci/licence_check Local licence_check testing has passed
Details
travis-ci/littlefs Passed, code size is 9964B (+0.00%)
Details
travis-ci/tools-py2.7 Local tools-py2.7 testing has passed
Details

@cmonr cmonr removed the needs: CI label Jul 5, 2018

@kjbracey-arm kjbracey-arm deleted the kjbracey-arm:itm_block branch Jul 6, 2018

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