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

Feature/http client wi fi client parameter #4979

Closed
wants to merge 15 commits into from
Closed

Feature/http client wi fi client parameter #4979

wants to merge 15 commits into from

Conversation

Jeroen88
Copy link
Contributor

This PR replaces #4825.

Adaptation to HTTPClient as suggested by @igrr on April 10 in PR #4273 and discussed with @earlephilhower by e-mail. Also the review comments by @earlephilhower are processed in this PR.

With this PR it is possible to use any of the BearSSL security methods, by first preparing the WiFiClient with any of these methods, and next passing the prepared WiFiClient into one of the new begin() functions.

The main changes are:

  1. Introduction of 2 begin() functions, passing in WiFiClient& as a parameter, to make HTTPClient suitable for any WiFiClientSecureBearSSL security method
  2. The present API is kept to maintain backward compatibility, however can be set deprecated.
  3. All code that belongs to the present API is wrapped inside a #ifdef KEEP_PRESENT_API, so in a future release can be easily removed
  4. A https example with Maximum Fragment Length Negotiation (MFLN) is added. If MFLN is supported by the server, the ESP8266 client needs for a SSL/TLS connection are significantly reduced. MFLN may be part of the upcoming LTS release of OpenSSL 1.1.1. The URL in the example already supports MFLN.

Copy link
Member

@igrr igrr left a comment

Choose a reason for hiding this comment

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

I think this PR is a very good change! One more request, please bump the version of HTTPClient library in library.properties to 1.2. Then we can disable deprecated functions and bump version to 2.0 for the next major release.

@@ -40,34 +42,39 @@ void loop() {
// wait for WiFi connection
if ((WiFiMulti.run() == WL_CONNECTED)) {

WiFiClient client;

HTTPClient http;

USE_SERIAL.print("[HTTP] begin...\n");
// configure traged server and url
//http.begin("https://192.168.1.12/test.html", "7a 9c f4 db 40 d3 62 5a 6e 21 bc 5c cc 66 c8 3e a1 45 59 38"); //HTTPS
Copy link
Member

Choose a reason for hiding this comment

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

let's remove the comment, maybe it's better to add "BasicHTTPSClient.ino" instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing comment and adding a BasicHTTPSClient.ino with fingerprint security check


void setup() {

USE_SERIAL.begin(115200);
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure that Serial1 is such a common use case to justify USE_SERIAL. I think using Serial makes for more obvious examples which are easier to copy-paste and combine. (This applied to many existing examples, just wanted to give an opinion.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

USE_SERIAL was already present in the examples of the current 1.1 version of the library. Replacing USE_SERIAL with Serial in all examples


bool mfln = client.probeMaxFragmentLength("tls.mbed.org", 443, 1024);
USE_SERIAL.printf("\nConnecting to https://tls.mbed.org\n");
USE_SERIAL.printf("MFLN supported: %s\n", mfln ? "yes" : "no");
Copy link
Member

Choose a reason for hiding this comment

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

MFLN is not a commonly used acronym, suggest spelling out "Maximum fragment length negotiation".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacing MFLN with "Maximum fragment length negotiation"

int len = http.getSize();

// create buffer for read
uint8_t buff[128] = { 0 };
Copy link
Member

Choose a reason for hiding this comment

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

Suggest moving this from stack to global static. I can see how a user might think "i need to make this a bit bigger" and get a stack overflow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing into static uint8_t buff[128]

std::unique_ptr<WiFiClient> _tcp;
std::unique_ptr<WiFiClient> _tcpDeprecated;
#endif
WiFiClient* _tcp;
Copy link
Member

Choose a reason for hiding this comment

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

Suggest _client — this is not necessarily (only) TCP, it can also be providing TLS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, TLS runs over TCP. :) DTLS is required for UDP-level secure sockets...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changing _tcp into _client

#include <StreamString.h>
#include <base64.h>

#include "ESP8266HTTPClient.h"

#ifdef KEEP_PRESENT_API
Copy link
Member

Choose a reason for hiding this comment

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

Suggest making KEEP_PRESENT_API less ambiguous, for example HTTPCLIENT_1_1_COMPATIBLE (referring to current 1.1 version of the library).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Chaning KEEP_PRESENT_API into HTTPCLIENT_1_1_COMPATIBLE

bool begin(WiFiClient &client, String url);
bool begin(WiFiClient &client, String host, uint16_t port, String uri = "/", bool https = false);

#ifdef KEEP_PRESENT_API
Copy link
Member

Choose a reason for hiding this comment

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

Please mark the functions wrapped in KEEP_PRESENT_API block with __attribute__((deprecated))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I'm not sure what purpose the #define macro serves. We're trying to add a new begin() method and deprecate older ones, so shouldn't they all be present in all builds (today), with the deprecated ones disappearing in a later full-rev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding attribute ((deprecated)) to all marked functions.

@earlephilhower KEEP_PRESENT_API (now changed into HTTPCLIENT_1_1_COMPATIBLE) serves a few goals for me:

  • Now that I am working on the library and fully understand it, I am already writing the final, full-rev code. The only thing needed for the full-rev is simply removing everything marked within the #ifdef HTTPCLIENT_1_1_COMPATIBLE ... #endif. I already tested the library without #define HTTPCLIENT_1_1_COMPATIBLE
  • If someone wants to compile the library without the #define it is possible, thus saving memory
  • By maintaining backward compatibility, it is possible to merge this library into a minor release, but, as already said, it is fully prepared for a major release

return false;
}

_port = (protocol == "https" ? 443 : 80);
Copy link
Member

Choose a reason for hiding this comment

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

port may be given in the URL (https://site.company.com:55566/), suggest at least adding a comment here that parsing the port is not implemented yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

port is implemented. In line 164 _port is set to 443 or 80, depending on https or not. In the next line, beginInternal() is called. This sets _port to the value provided in the URL,if any is provided.

@@ -100,6 +104,7 @@ class BearSSLTraits : public TransportTraits
protected:
uint8_t _fingerprint[20];
};
#endif
Copy link
Member

Choose a reason for hiding this comment

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

When wrapping a large block of code (especially more than one screeful) with ifdef, suggest adding a comment on the line with #endif, e.g. #endif // KEEP_PRESENT_API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding "// HTTPCLIENT_1_1_COMPATIBLE" 3 times

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.

Love the update, but I'm having trouble with the ifdef existence and worried about some confusion w/the two parallel _tcp* vars (which are a real pain, I get it!).

} else {
USE_SERIAL.printf("[HTTP] GET... failed, error: %s\n", http.errorToString(httpCode).c_str());
USE_SERIAL.printf("Unable to connect\n");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor thing, but suggest adding "[HTTP]" to this message to match formatting of other outputs in the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding [HTTP] in basicHttpClient.ino and [HTTPS] in the new basicHttpsClient.ino

HTTPClient http;

USE_SERIAL.print("[HTTP] begin...\n");
// configure traged server and url


http.begin("http://user:password@192.168.1.12/test.html");
http.begin(client, "http://guest:guest@jigsaw.w3.org/HTTP/Basic/");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice test, moving it to a public web service!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

:)

//http.begin("192.168.1.12", 80, "/test.html");
WiFiClient client;

http.begin(client, "http://192.168.1.12/test.html");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe move this to jigsaw as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving to jigsaw


HTTPClient http;

BearSSL::WiFiClientSecure client;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Reinforcing @igrr's comment below, the SSL client object is pretty big and using BearSSL::WiFiClientSecure *client = new BearSSL::WiFiClientSecure ; would help new users. Obviously there will be a delete client; somewhere later on...

Copy link
Contributor Author

@Jeroen88 Jeroen88 Aug 20, 2018

Choose a reason for hiding this comment

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

@earlephilhower I tried to use new and delete, however I get a stack dump (zero pointer reference with a delete). Is it possible that BearSLL destructor is causing the problem?
Stack decode:
Decoding stack results
0x401006dc: free at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/cores/esp8266/umm_malloc/umm_malloc.c line 1755
0x4020539c: std::__shared_ptr ::operator=(std::__shared_ptr &&) at /home/jeroen/.arduino15/packages/esp8266/tools/xtensa-lx106-elf-gcc/1.20.0-26-gb404fb9-2/xtensa-lx106-elf/include/c++/4.8.2/bits/shared_ptr_base.h line 862
0x4010020c: _umm_free at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/cores/esp8266/umm_malloc/umm_malloc.c line 1295
0x401006dc: free at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/cores/esp8266/umm_malloc/umm_malloc.c line 1755
0x40207ca0: operator delete(void*) at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/cores/esp8266/abi.cpp line 54
0x40201592: delay at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/cores/esp8266/core_esp8266_wiring.c line 54
0x402055c4: BearSSL::WiFiClientSecure::~WiFiClientSecure() at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/libraries/ESP8266WiFi/src/WiFiClientSecureBearSSL.cpp line 110
0x402028d9: loop() at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/libraries/ESP8266HTTPClient/examples/StreamHttpsClient/StreamHttpsClient.ino line 114
0x40207ebc: esp_yield() at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/cores/esp8266/core_esp8266_main.cpp line 91
0x40203b2c: ESP8266WiFiMulti::addAP(char const*, char const*) at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/libraries/ESP8266WiFi/src/ESP8266WiFiMulti.cpp line 39
0x40207f48: loop_wrapper() at /home/jeroen/Downloads/arduino-PR-beta1.9-BUILD-69/hardware/esp8266com/esp8266/cores/esp8266/core_esp8266_main.cpp line 125

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're getting that kind of crash, you've probably got a use-after-free problem in the code you changed. Without the source that is crashing, can't really get deeper than that. The constructor/destructor is pretty solid and been used with new/delete especially due to the size of the object involved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@earlephilhower I am almost sure that the problem is in WiFiClientSecureBearSSL.cpp.

This is the code that crashes:

/**
StreamHTTPClient.ino

Created on: 24.05.2015

*/

#include <Arduino.h>

#include <ESP8266WiFi.h>
#include <ESP8266WiFiMulti.h>

#include <ESP8266HTTPClient.h>

ESP8266WiFiMulti WiFiMulti;

void setup() {

Serial.begin(115200);
// Serial.setDebugOutput(true);

Serial.println();
Serial.println();
Serial.println();

for (uint8_t t = 4; t > 0; t--) {
Serial.printf("[SETUP] WAIT %d...\n", t);
Serial.flush();
delay(1000);
}

WiFi.mode(WIFI_STA);
WiFiMulti.addAP("SSID", "PASSWORD");

}

void loop() {
// wait for WiFi connection
if ((WiFiMulti.run() == WL_CONNECTED)) {

HTTPClient http;

// BearSSL::WiFiClientSecure client;
BearSSL::WiFiClientSecure *client = new BearSSL::WiFiClientSecure ;

// bool mfln = client.probeMaxFragmentLength("tls.mbed.org", 443, 1024);
bool mfln = client->probeMaxFragmentLength("tls.mbed.org", 443, 1024);
Serial.printf("\nConnecting to https://tls.mbed.org\n");
Serial.printf("Maximum fragment Length negotiation supported: %s\n", mfln ? "yes" : "no");
if (mfln) {
// client.setBufferSizes(1024, 1024);
client->setBufferSizes(1024, 1024);
}

Serial.print("[HTTPS] begin...\n");

// configure server and url
const uint8_t fingerprint[20] = {0xEB, 0xD9, 0xDF, 0x37, 0xC2, 0xCC, 0x84, 0x89, 0x00, 0xA0, 0x58, 0x52, 0x24, 0x04, 0xE4, 0x37, 0x3E, 0x2B, 0xF1, 0x41};

// client.setFingerprint(fingerprint);
client->setFingerprint(fingerprint);

// if (http.begin(client, "https://tls.mbed.org/")) {
if (http.begin(*client, "https://tls.mbed.org/")) {

  Serial.print("[HTTPS] GET...\n");
  // start connection and send HTTP header
  int httpCode = http.GET();
  if (httpCode > 0) {
    // HTTP header has been send and Server response header has been handled
    Serial.printf("[HTTPS] GET... code: %d\n", httpCode);

    // file found at server
    if (httpCode == HTTP_CODE_OK) {

      // get lenght of document (is -1 when Server sends no Content-Length header)
      int len = http.getSize();

      // create buffer for read
      static uint8_t buff[128] = { 0 };

      // get tcp stream

// WiFiClient * stream = &client;
WiFiClient * stream = client;

      // read all data from server
      while (http.connected() && (len > 0 || len == -1)) {
        // get available data size
        size_t size = stream->available();

        if (size) {
          // read up to 128 byte
          int c = stream->readBytes(buff, ((size > sizeof(buff)) ? sizeof(buff) : size));

          // write it to Serial
          Serial.write(buff, c);

          if (len > 0) {
            len -= c;
          }
        }
        delay(1);
        yield(); // ADDED
      }

      Serial.println();
      Serial.print("[HTTPS] connection closed or file end.\n");

    }
  } else {
    Serial.printf("[HTTPS] GET... failed, error: %s\n", http.errorToString(httpCode).c_str());
  }

  http.end();
} else {
  Serial.printf("Unable to connect\n");
}

delete client;

}

delay(10000);
}

I nailed the problem down to the destructor ~HTTPClient() in ESP8266HTTPClient. This calls _client->stop().
In WiFiClientSecure::stop() the function _clearAuthenticationSettings() is called. This function clears the pointers _sk and _ta. Next the destructor ~WiFiClientSecure() is called. This calls delete _sk and delete _ta (if _deleteChainKeyTA is true, which I asume is) but these pointers were cleared already.
If I skip _client->stop() in ~HTTPClient() in ESP8266HTTPClient everything works fine.

}

String protocol = url.substring(0, index);
if(protocol != "http" && protocol != "https") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this compiles w/-Wall cleanly, fine, but I think the compiler may complain unless there are parenthesis around the comparisons. Very minor, and the order of precedence means you're perfectly safe doing it your present way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it compiles without the parenthesis

* @return success bool
*/
bool HTTPClient::begin(WiFiClient &client, String url) {
#ifdef KEEP_PRESENT_API
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you please add a comment to the header/examples about the fact that this now takes a pointer to an object, and as such the user needs to ensure the object lives the entire time of the HTTPClient? You're doing the right thing (only thing possible since this class doesn't know the size of the object!), but many Arduino APIs pass in copies of the actual client object and so don't have this restriction.

No code change, just a little note in the example or header would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a comment in ESP8266HTTPClient.h

@@ -631,7 +709,11 @@ int HTTPClient::getSize(void)
WiFiClient& HTTPClient::getStream(void)
{
if(connected()) {
#ifdef KEEP_PRESENT_API
Copy link
Collaborator

Choose a reason for hiding this comment

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

For this and the following methods, you're using #ifdef KEEP_PRES_API to bracket which value to return. But, that's not actually right as, when that define is enabled (i.e. default case) both _tcpDeprecated and _tcp can be valid and you need to identify/keep track of which one is really to be used. Needs to be some sort of flag (@devyte would say that's a code smell and he's right, but I don't know of a simpler way) identifying which one of the two possible values should be used.

I think this change, as-is, will either end up breaking existing (unmodified) code or not really return the same thing when the new constructor is used.

It's a real pain in the butt, this backwards compatibility...

Copy link
Contributor Author

@Jeroen88 Jeroen88 Aug 20, 2018

Choose a reason for hiding this comment

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

I did not realise that both _tcpDeprecated and _tcp (changed into _client) could be true, but you are right...! I think you identified an even more major problem, if someone is mixing up both types of begin() functions. This problem disappears after the removal of the deprecated functions.
To stay backward compatible, I added a call to the end() function at the beginning of every begin() function. I also set _canReuse to false if someone tries to reuse the connection created on _client with _tcpDeprecated (just to be sure :)). If there is a connection, and it is not reused, the end() function closes the connection, on either _client OR _tcpDeprecated and resets the pointer. In the constructor both pointers are reset. This ensures that either NO connection is made and both pointers are NULL or ONLY ONE of the pointers _client or _tcpDeprecated has a connection. I did however NOT TEST it thoroughly, I tested it with StreamHttpsClient.ino and it works fine, but I did not test it with the reuse of a connection nor with a deprecated begin(). @earlephilhower could you please review the constructor, begin() and end() functions? And I fully agree, it is a pain... :)

bool begin(WiFiClient &client, String url);
bool begin(WiFiClient &client, String host, uint16_t port, String uri = "/", bool https = false);

#ifdef KEEP_PRESENT_API
Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, I'm not sure what purpose the #define macro serves. We're trying to add a new begin() method and deprecate older ones, so shouldn't they all be present in all builds (today), with the deprecated ones disappearing in a later full-rev?

std::unique_ptr<WiFiClient> _tcp;
std::unique_ptr<WiFiClient> _tcpDeprecated;
#endif
WiFiClient* _tcp;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, TLS runs over TCP. :) DTLS is required for UDP-level secure sockets...

Jeroen88 added 2 commits August 20, 2018 12:31
…tion that only one connection is made, updated version to 1.2 deprecating present begin() functions, several minor updates
@Jeroen88
Copy link
Contributor Author

Jeroen88 commented Aug 20, 2018

Travis test failed, because functions used in the examples are set deprecated in ESP8266HTTPClient

@devyte
Copy link
Collaborator

devyte commented Aug 21, 2018

When an old api is deprecated in favor of a new one, the examples need to be updated to the new api. In other words, examples should never use deprecated functions/methods/objects.

@Jeroen88
Copy link
Contributor Author

@devyte thnx for your reaction. I read the Travis log better this time: the problem is not the examples, I upgraded them all to the new api. The problem is in the library ESP8266HTTPClient itself. A deprecated function calls two other deprecated functions. This is the function definition causing the problem:

bool HTTPClient::begin(String host, uint16_t port, String uri, bool https, String httpsFingerprint)
{
    if (https) {
        return begin(host, port, uri, httpsFingerprint);
    } else {
        return begin(host, port, uri);
    }
}

The defined begin() function itself and both called begin() functions are all deprecated. I think this should be perfectly legal! After all, every function is deprecated. Only calling a deprecated function from a non-deprecated function should be illegal.
Do you know a way how to fix this and not having the Travis tests complain about this? The only way I can imagine is to duplicate the code of the two called begin() functions, into the function definition, but this seems overkill to me.

@igrr
Copy link
Member

igrr commented Aug 21, 2018

That's not a Travis issue, just GCC doing what it was instructed to do — warn whenever a deprecated function is used. To avoid this you can wrap the problematic function with pragma push/pop, suppressing the warning.

@d-a-v
Copy link
Collaborator

d-a-v commented Aug 21, 2018

What about using -Wno-deprecated-declarations in CI ?
I made a patch (tested locally)

@igrr
Copy link
Member

igrr commented Aug 21, 2018

That would defeat the purpose of CI check, that examples should not use deprecated functions.

@devyte
Copy link
Collaborator

devyte commented Aug 21, 2018

Do you mean that a non-deprecated method is calling a deprecated one? Or is the issue just that deprecated.etbids are being compiled?
If the former, then code needs to be fixed.
If the latter, how about moving all deprecated methods to another .cpp? I think that was done by @aerlon for the wifimesh.

Jeroen88 added 2 commits August 21, 2018 17:49

#include "ESP8266HTTPClient.h"

#ifdef HTTPCLIENT_1_1_COMPATIBLE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same concern as before. If you use #ifdef XXXX then I think you're going the wrong way. Both APIs should be available, with no need for #ifdefs. Right now you either compile an object with one API or with the other API, but never both. So even basic compile checks aren't being done on one code path (unless you explicitly make an older test case and #undef as appropriate...

@igrr and @devyte , am I making sense or is this the way you'd do the migration? Don't want to harp on something that may not be a problem.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure changing the name does not change the concept. I tried to explain why I did this though. With the #define HTTPCLIENT_1_1_COMPATIBLE both APIs, deprecated and new, are available. In the next release everything between #ifdef .. #endif can be removed, including the #define itself. Only the new functions remain.
Don't worry about the basic compiler checks, there is only one #else to make the two variants of the end() function more readable. If you prefer to have no #else I can fix that. Without any #else only #ifdefs and the #define everything is compiled...

@Jeroen88
Copy link
Contributor Author

Thank you all for the suggestions to get rid of the Travis warnings. I wrapped the one begin() function causing the warnings into #pragma push and #pragme pop clauses to suppress them as @igrr suggested. I also had to wrap the begin() calls in the ESP8266HTTPUpdate.cpp library in those clauses, because they gave the same warning. In PR #4980 the begin() functions for ESP8266HTTPUpdate.cpp are already updated to the new api, but I will check it later today and update that PR if necessary.

@earlephilhower
Copy link
Collaborator

This is superseded by #4980. Closing in favor of the updated PR.

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.

5 participants