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

Print.flush() - Arduino API conformance #6084

Merged
merged 1 commit into from
Jan 17, 2022
Merged

Print.flush() - Arduino API conformance #6084

merged 1 commit into from
Jan 17, 2022

Conversation

JAndrassy
Copy link
Contributor

In ARDUINO 1.0 (2011.11.30) Arduino changed Serial.flush() to "wait for outgoing data to be transmitted". In following years other classes derived from Stream (or Print) implemented flush() as "send the buffer out". In ARDUINO 1.8.4 (2017.08.23) they moved flush() from Stream to Print in the prominent Arduino AVR core. For backward compatibility of flush() in Print they changed the pure virtual function to empty implementation. The ArduinoCore-API recommended for new cores or major updates has flush() in Print too.

Most major cores already implemented this change. This PR moves flush() from Stream.h to Print.h in this core.

@CLAassistant
Copy link

CLAassistant commented Dec 30, 2021

CLA assistant check
All committers have signed the CLA.

@bxparks
Copy link
Contributor

bxparks commented Jan 12, 2022

Thank you, I was about to create a PR to do this exact thing. The missing Print::flush() on the ESP32 core has been a minor annoyance to me for several years. I've been doing things like this to get around this inconsistency:

class Subclass : public Print {
  public:
  #if defined(ESP32)
    void flush() {...}
  #else
    void flush() override {...}
  #end

[...]
};

But I still cannot use Subclass::flush() polymorphically through the Print pointer or reference on the ESP32.

For reference, I am aware of the following Cores which do implement a virtual void Print::flush():

I am aware of only these Cores which do not implement a Print::flush():

As far as I can tell, there are only 3 classes in the ESP32 core which inherit directly from Print. The rest inherit from Stream which will not be affected by this PR because Stream already defines a virtual Stream::flush().

  • cores/esp32/Server.h
  • libraries/AsyncUDP/src/AsyncUDP.h (the AsyncUDPMessage class)
  • libraries/USB/src/USBHIDKeyboard.h

The result is that these classes will consume a handful more bytes of flash memory, and maybe 4 extra bytes of static memory for the extra slot in the vtable? I hope this will not be the reason for rejecting this PR.

[Edit: fixed effected -> affected]

@me-no-dev me-no-dev merged commit e84e9c1 into espressif:master Jan 17, 2022
@me-no-dev
Copy link
Member

Thanks!

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.

4 participants