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

Stream::send() #6979

Merged
merged 328 commits into from
Mar 15, 2021
Merged

Stream::send() #6979

merged 328 commits into from
Mar 15, 2021

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Jan 3, 2020

edit:

fixes #6907 (datasource is replaced by stream) (comment)

edit:

References to "String is Stream" are removed:
An helper class S2Stream myStringVarNowIsAStream(myStringVar) has been added to make an already existing String into a Stream, and StreamString derives from it.

edit:

edit:

  • WiFiEcho server sketch example, with provided python echo client:
code BW (Kibits/s) min. free stack (Bytes)
byte by byte 539 2864
512 bytes stack buffer 4295 2212
Stream::to() 4980 2576

(these results are environment-dependent and have been made under the same conditions)

Additional notes:

  • flash strings or char arrays can be directly/generically streamed
  • ClientContext: DataSource is removed and replaced by Stream:: offering the same services: from Stream, from in-ram data, from in-flash data). That's also an additional reduction of allocation frequency.

Cost:

  • BasicHttpClient.ino: (less than 1KB in flash)

with master:

IROM   : 259112          - code in flash         (default or ICACHE_FLASH_ATTR) 
IRAM   : 27080   / 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...) 
DATA   : 1148  )         - initialized variables (global, static) in RAM/HEAP 
RODATA : 1212  ) / 81920 - constants             (global, static) in RAM/HEAP 
BSS    : 26408 )         - zeroed variables      (global, static) in RAM/HEAP 

w/ Stream::to PR:

IROM   : 260056          - code in flash         (default or ICACHE_FLASH_ATTR) 
IRAM   : 26904   / 32768 - code in IRAM          (ICACHE_RAM_ATTR, ISRs...) 
DATA   : 1148  )         - initialized variables (global, static) in RAM/HEAP 
RODATA : 1220  ) / 81920 - constants             (global, static) in RAM/HEAP 
BSS    : 26408 )         - zeroed variables      (global, static) in RAM/HEAP 

Ref: plerup/espsoftwareserial#171 replaced by #7840
Ref: bblanchon/ArduinoJson#1341

OP: (fixed)

This pull request aims at simplifying transfers between sources (Stream, String) and destinations (Print, String) in a single function in a single location.

This kind of transfer is indeed reimplemented everytime it is needed making the esp8266 arduino core (or any code using Stream) hard to optimize or maintain. There are no general functionality difference between any of these transfers.

The proposed implementation is in the form of "Stream::to*(Print)". This function relies on Arduino virtual Stream and Print API. The Arduino Stream API is historically limited so such transfers require the use of an intermediary buffer, implying two copies. This pull request also proposes an extension to the Stream API which allows to reduce the number of copies and avoid the use of such buffer. It is called in the following the "peekBuffer extension".

The peekBuffer extension is not a breaking change. It is by default not available (ie disabled in Stream::) and can be enabled by any stream class extender (bool Stream::peekBufferAPI()).

This pull request enables the Stream's peekBuffer API in WiFiClient::, WiFiClientSecureBearSSL::, HardwareSerial:: and StreamString::. The latter is optionally merged with String:: and this can be seen as a breaking change in some particular cases where libraries like ArduinoJson has overloads for Stream:: and String::.

At the time of writing, Stream's peekBuffer API has yet to be implemented in the filesystem classes (SDFat, LittleFS, SPIFFS).
Implementing it in external libraries such as EspSoftwareSerial should not induce changes in binary size or API compatibility with other cores.

Lots of places in the esp8266 arduino core would benefit from Stream::to(). This pull request proposes changes to ESP8266HTTPClient (it allowed to remove some big chunks of code including the interesting HTTPClient::writeToStreamDataBlock()). This change is (partly) validated by the device test test_sw_http_client (http, https) that can also be run-tested on host.

Some parts of ESP8266WebServer have been updated to use Stream::to(). Some examples too (Telnet2Serial, serial tests, ...) deserves the same changes.
A new example called WiFiEcho has been added to demonstrate the simplicity of the API.

About the functions Stream::to*(), they allow to

  • transfer with no timeout all immediately available data from a Stream to a Print
  • or, with a timeout, a specific amount of bytes
  • or, with a timeout, bytes until a char delimiter.

The timeout used is by default the stream's one and can be changed in arguments.
The peekBuffer API is used when available and avoids the use of an intermediary buffer with a twofold effect: speed and absence of any kind of stack/heap allocation.
When peekBuffer is not available, Stream::to() does the job using the legacy way.

@earlephilhower earlephilhower added this to the 3.0.0 milestone Feb 15, 2021
Copy link
Collaborator

@earlephilhower earlephilhower left a comment

Choose a reason for hiding this comment

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

LGTM now. Appreciate the clearer naming!

At this point it was found that wpos is above rpos, i.e.: the ringbuffer has
a single contiguous block, with possible free mem at the bottom and at the
top.
If at this point there is an interrupt that causes wpos to wrap, the
returned value below will be wrong, i.e.: some large number because of
negative result returned as size_t type.  Then, in SendGenericPeekBuffer(),
the to->write() call will crash.
@earlephilhower earlephilhower merged commit c720c0d into esp8266:master Mar 15, 2021
@earlephilhower
Copy link
Collaborator

Congrats, @d-a-v!

@blue-wind-25
Copy link

Hello, I would like to confirm my founding. It seems that the updated ESP8266WebServer (and maybe ESP8266WiFi) relies on the non32xfer service because:

template <typename ServerType>
void ESP8266WebServerTemplate<ServerType>::send_P(int code, PGM_P content_type, PGM_P content) {
StreamConstPtr ref(content, strlen_P(content));
return send(code, String(content_type).c_str(), &ref);
}


Then, because PGM_P is const char* then this CTOR will be called:
StreamConstPtr(const char* buffer, size_t size)
: _buffer(buffer), _size(size), _byteAddressable(__byteAddressable(buffer)) { }

which mean that the buffer will be read using non PGM method.


Also, there is another CTOR:
StreamConstPtr(const __FlashStringHelper* text)
: _buffer(reinterpret_cast<const char*>(text)), _size(strlen_P((PGM_P)text)), _byteAddressable(false) { }

It casts __FlashStringHelper to const char*. While strln_P() is correctly called, the buffer will be read using non PGM method.


Then pgmspace.h still defines PSTR as:

#ifndef PSTRN
#define PSTRN(s,n) (__extension__({static const char __pstr__[] __attribute__((__aligned__(n))) __attribute__((section( "\".irom0.pstr." __FILE__ "." __STRINGIZE(__LINE__) "." __STRINGIZE(__COUNTER__) "\", \"aSM\", @progbits, 1 #"))) = (s); &__pstr__[0];}))
#endif
#ifndef PSTR
#define PSTR(s) PSTRN(s,PSTR_ALIGN)
#endif

So it seems that PSTR strings still need to be accessed using the _P() functions.


I found that if I do not enable/include the non32xfer service then calls to web server send_P() will cause exception 3 (EXCCAUSE_LOAD_STORE_ERROR) inside the Arduino String class when copying the characters.

It seems also the case for SSL web server (even with non32xfer enabled, my SSL web server failed to send anything to the browser despite it does not cause exception 3). I am still unsure why this behavior with SSL happened.

I want to ask if this was by design (non32xfer must be enabled/included)?

I need to inform you that until at least the new official Arduino package is released, I will not be able to produce a test sketch because like I said to @earlephilhower in my other discussion, I do not use the Arduino build system.

Because I also follow updates from Arduino ESP8266 GIT and then modify the cores a bit to match my need and style, my application code will not compile using Arduino's original core. Therefore, I am also not sure if my comment about this is actually valid when using the original core and Arduino IDE. Thank you!

@earlephilhower
Copy link
Collaborator

@blue-wind-25 Can you cut-n-paste your observation into a new issue for tracking? A closed PR can easily get overlooked.

Thx!

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

Successfully merging this pull request may close these issues.

Stream File Problems 2.6.2
10 participants