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

mbed requires use of printf/scanf through any use of Stream #4830

Closed
fahhem opened this issue Jul 30, 2017 · 3 comments
Closed

mbed requires use of printf/scanf through any use of Stream #4830

fahhem opened this issue Jul 30, 2017 · 3 comments

Comments

@fahhem
Copy link
Contributor

fahhem commented Jul 30, 2017

Description

  • Type: Enhancement
  • Priority: Minor

Enhancement

Reason to enhance or problem with existing solution
scanf/printf can be heavyweight (nanolib takes >2.5kb in flash) and hard to optimize even when only used once due to source->static lib->source traversal.

When you use a Stream subclass (like Serial), here's what happens:
Stream::Stream() calls fdopen->mbed_fdopen

In mbed_retarget.cpp, mbed_fdopen() calls sprintf just to do ":%p" % this, then calls std::fopen, which eventually leads to _open, which calls sscanf to rip the pointer out of ":%p".

Suggested enhancement
Option A: optimize these specific uses of printf/scanf out.
printf: A simpler (computationally) way to calculate a buf that will be checked by _open is:

// Around line 828
std::FILE *mbed_fdopen(FileHandle *fh, const char *mode) {
    // char buf[12]; /* :0x12345678 + null byte */
    // std::sprintf(buf, ":%p", fh);
    char buf[2 + sizeof(fh) + 1]; 
    static_assert(sizeof(buf) == 7, "Pointers aren't 4 bytes?");
    buf[0] = ':';
    memcpy(buf + 1, &fh, sizeof(fh));
    buf[1 + sizeof(fh)] = '\0';

// Around line 178
extern "C" FILEHANDLE PREFIX(_open)(const char* name, int openmode) {
...
// Around line 232
      if (name[0] == ':') {
        void *p;
        // std::sscanf(name, ":%p", &p);
        memcpy(&p, name + 1, sizeof(p));
        res = (FileHandle*)p;

This saves ~3.3kB from my program, though I still have these symbols in my Map and I can't trace them any further: svfprintf, vfprintf, fseek, setvbuf.

Option B: Allow Serial to not support file-like operations (via define?, or a SerialLite?)
To compare apples to apples, I first wrote the following code to ensure I was linking everything in:

mbed::Callback<void(int)> null_cb(nullptr);
char buf[80];
int written = std::sprintf(buf, "Some stuff: %d", 9);
serial.write((uint8_t*)buf, written, null_cb));

This uses the async write() code since the synchronous write() is implemented by Stream. Then I removed Stream as a base class of Serial in both Serial.h and Serial.cpp. Compiling before and after that base class removal, I got a 5kB shrink in text (and 300 in bss because mbed_retarget's filehandles aren't linked in anymore).

Pros
Both options can be done, though B is more intrusive and more work as it seems the purpose of Stream was to capitalize on the stdlib's implementations of fprintf, fflush, etc. However, for more production-worthy systems, being able to use Serial ports without linking in string-manipulation libraries and/or file-manipulation libraries means being able to put more code in the same microcontroller or to cost-down and use one with less flash (and less ram, etc).

Option A:

  • Save ~3kB in firmware not using scanf elsewhere

Option B:

  • Save ~5kB in firmware not using file-like abilities on Serial.
  • Since Stream::printf calls fprintf synchronously, but SerialBase::write is an async write, adding/moving Stream::write to SerialBase or a new StreamLitewould give Serial a synchronous write that doesn't link in file handling.

To be clear, I haven't thought through Option B, partly because I don't know mbed's long-term vision for Serial, Stream, etc. I just know that ripping out file handling code brings my blinky+serial code down to ~15kB from ~23kB. While that's still really large for the functionality, it's mostly UART and RCC related plus 4 printf-related functions that I'll figure out some other way.

Cons
Option A:

  • The filename string is less human-readable (not really a problem)
  • Slightly more code to use memcpy over sscanf and sprintf.

Option B:

  • This requires more thinking/decisions and a lot more work than what I've done here. If you're okay with more defines, I can make one to just rip out Stream as a subclass, but that seems hacky.

Conclusion

I'll send a PR for Option A to discuss the exact implementation since that seems reasonable. For Option B, I'd like to hear your thoughts and see what path you would prefer.

@geky
Copy link
Contributor

geky commented Jul 31, 2017

Another option to consider:

  • Could we rearrange the Stream class (and FileHandle interface?) to only pull in the printf code when Stream::printf is used?

I was looking into this a while back, if you moved the all of the ties into printf into the Stream::printf function, it's theoretically possible for the compiler to drop the function when it's not used. I had a messy prototype put together, but haven't had the time to get it merge ready:
geky/mbed@04a31f3...71618a4


To hopefully explain some of mbed's long-term direction, the biggest obstacle for mbed is backwards compatibility. We do our best to avoid breaking existing APIs without marking the APIs as deprecated for several releases. For this reason the development around the Stream class has been slow, and we're thinking the best plan is to move onto a different interface, potentially deprecating the Stream class in the future.

Currently, we are looking at moving over the FileHandle interface for stream operations. As is, FileHandle is a minimal stream interface that doesn't bring in the printf/scanf code, while still being compatible with the stdio retargeting code. We currently have the classes RawSerial and UARTSerial, which are intended to replace Serial while only inheriting from FileHandle. If the seperate printf/scanf functions work out, they will probably be added to the FileHandle class.

Unfortunately we haven't begun deprecating the Stream classes since we don't yet have a complete replacement. Deprecating the Stream class will also require deprecating the Serial class, which is a heavily used class.

Sorry that I don't have a more straightforward explanation, it is quite a mess right now. Your pr will be a great addition for some immediate savings. For new code it would be a good idea to adopt FileHandle and RawSerial/UARTSerial where possible.
[Mirrored to Jira]

@adbridge
Copy link
Contributor

adbridge commented Oct 4, 2018

Internal Jira reference: https://jira.arm.com/browse/IOTCORE-127

@ciarmcom
Copy link
Member

Thank you for raising this issue. Please note we have updated our policies and
now only defects should be raised directly in GitHub. Going forward questions and
enhancements will be considered in our forums, https://forums.mbed.com/ . If this
issue is still relevant please re-raise it there.
This GitHub issue will now be closed.

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

No branches or pull requests

5 participants