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

[feature-hal-itm] Instrumented Trace Macrocell HAL API for SWO debug output #5956

Merged
merged 3 commits into from Feb 16, 2018

Conversation

@marcuschangarm
Contributor

marcuschangarm commented Jan 28, 2018

HAL API for initializing ITM and setting up SWO output.

Uses FileHandle redirection for debug output.

Note: this should be merged to a new feature branch and not to master.

Depends on: #5571

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jan 28, 2018

@yogpan01 @TeroJaasko

This is what you really need for the miniclient.

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Jan 28, 2018

@0xc0170 @bulislaw

As we discussed. Can you create a feature branch please?

@cmonr cmonr added the needs: review label Jan 29, 2018

@mbed-ci

This comment has been minimized.

mbed-ci commented Jan 29, 2018

@marcuschangarm, thank you for your changes. You should get some feedback from the following people shortly: @bulislaw @ARMmbed/team-nordic @geky @pan- @c1728p9 @SenRamakri @mbed-os-maintainers @scartmell-arm please review.

@kjbracey-arm

Yes, this is how the FileHandle stuff should work.

You could also make your target implement mbed_target_override_console so that stdout/stderr goes to SWO by default. Or the application itself can do mbed_overide_console.

If you do do that in the target, you may or may not want to take care in the hal ITM implementation so that it isn't the end of the world if stdout is a SingleWireOutput in the target code, and the application also defines their SingleWireOutput, so it is double inited. The serial API stuff has similar mucking around. (TBH I think that's something that shouldn't be encouraged, so feel free to not worry about that). It may be there's nothing really to do as your current HAL is singleton anyway, so SingleWireOutput has no state.

I'm kicking people for review on #5571.

#include "hal/itm_api.h"
class SingleWireOutput : public FileHandle {

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 29, 2018

Contributor

To fit the pattern of the rest of the system, this would be drivers/SingleWireOutput.h - you've put the ITM bits in the HAL, and this is the corresponding C++ wrapper. So it goes with SPI and friends.

This comment has been minimized.

@marcuschangarm

marcuschangarm Jan 29, 2018

Contributor

I was hoping to eventually put it into mbed_retarget.cpp like you did with the serial class: https://github.com/kjbracey-arm/mbed-os/blob/d7e42d1f69e5a8f06441742d910d795c8ae22c85/platform/mbed_retarget.cpp#L128-L180

so that we can enable SWO from the config system.

My intention with this PR is to start some discussion about what would be right approach. @0xc0170 @bulislaw any comments?

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 30, 2018

Contributor

I'm slightly wary of setting up a sort of "central registry" pattern where every potential device has to be plopped in there.

In #5558 there's a similar concept - the JSON choice of "default onboard network stack". You set nsapi.default-stack to either "LWIP" or "NANOSTACK". There's no central logic for that, but each of the two stacks has a code snippet that looks like

#define NANOSTACK 0x82917429
#if MBED_CONF_NSAPI_DEFAULT_STACK == NANOSTACK
OnboardNetworkStack &OnboardNetworkStack::get_default_instance() {
    return Nanostack::get_instance();
}
#endif

So only one of them will define that function.

Maybe the same concept would work here. JSON option in central place, but any target or library one can add a new selectable default without touching the core code - just invent a name.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 30, 2018

Contributor

Anyway, regardless of the "default console" selection, this needs to be exposed as a separate standalone device manually accessible as SingleWireOutput.

See my 18 Dec comments on #5356, where they're concentrating on alternative console, but I'm urging them to make sure that the device is always manually useable by the app without being forced to make it stdin/stdout.

The built-in FileHandles in my mbed_retarget.cpp are purely to preserve legacy behaviour. If anything, they will be moved out (@geky has suggested making Sink public, and discussions with @sg- have led to concensus that RawSerial should be a FileHandle, thus it can do the job of DirectSerial).

This comment has been minimized.

@marcuschangarm

marcuschangarm Jan 30, 2018

Contributor

No problem, I'll move/rename it to mbed-os/drivers.

I'm fine with either invocation as well, but I think we should have it documented exactly how we expect people to do it!

virtual ssize_t read(void *buffer, size_t size) {
unsigned char *buf = static_cast<unsigned char *>(buffer);
buf[0] = 0;

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 29, 2018

Contributor

I think it's better to just return 0 (indicating "end of file") or an error code (not sure which - -EBADF = "not valid file descriptor open for reading"?; -ENXIO = "... request was outside the capabilities of the device"?). Don't know whether EOF or error is better.

My dummy device implementation inside the retarget code is only reading endless NUL bytes to preserve the legacy behaviour of stdin with DEVICE_SERIAL unset. I don't think that's actually useful, or what you should do for something new.

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 29, 2018

Contributor

Did a bit of background reading to compare other systems. This is an interesting general question.

/dev/null returns EOF if opened for input. But that's more likely to be used manually as a redirect for processes that expect a redirect from file, and hence understand/expect EOF on stdin.

Many real devices probably would do ENXIO on the attempt to open for read. I guess that means attempts to use a read-only device as process input would fail the read open and then you'd get EBADF later.

Linux gives EBADF if you read from a file that's open for output only.

When you nohup and detach a process in Linux it does seem you get EBADF on input (and you can change that to EOF if you redirect from /dev/null). POSIX says behaviour of nohup input is up to the implementation - it might leave input from original terminal, or get it from a file.

On balance, I guess I'll say EBADF is what to do on read. This is a "standalone" FileHandle where it's automatically opened on construction, so we should assume that the implicit open is write-only, thus give EBADF on read attempts.

If there was a device filing system, so we were able to participate in the open call, we'd return ENXIO on the attempt to open for read.

This comment has been minimized.

@marcuschangarm

marcuschangarm Jan 29, 2018

Contributor

Thank you! I'll change it to EBADF.

This comment has been minimized.

@geky

geky Jan 29, 2018

Member

Lesson learned, looks like I have some filesystems to update...

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 30, 2018

Contributor

Ah, normal filesystems might be a bit different. Eg EACCES for access violation rather than ENXIO. I was only really thinking about devices, and particularly considering this odd case of "opened by construction" objects.

I just spend too much time Googling "POSIX open" or "POSIX read" to check POSIX specs for my own good...

This comment has been minimized.

@geky

geky Jan 30, 2018

Member

Well, in the open group docs for write:
http://pubs.opengroup.org/onlinepubs/9699919799/functions/write.html

[EBADF]
The fildes argument is not a valid file descriptor open for writing.

Pretty straightforward for POSIX docs actually. 😆

A quick test program confirms this is what Linux returns.

Fortunately reading/writing to a file after opening it in a specific mode is pretty uncommon outside of user errors. I'd be worried if someone was relying on a specific error code in that situation.

virtual ssize_t write(const void *buffer, size_t size) {
const unsigned char *buf = static_cast<const unsigned char *>(buffer);
for (size_t i = 0; i < size; i++) {
ITM_SendChar(buf[i]);

This comment has been minimized.

@kjbracey-arm

kjbracey-arm Jan 29, 2018

Contributor

This isn't in itm_api.h? Skipping a layer?

This comment has been minimized.

@marcuschangarm

marcuschangarm Jan 29, 2018

Contributor

ITM_SendChar is defined in:

cmsis/TARGET_CORTEX_M/core_cm3.h
cmsis/TARGET_CORTEX_M/core_cm4.h
etc.

I'm not sure how to explicitly include the right header file based on core.

This comment has been minimized.

@marcuschangarm

marcuschangarm Jan 29, 2018

Contributor

ah, #include "cmsis.h" should do the trick.

This comment has been minimized.

@0xc0170

0xc0170 Jan 30, 2018

Member

yes, cmsis should be used as a generic cmsis header file

@TeroJaasko

This comment has been minimized.

Contributor

TeroJaasko commented Jan 29, 2018

Works on my NRF52_DK. Redirecting the console is now really easy:

FileHandle *mbed::mbed_override_console(int fd) {
    static SingleWireOutput swo_serial;
    return &swo_serial;
}

int main() {
    while (true) {
        wait(0.5);
        printf("hello world\n");
    }
}
#if defined(DEVICE_ITM)
#include "hal/itm_api.h"

This comment has been minimized.

@pan-

pan- Jan 29, 2018

Member

I think you'll need to include platform/FileHandle.h.

This comment has been minimized.

@marcuschangarm

marcuschangarm Jan 29, 2018

Contributor

Thank you! I'll add the missing include.

@yogpan01

This comment has been minimized.

Contributor

yogpan01 commented Jan 29, 2018

@marcuschangarm Thanks for this ! @0xc0170 When can this be merged into MASTER. We are currently blocked on using nRF52 for our development because of this. We need this ASAP !
@JanneKiiskila @sg- FYI

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:feature-hal-swo branch from 61cd404 to 57dba11 Jan 29, 2018

@0xc0170

This comment has been minimized.

Member

0xc0170 commented Jan 30, 2018

@0xc0170 When can this be merged into MASTER

First, Filehandle PR needs to be integrated first (there are still some reviews + tests outstanding). As soon as that one is in, this should be updated to reflect the latest changes.

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:feature-hal-swo branch 2 times, most recently from 0070e34 to e65dc7b Jan 30, 2018

@geky

geky approved these changes Jan 31, 2018

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:feature-hal-swo branch 3 times, most recently from 69d8966 to 9fea19e Feb 1, 2018

@SenRamakri SenRamakri requested review from bulislaw, c1728p9 and SenRamakri Feb 2, 2018

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:feature-hal-swo branch from 9fea19e to 1d4a16d Feb 8, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Feb 8, 2018

Rebased with master to get #5571

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Feb 8, 2018

@0xc0170 @yogpan01

@0xc0170 When can this be merged into MASTER

First, Filehandle PR needs to be integrated first (there are still some reviews + tests outstanding). As soon as that one is in, this should be updated to reflect the latest changes.

FileHandle PR has been merged and this PR has been rebased. Now what? 😄

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Feb 9, 2018

/morph build

@kjbracey-arm

This comment has been minimized.

Contributor

kjbracey-arm commented Feb 9, 2018

@SenRamakri, @c1728p9, @bulislaw - dependency is now merged, so this could proceed. Please review.

I'm happy with the FileHandle end of this. Are people happy with the HAL API?

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 9, 2018

Build : SUCCESS

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

Triggering tests

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

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Feb 15, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 15, 2018

Build : SUCCESS

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

Triggering tests

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

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:feature-hal-swo branch from a0efeb1 to 0c1f2f7 Feb 15, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Feb 15, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 15, 2018

Build : SUCCESS

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

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.

marcuschangarm added some commits Jan 28, 2018

Instrumented Trace Macrocell (ITM) HAL API
HAL API for initializing the ITM and setting SWO debug output.
Actual debug output implemented as SWO FileHandle.

@marcuschangarm marcuschangarm force-pushed the marcuschangarm:feature-hal-swo branch from 0c1f2f7 to 954ae4a Feb 16, 2018

@marcuschangarm

This comment has been minimized.

Contributor

marcuschangarm commented Feb 16, 2018

/morph build

@mbed-ci

This comment has been minimized.

mbed-ci commented Feb 16, 2018

Build : SUCCESS

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

Triggering tests

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

@cmonr cmonr added needs: CI and removed ready for merge labels Feb 16, 2018

@mbed-ci

This comment has been minimized.

@mbed-ci

This comment has been minimized.

@cmonr cmonr merged commit 0ceecb9 into ARMmbed:master Feb 16, 2018

18 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
mbed-ci-generic Build finished.
Details
travis-ci/docs/ Local docs testing has passed
Details
travis-ci/events/ Local events testing has passed
Details
travis-ci/littlefs/ Local littlefs testing has passed
Details
travis-ci/mbed2-ATMEL/ Local mbed2-ATMEL testing has passed
Details
travis-ci/mbed2-MAXIM/ Local mbed2-MAXIM testing has passed
Details
travis-ci/mbed2-NORDIC/ Local mbed2-NORDIC testing has passed
Details
travis-ci/mbed2-NUVOTON/ Local mbed2-NUVOTON testing has passed
Details
travis-ci/mbed2-NXP/ Local mbed2-NXP testing has passed
Details
travis-ci/mbed2-SILICON_LABS/ Local mbed2-SILICON_LABS testing has passed
Details
travis-ci/mbed2-STM/ Local mbed2-STM testing has passed
Details

@marcuschangarm marcuschangarm deleted the marcuschangarm:feature-hal-swo branch Feb 17, 2018

@amq amq referenced this pull request Feb 27, 2018

Open

How to run tests over SWD? #475

SeppoTakalo added a commit to SeppoTakalo/Handbook that referenced this pull request May 29, 2018

Create itm.md
Documentation for the new ITM HAL API: ARMmbed/mbed-os#5956
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment