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

Bugfix/esp8266 http client #5250

Merged
merged 14 commits into from
Oct 21, 2018
Merged
Prev Previous commit
Next Next commit
Minor formatting to pass Travis tests
Jeroen88 committed Oct 16, 2018

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
commit 29d97b0e2a742217b184db89b101448a98c1bc89
Original file line number Diff line number Diff line change
@@ -45,7 +45,8 @@ void loop() {

client->setFingerprint(fingerprint);

{ // Create a block arround HTTPClient, so it is destroyed before WiFiClientSecure *client is deleted
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is technically correct: the client object must outlive the http object that uses it.
However, it is rather unclear at first glance that this is the reason for the scope. Also, someone looking at the example would be unlikely to realize the importance of the scope.
Instead, consider wrapping the allocation in a std::unique_ptr:

std::unique_ptr<BearSSL::WiFiClientSecure> client(new BearSSL::WiFiClientSecure);

Then, the internal object can be passed by get():

if (https.begin(*client, "https://jigsaw.w3.org/HTTP/connection.html")) {  // HTTPS
...

Because both objects are in the same scope, and because the client must be declared before the http object, when exiting the scope their destruction will automatically happen in the correct order: inverse of their construction.
Also, no need to call delete.
Also, I don't like having manual handling of dynamic objects in an example ino, which will be viewed and used by inexperienced users, so smart pointers is better.

// Create a block arround HTTPClient, so it is destroyed before WiFiClientSecure *client is deleted
HTTPClient https;

Serial.print("[HTTPS] begin...\n");
@@ -73,7 +74,9 @@ void loop() {
} else {
Serial.printf("[HTTPS] Unable to connect\n");
}
} // End block around HTTPClient

// End block around HTTPClient
}
delete client;
}

Original file line number Diff line number Diff line change
@@ -53,7 +53,8 @@ void loop() {
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);

{ // Create a block arround HTTPClient, so it is destroyed before WiFiClientSecure *client is deleted
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above: consider wrapping in a std::unique_ptr, and removing the scope.

// Create a block arround HTTPClient, so it is destroyed before WiFiClientSecure *client is deleted
HTTPClient https;

if (https.begin(*client, "https://tls.mbed.org/")) {
@@ -108,7 +109,9 @@ void loop() {
} else {
Serial.printf("Unable to connect\n");
}
} // End block around HTTPCLient

// End block around HTTPCLient
}

delete client;
}