Skip to content

request to modify String::concat #5061

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
Closed
5 of 6 tasks
darrendavidhumphrey opened this issue Aug 18, 2018 · 9 comments · Fixed by #6754
Closed
5 of 6 tasks

request to modify String::concat #5061

darrendavidhumphrey opened this issue Aug 18, 2018 · 9 comments · Fixed by #6754
Assignees
@earlephilhower
Labels
level: easy type: enhancement
Milestone
2.7.0

Comments

@darrendavidhumphrey
Copy link

unsigned char String::concat(const char *cstr, unsigned int length) {
unsigned int newlen = len + length;
if(!cstr)
return 0;
if(length == 0)
return 1;
if(!reserve(newlen))
return 0;
strcpy(buffer + len, cstr);
len = newlen;
return 1;
}

Basic Infos

  • This issue complies with the issue POLICY doc.
  • I have read the documentation at readthedocs and the issue is not addressed there.
  • I have tested that the issue is present in current master branch (aka latest git).
  • I have searched the issue tracker for a similar issue.
  • If there is a stack dump, I have decoded it.
  • I have filled out all fields below.

Platform

  • Hardware: [All ESPxx]
  • Core Version: [latest]
  • Development Env: [All]
  • Operating System: [All]

Problem Description

The String (Wstring.cpp) class has no method for concat() from a non-null terminated string. This is annoying, and inffecient, when processing payloads from MQTT messages, which are not-null terminated.

This existing Concat(cstr,length) method would work with minor modifications, but it is protected. I am proposing modifying it as shown below and making it public.

unsigned char String::concat(const char *cstr, unsigned int length) {
unsigned int newlen = len + length;
if(!cstr)
return 0;
if(length == 0)
return 1;
if(!reserve(newlen))
return 0;

-- strcpy(buffer + len, cstr); // Would fail if cstr is not NULL terminated
++ memcpy(buffer + len, cstr,length);
++ buffer[newlen] = 0; // insert NULL terminator

len = newlen;
return 1;
}

MCVE Sketch

#include <Arduino.h>
#include "WString.h"

void setup() {
   char bigBuff[1000];

   int messageSize= ReadMQTT_Payload(bigBuff);
   
    String someString;

// How you have to do it now.  concat called for each character
   string.reserve(messageSize)
   for (int i=0; i < messageSize; i++)
      someString.concat(bigBuff[i];

// How you could do it with proposal.  Single call to concat
   someString.concat(bigBuff,messageSize);
}

void loop() {

}

Debug Messages

Debug messages go here
@earlephilhower
Copy link
Collaborator

Interesting idea. Why not submit a PR with the change? It's simple and means less work for the few maintainers, which means it's got a better chance of getting in there faster. :)

@darrendavidhumphrey
Copy link
Author

darrendavidhumphrey commented Aug 18, 2018 via email

@earlephilhower
Copy link
Collaborator

@darrendavidhumphrey You're almost there. This is an issue, but a PR is an actual code repo you want merged. Makes it easier for us, and makes it better for you since you get the honor (or blame, sometimes, believe me!) of having your name in the repo associated with the code change.

There's a good github description, but basically to make a PR you just
. From the web interface hit "fork this repo"
. git clone https://github.com/**your fork**
. git checkout -b new-pr-branch-name
<apply changes, do your testing>
. git commit -a
. git push
. Back at the web UI, a new notice will appear above your code asking if you're like to submit a PR. Hit the button, fill out a field or two, and we're set (CI will verify it compiles, some host tests will run, etc).

At that stage, the maintainers can just click one button and merge. We get one more contributor on the list, and have one less thing to worry about. :)

@darrendavidhumphrey
Copy link
Author

darrendavidhumphrey commented Aug 22, 2018 via email

@earlephilhower
Copy link
Collaborator

BearSSL::WiFiClientSecure() is the one I'd recommend. It allocates memory all at once, so it is much less prone to OOM errors. It also is actively maintained so when bugs happen they get fixed, unlike axTLS.

@darrendavidhumphrey
Copy link
Author

darrendavidhumphrey commented Aug 22, 2018 via email

@earlephilhower
Copy link
Collaborator

It's been a little bit since I did measurements, but roughly the memory breakdown is:
. 4.5KB (only) on the first instance of BearSSL::client/server for the 2nd stack, present until all BearSSL objects are destroyed
. ~24KB (IIRC, could be a little higher) for each server/client connection. If you can use MFLN (see the examples, depends on the server supporting it) I think this can go down to ~8KB or so per connection. Again, though, only with MFLN to limit the TLS packet sizes.

@darrendavidhumphrey
Copy link
Author

darrendavidhumphrey commented Aug 23, 2018 via email

@earlephilhower
Copy link
Collaborator

Great news! If you control both ends, the memory really can be reasonable. Just be aware, anything that doesn't fit will cause the SSL connection to completely drop. There is no fragmentation (w/o real MFLN) in TLS other than at that 16K chunksize.

I don't remember it taking 4 seconds to parse the certstore output before, but what I have found with SPIFFs is it can get into a very slow state sometimes.

The file is a simple UNIX .AR archive, so you can use any tool to make it with just your own certs, if desired. If you only have a single cert (say, self-signed CA) then just use that one and don't use a CertStore at all, just pass in the CA.

@devyte devyte added this to the 2.7.0 milestone Nov 9, 2019
earlephilhower added a commit to earlephilhower/Arduino that referenced this issue Nov 11, 2019
@earlephilhower
Add comcat(char*, len) to Sting
8ccf5d0 
Fixes esp8266#5061

Adds a concat(const char *data, int len) method which allows arbitrary
sequences of data (including ones w/embedded \0s) to be appended to a
String.  May be useful for certain MQTT operations.

Adds sanity test for the feature to host suite
@earlephilhower earlephilhower mentioned this issue Nov 11, 2019
Merged
d-a-v pushed a commit that referenced this issue Nov 13, 2019
@earlephilhower @d-a-v
Add String::concat(char*, len) to allow non null-term strings (#6754)
09896d5 
* Add comcat(char*, len) to Sting

Fixes #5061

Adds a concat(const char *data, int len) method which allows arbitrary
sequences of data (including ones w/embedded \0s) to be appended to a
String.  May be useful for certain MQTT operations.

Adds sanity test for the feature to host suite

* Review comment cleanups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@earlephilhower earlephilhower

Labels
level: easy type: enhancement
Projects
None yet
Milestone
2.7.0
Development

Successfully merging a pull request may close this issue.

Add String::concat(char*, len) to allow non null-term strings earlephilhower/Arduino
3 participants
@earlephilhower @devyte @darrendavidhumphrey