-
Notifications
You must be signed in to change notification settings - Fork 13.3k
BearSSL Max Fragment Length Negotation and Node.js server #5929
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
Conversation
… giving background processes some time in fetch()
There was a problem hiding this 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 😝 .
libraries/ESP8266WiFi/examples/BearSSL_MaxFragmentLength/BearSSL_MaxFragmentLength.ino
Outdated
Show resolved
Hide resolved
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... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
;-)
There was a problem hiding this 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.
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:
@earlephilhower I think you like this!