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

Interface for alternative stdio retargeting #5356

Closed
wants to merge 3 commits into from
Closed

Interface for alternative stdio retargeting #5356

wants to merge 3 commits into from

Conversation

0x6d61726b
Copy link
Contributor

Description

This changes implement the mbed_retarget_alt.h interface that allows using an alternative/custom stdin/stdout/stderr interface other than the default serial port.
By setting MBED_CONF_RETARGET_STDIO_ALT = 1 the custom interface will be enabled.
An example implementation can be found in the 0x6d61726b/mbed-os-retarget-segger-rtt repository.

Todos

  • [?] Documentation
  • what else?

Deploy notes

?

Steps to test or reproduce

The following test program can be used for testing general functionality:

#include <stdio.h>
int main() {
    printf("-------------------------------------------------------------------------------\n");
    printf("Segger Real-Time Terminal ECHO program (type '.' to exit)\n");
    char c;
    while ((c = getchar()) != '.') {
        putchar(c);
    }
    
    printf("\n\nExit!\n");
    while (true);
}

Result (when typing Hello World!.):

demo

@theotherjimmy
Copy link
Contributor

@sg- could you take a look

@0xc0170
Copy link
Contributor

0xc0170 commented Oct 23, 2017

stdio retargeting 👍 , @0x6d61726b thanks for the bringing this up with a proposal

The default implementation should be the one that is currently used and a target needs to support. A question is how it should be provided? Can we find a better way than in this patch?

Should there be an API for stdin/out, default implementation using mbed serial as it is now, the overriden by a config setting?

Copy link
Contributor

@sg- sg- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont care for the implementation but the concept is a good one. Where is the JLINK implementaiton? Not sure we'd want a stub without the debugger support.

@bulislaw
Copy link
Member

Shall we move all the standard IO ops (or their actual backend implementation) to separate file implementing some set API and pull in specific implementation based on compile time flags? That would make it nicely abstracted and extensible (SWO?) and would get rid of all the ugly ifdefs.

@0x6d61726b
Copy link
Contributor Author

Where is the JLINK implementaiton?

@sg- the sample implementation is here: 0x6d61726b/mbed-os-retarget-segger-rtt

@theotherjimmy
Copy link
Contributor

@bulislaw did you mean to request changes?

@0x6d61726b Could you move all the standard IO ops to separate file as @bulislaw suggested?

@0x6d61726b
Copy link
Contributor Author

Could you move all the standard IO ops to separate file as @bulislaw suggested?

@theotherjimmy I don't know how it shall look like in the final implementation. Since I am not a firmware developement engineer, I may not be the right person for architecture changes.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 3, 2017

Shall we move all the standard IO ops (or their actual backend implementation) to separate file implementing some set API and pull in specific implementation based on compile time flags? That would make it nicely abstracted and extensible (SWO?) and would get rid of all the ugly ifdefs.

@0x6d61726b You created already declaration header file, this is 👍 . As quoted, if we can move those ifdef in the retarget file in this patch to own implementation (then retarget only invokes new API functions).
We would provide a default implementation in its own code file (retarget_stdio_default.cpp for instance can be a name, this would be the one that is current in retarget, using serial HAL). The default implementation would be protected via #if !MBED_CONF_RETARGET_STDIO_ALT as you did in retarget file (just all in one file), and if a user provides own file, would define this configuration.

One question is , can we add this segger implementation to the codebase? Having:

  • MBED_CONF_RETARGET_STDIO_SERIAL
  • MBED_CONF_RETARGET_STDIO_SEGGER_RTT
  • possible other debugger option

Would this make sense to have these stubs in place here and possible to use out of the box.

@0x6d61726b
Copy link
Contributor Author

0x6d61726b commented Nov 7, 2017

Ok, I will work on that on the upcoming weekend

One question is , can we add this segger implementation to the codebase?

That is a question I cannot answer. It may also be a legal issue?

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 8, 2017

That is a question I cannot answer. It may also be a legal issue?

I had a look at those files previously, they are released with 3-Clause BSD License so do not see a problem adding them. Let's first complete the API phase, and add the additional implementation right after

@sg-
Copy link
Contributor

sg- commented Nov 8, 2017

You can always create a class that inherits from Stream and them implement a claim function making that call in mbed_main or a constructor if needed.

https://os.mbed.com/users/Sissors/code/MODSERIAL/file/a3b2bc878529/MODSERIAL.cpp

@0x6d61726b
Copy link
Contributor Author

0x6d61726b commented Nov 16, 2017

extern "C" int PREFIX(_read)(FILEHANDLE fh, unsigned char *buffer, unsigned int length, int mode) {
...
// only read a character at a time from stdin

Can somebody explain why there should only one character being read at a time, please?
Is this related to the serial interface only or a general requirement? Further down when reading from fh >= 3, a total length of length can be read.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 17, 2017

Can somebody explain why there should only one character being read at a time, please?
Is this related to the serial interface only or a general requirement? Further down when reading from fh >= 3, a total length of length can be read.

I would assume that is a reason, serial_getc() reads only one character (no block read available)

@marcuschangarm
Copy link
Contributor

@0xc0170

One question is , can we add this segger implementation to the codebase? Having:
MBED_CONF_RETARGET_STDIO_SERIAL
MBED_CONF_RETARGET_STDIO_SEGGER_RTT
possible other debugger option
Would this make sense to have these stubs in place here and possible to use out of the box.

I would suggest adding SWO as one of the out of the box options. We already have CMSIS support for it: https://github.com/ARMmbed/mbed-os/blob/master/cmsis/TARGET_CORTEX_M/core_cm4.h#L2042-L2062

The only thing missing is the per platform initialization of the output pin and clock frequency.

@kjbracey
Copy link
Contributor

This is a major overlap with something I've been hoping to do a while, which is to allow stdin/stdout/stderr to be mapped to a normal FileHandle.

At the minute it is advantageous to do

 UARTSerial serial(USBTX, USBRX);
 FILE *my_out = fdopen(&serial, "w");
 FILE *my_in = fdopen(&serial, "r");

And use my_out and my_in instead of stdout and stdin, as you get buffering and better sleep characteristics for output, plus the input actually works reliably because of the buffering. Some apps are starting to do that now, but it would be nice if stdout and stdin could just automatically be a UARTSerial via a config option to save the hassle.

So I'm not sure we need to add a new special custom "FileHandle-lite" mechanism to retarget - it would make more sense to just let stdin/stdout/stderr be FileHandles, then users can implement FileHandle for whatever custom I/O device they have. Then that I/O device can be used either for stdout, or for any other manually-specified stream.

You can implement FileHandle now, and use it with the C library as shown above, you just have to use fprintf, fgetc, fputc instead of printf, getchar and putchar.

@kjbracey
Copy link
Contributor

I'm preparing a PR alternative to this which regularises the system, and should achieve the desired result more flexibly.

The default raw serial_getc/serial_putc behaviour will become an internal FileHandle that does that, rather than a special case, and there will be a simple config option to have stdin/stdout/stderr use UARTSerial instead, plus a mechanism to provide an alternative FileHandle.

A minimal FileHandle implementation is just read, write and dummy seek and close - effectively the same as implementing that retarget_alt, but in C++.

@kjbracey
Copy link
Contributor

kjbracey commented Nov 23, 2017

See #5571 - @0x6d61726b, do you think that would serve your purpose? As it stands, your target would implement an mbed_target_override_console that overrides the weak default, and provide a FileHandle.

0x6d61726b added a commit to 0x6d61726b/mbed-os-retarget-segger-rtt that referenced this pull request Dec 17, 2017
Implemented FileHandle for SeggerRTT and registration of stdio according to discussion in ARMmbed/mbed-os#5356
@0x6d61726b
Copy link
Contributor Author

0x6d61726b commented Dec 17, 2017

@kjbracey-arm: I updated my sample implementation 0x6d61726b/mbed-os-retarget-segger-rtt with the functionality you implemented in #5571 and it works perfectly.

@kjbracey
Copy link
Contributor

Excellent! Good to see it worked for you.

#5571 is still awaiting review though, so we'll have to see how that goes.

Looking at your example, on the assumption that #5571 goes through as-is, it seems this is basically target-independent code, so you shouldn't be overriding mbed_target_override_console - my intent was that that is for the platform itself to set the default target-specific console.

Rather, as this is a somewhat-portable "support" library that apps can include, you should permit two ways of using it:

  1. Allow users to create a SeggerRTT themselves, and use it manually.

    SeggerRTT segger;
    FILE *segger_out = fdopen(&segger, "w");
    fprintf(segger, "Hello!\n");
    

Then they can use both the serial and segger output.

  1. Have a config option for your library in an mbed_lib.json to make it the default console - you provide mbed_override_console if your MBED_CONF_SEGGER_RTT_OVERRIDE_CONSOLE is true. I'm torn on whether this should be true by default - not sure if it's good or not that adding a library to the build automatically switches the console. But it should be an option, just in case someone only wants to use the Segger for some specific data, without replacing the serial console.

Using mbed_override_console makes sure your override wins if a particular target already had a custom console like an LCD screen set with mbed_target_override_console.

Obviously apps could provide an mbed_override_console to a SeggerRTT themselves, but it's convenient for the SeggerRTT library to do that for them.

Note that it's not worth checking the fd parameter in the override if you're overriding everything - it's defined as only being called for stdin, stdout and stderr.

Another thing to be aware of - neither your original suggestion nor the #5571 override will redirect the output from error or MBED_ASSERT. They're hard-coded to go via the serial port. That's to attempt to make sure they work from weird exception & interrupt conditions. With care after #5571 we could maybe permit them to do write(STDERR_FILENO) for a custom console, if the FileHandle in question promised to behave well in such conditions. Something for more thought.

@cmonr
Copy link
Contributor

cmonr commented Jan 17, 2018

@0x6d61726b, is it safe to say that this PR now depends on #5571, as per #5571 (comment) ?

@mbed-ci
Copy link

mbed-ci commented Jan 17, 2018

Automatic CI verification build not done, please verify manually.

0x6d61726b added a commit to 0x6d61726b/mbed-os-retarget-segger-rtt that referenced this pull request Jan 27, 2018
mbed_target_override_console replaced with mbed_override_console according to [comment from kjbracey-arm](ARMmbed/mbed-os#5356 (comment))
@0x6d61726b
Copy link
Contributor Author

@kjbracey-arm, I changed my demo code according to your comment using the second option. I may have misunderstood the meaing of both override functions.

@cmonr, it is correct, that this implementation relies on the code of #5571.

@kjbracey
Copy link
Contributor

@0x6d61726b I think I misled you originally - I misunderstood that your code was going into some target-specific area, so told you to use the target override.

@kjbracey
Copy link
Contributor

kjbracey commented Jan 29, 2018

@cmonr note that the code currently in the PR itself is standalone and not what we want because #5571 allows a different approach - @0x6d61726b has put the #5571-based version on a separate branch.

@0x6d61726b - for your branch, please re-read my full proposal above. I don't think as it stands this is flexible enough - including the library automatically makes stdin/stdout go to SeggerRTT. While that's "easy", and maybe is a good default (and what your original did), there should still be a way to use this device separately without forcibly replacing the normal target stdout. #5571 makes that easy.

@kjbracey
Copy link
Contributor

kjbracey commented Feb 8, 2018

#5571 has now been merged and will be in mbed OS 5.8 - is it okay to close this, and you will use that interface?

@0xc0170
Copy link
Contributor

0xc0170 commented Feb 19, 2018

#5571 has now been merged and will be in mbed OS 5.8 - is it okay to close this, and you will use that interface?

@0x6d61726b Please reopen if its still relevant

@0x6d61726b
Copy link
Contributor Author

0x6d61726b commented Feb 21, 2018

@kjbracey-arm Thanks a lot for merging it to mbed OS 5.8 - I will use that interface because it helps me a lot for debugging in custom hardware. I have already merged it into my local copy of OS 5.7 where it is in use.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants