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

Make HTTPClient suitabe for any Client #4825

Closed
wants to merge 20 commits into from
Closed

Make HTTPClient suitabe for any Client #4825

wants to merge 20 commits into from

Conversation

Jeroen88
Copy link
Contributor

@Jeroen88 Jeroen88 commented Jun 18, 2018

Adaptation to HTTPClient as suggested by @igrr on April 10 in PR #4273 and discussed with @earlephilhower by e-mail:

  1. Introduction of 2 begin() functions, passing in Client& as a parameter, to make HTTPClient suitable for any Client
  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

Example usage:

    BearSSL::WiFiClientSecure client;
    client.setInsecure();

    HTTPClient httpClient;
    if(httpClient.begin((Client &)client, "https://tls.mbed.org/")) {
      t_http_codes httpCode = (t_http_codes) httpClient.GET();
      if(httpCode == HTTP_CODE_OK) {
        uint32_t startTime = millis();
        do {  // Use a stream because getString() runs out of memory, because the response is very large
          char tmp[32];
          int rlen = client.read((uint8_t*)tmp, sizeof(tmp) - 1);
          yield();
          if (rlen < 0) {
            break;
          }
          tmp[rlen] = '\0';
          Serial.print(tmp);
        } while (millis() - startTime < 5000);
     
        Serial.printf("\n-------\n");
        httpClient.end();
      } else {
        Serial.printf("HTTP returned (%d)\n", httpCode);      
      }
    } else {
      Serial.printf("Unable to connect\n");
    }

If the server supports Maximum Fragment Length negotiation, the following code can be used, reducing the memory needs very significantly:


      BearSSL::WiFiClientSecure client;
      client.setInsecure();
      bool mfln = client.probeMaxFragmentLength("tls.mbed.org", 443, 1024);
      Serial.printf("\nConnecting to https://tls.mbed.org\n");
      Serial.printf("MFLN supported: %s\n", mfln ? "yes" : "no");
      if (mfln) {
        client.setBufferSizes(1024, 1024);
      }

      HTTPClient httpClient;
      if(httpClient.begin((Client &)client, "https://tls.mbed.org/")) {
        t_http_codes httpCode = (t_http_codes) httpClient.GET();
        if(httpCode == HTTP_CODE_OK) {
          Serial.println(httpClient.getString());
          Serial.printf("\n-------\n");
          httpClient.end();
        } else {
          Serial.printf("HTTP returned (%d)\n", httpCode);
        }
      } else {
        Serial.printf("Unable to connect\n");
      }

@Jeroen88
Copy link
Contributor Author

Jeroen88 commented Jun 18, 2018

Travis build failed because I set the current API deprecated, this generates the failure:

    // Plain HTTP connection, unencrypted
    bool begin(String url)  __attribute__ ((deprecated));
    bool begin(String host, uint16_t port, String uri = "/")  __attribute__ ((deprecated));
    // Use axTLS for secure HTTPS connection
    bool begin(String url, String httpsFingerprint)  __attribute__ ((deprecated));
    bool begin(String host, uint16_t port, String uri, String httpsFingerprint)  __attribute__ ((deprecated));
    // Use BearSSL for secure HTTPS connection
    bool begin(String url, const uint8_t httpsFingerprint[20])  __attribute__ ((deprecated));
    bool begin(String host, uint16_t port, String uri, const uint8_t httpsFingerprint[20])  __attribute__ ((deprecated));

/home/travis/build/esp8266/Arduino/libraries/ESP8266HTTPClient/examples/Authorization/Authorization.ino:49:61: error: 'bool HTTPClient::begin(String)' is deprecated (declared at /home/travis/arduino_ide/hardware/esp8266com/esp8266/libraries/ESP8266HTTPClient/src/ESP8266HTTPClient.h:151) [-Werror=deprecated-declarations]
http.begin("http://user:[email protected]/test.html");

@devyte
Copy link
Collaborator

devyte commented Jun 18, 2018

I suggest not marking the current api as deprecated just yet. There are several things that should happen before that. I think the steps to follow should be:

  1. merge the new api as experimental, this will get exposure
  2. fix any reported issues
  3. mark the new api as stable
  4. migrate the examples to the new api
  5. mark the old api as deprecated
  6. retire the old api (this is a breaking change, so should happen in release 3.0.0)

@Jeroen88
Copy link
Contributor Author

For now, I removed the deprecated from the current api. I will have a look at the examples to migrate them to the new api. @devyte How do I merge the new api as experimental?

@Jeroen88
Copy link
Contributor Author

Travis build failed because several macro's are redefined, however, not in my scope, e.g.

In file included from ../../cores/esp8266/WCharacter.h:23:0,
from common/Arduino.h:241,
from ../../cores/esp8266/MD5Builder.cpp:1:
/usr/include/ctype.h:234:0: note: this is the location of the previous definition
'# define toascii(c) __toascii (c)

@Jeroen88
Copy link
Contributor Author

The examples are migrated to new api as @devyte suggested. 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 is part of the upcoming LTS release of OpenSSL 1.1.1. The URL in the example already supports MFLN.

@earlephilhower earlephilhower self-requested a review July 2, 2018 21:55
@earlephilhower earlephilhower added this to the 2.5.0 milestone Jul 6, 2018
@devyte devyte self-requested a review July 9, 2018 18:55
@liebman
Copy link
Contributor

liebman commented Jul 15, 2018

One unexpected issue I just ran into was that the destructor for ESP8266WifiClient calls a method on the Client* passed in. If you've allocated ESPHTTPClient on the stack so its scoped to the local function and allocate the WiFiClient passed in like: (simplified/contrived code)

void doit(const char* url) {
  WiFiClient *client = new WiFiClient;
  ESP8266HTTPClient http;
  http.begin(*client, "http://www.example.com");
  http.end();
  delete client;
}

Then the client pointer saved in ESP8266HTTPClient is referenced after its been deleted when the doit() function exits and the ESP8266HTTPClient destructor is called. I worked around this in my project by allocating the ESP8266HTTPClient on the heap with new and then adding a delete before deleting the client.

I bring this up because its an easy issue to create if your checking the url and creating either a WiFiClientSecure or a WiFiClient to use on the fly. The only suggestion I have at the moment is that there should be an example that creates one or the other client with a comment warning about the destructor order.

@Jeroen88
Copy link
Contributor Author

@liebman Thanks for your comment! I made a small modification on the end() function. Could you check if your bug is fixed?

@Jeroen88
Copy link
Contributor Author

Replaced with #4979

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