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

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

merged 7 commits into from
Apr 11, 2019

Conversation

Jeroen88
Copy link
Contributor

@Jeroen88 Jeroen88 commented Mar 29, 2019

Good news!
The current version of Node.js (not the LTS at the time of writing this), so Node.js version 11 supports Maximum Fragment Length Negotiation (MFLN) out of the box!!

If you run a Node.js server you can connect your ESP8266 to this server over a secure connection with BearSSL using the technique explained in this sketch.

This saves more than 15k of RAM compared to not using MFLN.

Of course the smaller rx and tx buffers lead to more overhead, because less data can be transferred in one fragment.

This PR does some minor bug fixes on the MFLN example sketch:

  • In fetch() give a bit of time to the background processes if no data is (yet) returned by the server
  • In fetch() use polledTimeout to check the timeout interval
  • In fetch() skip the memory clearing of the buffer, instead just add a '\0' at the end of the data read
  • In fetchMaxFragmentLength() lower the rx and tx buffer sizes to 512 bytes each when using MFLN. It seems that 512 bytes is the bare minimum

@earlephilhower I think you like this!

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

I'm ok with this, pending the polledTimeout loop, but @earlephilhower is the bssl expert 😝 .

@Jeroen88
Copy link
Contributor Author

Jeroen88 commented Apr 1, 2019

I changed the loop using polledTimeout, but I also got the recently approved ESP8266WiFiSTA.cpp (the new version) in my commit, I do not know why...

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.

LGTM. Thanks.

@d-a-v d-a-v requested a review from devyte April 9, 2019 21:19
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

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.

;-)

Copy link
Collaborator

@devyte devyte left a comment

Choose a reason for hiding this comment

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

Nothing jumps out at me, looks good.

@d-a-v d-a-v merged commit 667816e into esp8266:master Apr 11, 2019
This pull request was closed.
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