Skip to content

Use two buffers instead of one buffer in split mode #18

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

Merged
merged 1 commit into from
May 18, 2020
Merged

Use two buffers instead of one buffer in split mode #18

merged 1 commit into from
May 18, 2020

Conversation

ffontaine
Copy link
Contributor

Use two dedicated buffers for input and output instead of split mode,
indeed some MQTT server (especially with TLS) needs a full 8k buffer as
they send their Certificate Chain. On the other hand, on output, a 1k
buffer seems to be enough

Signed-off-by: Fabrice Fontaine [email protected]

@gvarisco gvarisco added the type: enhancement Proposed improvement label Feb 13, 2020
@ffontaine
Copy link
Contributor Author

Do you have any comments on this PR?

@aentinger
Copy link
Contributor

I've just looked at it (and tested it) and it seems like a reasonable addition. However I'm currently having memory constraints in Arduino's main application of this library (that is the Arduino IoT Cloud firmware) so I'd rather hold off merging this in for now.

@ffontaine
Copy link
Contributor Author

If you have memory constraints, I could tune the PR to update the default values of BEAR_SSL_CLIENT_IBUF_SIZE or BEAR_SSL_CLIENT_OBUF_SIZE. What is important is to make it configurable for the end user.

@aentinger
Copy link
Contributor

Good morning @ffontaine 👋 This sounds like an acceptable compromise, can you please update this PR accordingly?

@ffontaine
Copy link
Contributor Author

Sure, however I'm not a user of the Arduino IoT Cloud firmware so what default values will please you? 4k for each buffer? 8k for input and 4k for output or even 8k for each buffer (even if this one seems a bit oversized)

@aentinger
Copy link
Contributor

So. I've investigated the current situation and found the following results. This is how the buffer is structured currently:

  • obuf_len = 597 = 512 (hardcoded in BearSSL) + MAX_OUT_OVERHEAD (85)
  • ibuf_len = 8005 = BEAR_SSL_CLIENT_IOBUF_SIZE (8192 + 85 + 325) - obuf_len

If your PR can preserve this structure then that would be great.

Use two dedicated buffers for input and output instead of split mode.

Indeed some MQTT server (especially with TLS) needs a full 8k buffer as
they send their Certificate. On the other hand, on output, a smaller
buffer is needed.

Clients will be able to finely tune those values by defining
BEAR_SSL_CLIENT_{I,O}BUF_SIZE before including ArduinoBearSSL.h, the
default default values have been chosen to keep current behavior as
requested during review.

Signed-off-by: Fabrice Fontaine <[email protected]>
@ffontaine
Copy link
Contributor Author

OK done

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

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

Thank you @ffontaine 🚀

@aentinger aentinger merged commit 73f4763 into arduino-libraries:master May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants