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.

Sign up for GitHub

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

Jump to bottom

BearSSL Max Fragment Length Negotation and Node.js server #5929

Merged
merged 7 commits into from
Apr 11, 2019
Merged

BearSSL Max Fragment Length Negotation and Node.js server #5929

Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e94d1e3
Minor bug fixes in Maximum Fragment Length Negotation example, mainly…
Mar 29, 2019
f727636
Minor layout changes to pass travis tests
Mar 29, 2019
092c43d
Merge branch 'master' into feature/MFLN
Apr 1, 2019
532a553
Use PolledTimeout for timeout
Apr 1, 2019
6006e8d
Merge remote and origin
Apr 1, 2019
3ecacf4
Merge branch 'master' into feature/MFLN
Apr 7, 2019
f920652
Merge branch 'master' into feature/MFLN
Apr 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions libraries/ESP8266WiFi/examples/BearSSL_MaxFragmentLength/BearSSL_MaxFragmentLength.ino
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,17 +17,22 @@ const char *pass = STAPSK;
void fetch(BearSSL::WiFiClientSecure *client) {
client->write("GET / HTTP/1.0\r\nHost: tls.mbed.org\r\nUser-Agent: ESP8266\r\n\r\n");
client->flush();
uint32_t to = millis() + 5000;
uint32_t startMillis = millis();
Jeroen88 marked this conversation as resolved.
Show resolved Hide resolved
do {
char tmp[32];
memset(tmp, 0, 32);
// memset(tmp, 0, 32);
int rlen = client->read((uint8_t*)tmp, sizeof(tmp) - 1);
yield();
if (rlen < 0) {
break;
}
if (rlen == 0) {
delay(10); // Give background processes some time
continue;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is delay() required ?
There is no background process except from receiving, storing, but without processing data.
Calling read() often ensures processing them as soon as data are received.

Copy link
Contributor Author

@Jeroen88 Jeroen88 Apr 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@d-a-v If there is no data available yet (rlen == 0), without delay I experienced timeout errors. My assumption is that this is because with no data available the loop that keeps the ESP busy reduces almost to:

do { } while (!timeout)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did experienced timeout error @80MHz even with delay(10) or with master. @earlephilhower reminded me that SSL works better @160MHz because "mbed uses EC which is insanely slow".
I had no timeout without delay(10) (your PR) @160MHz.

With delay, average duration over about 20 request is 273.5ms, and 249.6ms without (measuring the while loop).

Can you retry at 160MHz and see if you still have those timeouts (that I have with master @80Mhz) ?

To be honest, I am concerned about this delay and the associated comment which is wrong to me.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My assumption is that this is because with no data available the loop that keeps the ESP busy reduces almost to: do { } while (!timeout)

I agree with saying the arduino infinite loop way of coding is wrong. In that case we could "delay-and-reduce-cpu-activity-until-a byte-is-received" and this api/call is lacking in our API. We are not in an RTOS, but maybe something is doable for this common case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@d-a-v

Can you retry at 160MHz and see if you still have those timeouts (that I have with master @80Mhz) ?

I always run the ESP8266 @ 160MHz when using BearSSL, so I experienced the timeout without the delay @ 160MHz and never tried @ 80MHz.

To be honest, I am concerned about this delay and the associated comment which is wrong to me.

I asume you are not looking for just changing the text of the comment?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I too can be picky :) There are already two approvals and changes worksforme (and it's nice). So I let it go as-is. When my pickyness will be boiling I'll make a proposal for the comment-only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

;-)

}
tmp[rlen] = '\0';
Serial.print(tmp);
} while (millis() < to);
} while (millis() - startMillis < 5000);
client->stop();
Serial.printf("\n-------\n");
}
Expand Down Expand Up @@ -73,11 +78,11 @@ int fetchMaxFragmentLength() {

BearSSL::WiFiClientSecure client;
client.setInsecure();
bool mfln = client.probeMaxFragmentLength("tls.mbed.org", 443, 1024);
bool mfln = client.probeMaxFragmentLength("tls.mbed.org", 443, 512);
Serial.printf("\nConnecting to https://tls.mbed.org\n");
Serial.printf("MFLN supported: %s\n", mfln ? "yes" : "no");
if (mfln) {
client.setBufferSizes(1024, 1024);
client.setBufferSizes(512, 512);
}
client.connect("tls.mbed.org", 443);
if (client.connected()) {
Expand Down Expand Up @@ -125,6 +130,6 @@ void loop() {
yield();

Serial.printf("\n\n");
Serial.printf("Default SSL: %d bytes used\n", a);
Serial.printf("1024 byte MFLN SSL: %d bytes used\n", b);
Serial.printf("Default SSL: %d bytes used\n", a);
Serial.printf("512 byte MFLN SSL: %d bytes used\n", b);
}