Binary data not working

[pknoe3lh]

Looks like 0 chars in binary package terminats packed.

Or msg.data().length() is not working ...

[gilmaimon]

I will look into it.
Can you give some more info about your setup. Are you sending empty binary messages to an ESP client and getting a crash on the client? Can you add ESP logs? Can you attach a decoded stack trace and the relevant code you are using?

EDIT: I was unable to reproduce with the information you gave here.

Thanks.

[im-pro-at]

Sorry for the short and misleading text ...

Arduino ESP32 1.0.2 with ArduinoWebsockets 0.4.10 :

#include <ArduinoWebsockets.h>
#include <WiFi.h>
using namespace websockets;

WebsocketsServer wsserver;
WebsocketsClient client;

void setup(){
  // Start Serial port
  Serial.begin(115200);
 
  // Start access point
  WiFi.softAP("POV Display");
 
  // Print our IP address
  Serial.println();
  Serial.println("AP running");
  Serial.print("My IP address: ");
  Serial.println(WiFi.softAPIP());

}

void loop() {  

  if(client.available()) {
    client.poll();    
  }
  else if(wsserver.available()){
    if(wsserver.poll()){
      Serial.println("New Connection");
      client.close();
      client = wsserver.accept();
      client.onMessage([](WebsocketsClient& client, WebsocketsMessage msg) {
        Serial.printf("Message T%d B%d Pi%d Po%d C%d stream%d length: %u\n", msg.isText(), msg.isBinary(), msg.isPing(), msg.isPong(), msg.isClose(),msg.isPartial(), msg.data().length());
      });
      
    }
  }
  else {
    Serial.println("Start Server");
    wsserver.listen(8080);
  }
}

Webbrowser:

var websocket= new WebSocket("ws:192.168.4.1:8080");
websocket.onopen = function(evt){
  websocket.send("test"); // get 4 length
  var test = new Uint8Array(10);
  for(i=0;i<10;i++) test[i]=i+1;
  websocket.send(test); // get 10 length expect 10
  test[1]=0;
  websocket.send(test); // get 1 length expect 10
  test[0]=0;
  websocket.send(test); // get 1 length expect 10
};

Serial log:

load:0x40078000,len:9232
load:0x40080400,len:6400
entry 0x400806a8

AP running
My IP address: 192.168.4.1
Start Server
New Connection
Message T1 B0 Pi0 Po0 C0 stream0 length: 4
Message T0 B1 Pi0 Po0 C0 stream0 length: 10
Message T0 B1 Pi0 Po0 C0 stream0 length: 1
Message T0 B1 Pi0 Po0 C0 stream0 length: 0

Hope you can reconstruct it now ;-)

[gilmaimon]

Sadly I still can't see any crashes..

Is it possible that you forget to call .close() on your browser client code? Because with how your code is written, it seems like it can only handle one client at a time. And as long as a client is connected (not closed, meaning close was not called) you will not accept another client.

Let me know if you have any more info regarding what you see (some sort of crash?) or what you expect to happen that isn't happening.
Gil.

[pknoe3lh]

There are no crashes ;-) its working really stable!

Yes I only need one connection at a time. Only one client is planned.

But data().length() is not giving the right information.
That's what the difference between get and expected is coming from.

Or do I do something completely wrong?

[gilmaimon]

Oh, I see now! I understood you completely wrong, sorry.

The issue seems to be that Arduino's String class terminates on null, unlike the standard std::string. If you would run this sketch on a PC (using TinyWebsockets) you will see that the sizes are as expected.

In order to work around that, I'm consider adding a WebsocketsMessage.length() method, so you can get the original size directly from the library. The only issue is that knowing how big of a mess Arduino's libraries are, I assume they won't copy the whole string and will stop on null values.

The reason I chose Arduino's String is in order for users to be able to print the strings directly to Serial. But it seems like I should provide a pro feature to access the raw string directly. Will something like that be helpful to you?

Gil.

[pknoe3lh]

Yes exactly. You got it ;-)

Yes accessing the raw buffer ist ok for me.
I did not look into it. I was first writing the issue to get the message that bin packeds are not supported ...
I think this problem shows that no one has used this before.

I'm planing to send jpegs over the WS in the browser you can convert them to to blobs and easily send them.

I'm just worrying that there will be a problem with memory not freed. Arduino and Strings are really dangerous on this topic.

[gilmaimon]

Ok, I have the len change in master, give it a try.
I don't have time to see if it preserves the data after the nulls. I assume it won't. I'll think about a solution and will work on it hopefully next weekend.
Thanks.

[im-pro-at]

Thanks! Still need the internal buffer :-(

I think the Problem is in ws_common.cpp: fromInternalString()

std::string::c_str

const char* c_str() const;
Get C string equivalent
Returns a pointer to an array that contains a null-terminated sequence of characters (i.e., a C-string) representing the current value of the string object.

This array includes the same sequence of characters that make up the value of the string object plus an additional terminating null-character ('\0') at the end.

so if 0 char is in the internal string then it will be interpreted as terminating null-character ...

[im-pro-at]

I mad a temp fix for me:
im-pro-at@45f9b91
Thanks for your help!!!!

[denden4444]

Oh, I see now! I understood you completely wrong, sorry.

The issue seems to be that Arduino's String class terminates on null, unlike the standard std::string. If you would run this sketch on a PC (using TinyWebsockets) you will see that the sizes are as expected.

In order to work around that, I'm consider adding a WebsocketsMessage.length() method, so you can get the original size directly from the library. The only issue is that knowing how big of a mess Arduino's libraries are, I assume they won't copy the whole string and will stop on null values.

The reason I chose Arduino's String is in order for users to be able to print the strings directly to Serial. But it seems like I should provide a pro feature to access the raw string directly. Will something like that be helpful to you?

Gil.

Yes please re raw :-) in a char array perhaps ?

[gilmaimon]

A char array is not good in my opinion. It has ownership and memory issues.

Regarding your solution guys, I'm thinking about what can be done.. I want to make a change that won't break anyone's code but I'm not sure what can be done other than spending memory on duplicate of the data in it's raw form.

I'm thinking about it, and it is an open bug and issue. If you guys have any idea, please let me know!
I'm keeping this open for discussion 🤔

[gilmaimon]

The solution I'm considering now is:

• Removing the usage of Arduino Strings completely and only using std::string.
• As a consequence users won't be able to Serial.println(message.data())
• This will make the library more efficient, there will be no need for converting strings

Alternative 1:

• Message can store data as std::string and have a msg.getData which will return Arduino String and something like msg.getRawData() which will return std::string.
• This means copying and converting the string to Arduino String for each call to getData()
• This does not break anyone's existing code and can be a patch rather than a minor version update

[adelin-mcbsoft]

As an opinion:
Indeed working with strings is way more heavier than with chars...

However, if you remove the Arduino's Strings completely a lot of code will be broken as soon as users start to update their library...
From a semver point of view, that seems like it should be a new major version to me, which by definition may not be backwards compatible.

I would suggest adding a configuration value to the library, by which we could select the output type format we'd like to have for messages... (a method to be called in setup or so).
This way you will keep the current functionality working and also bring the new implementation in.

Just my two cents. 😁

[gilmaimon]

Why working with strings is "way more heavier" than with chars? std::string, when implemented well, is a very thin wrapper around a char array.

I don't think moving to a char* will give users any kind of noticeable efficiency benefit. Anyways, if you use an esp32 or esp8266 with Arduino you probably not doing any super heavy work or anything that makes char* handling worth the effort.

A configuration is a great idea. I've tried implementing a #define that will toggle a "pro mode" but it didn't work out, maybe I did something wrong.

[adelin-mcbsoft]

It's not really about the efficiency from processing perspective, but from memory.

When I said heavier, I was referring to the fact that - though String class is easy to use, takes a lot of RAM.
Chars can be manipulated in a much more efficient way so obviously will take less RAM, but comes with the "headache" (and time) cost while developing.

But - Gil - don't get me wrong, I love the library the way it is;
As for me, I wouldn't change it, but - I've seen that you considered the opinions above and you are looking forward for a new approach, so I just wanted to ring-a-bell about keeping the backwards-compatibility in an user-friendly manner; that's why I came with the idea of a configuration variable in setup or something similar.

I love the library and I want the best for it. 😀
Hope it sheds some light,

All the best,
A.

[gilmaimon]

You are definitely right, and I appreciate all the help and support 😃

I think breaking the interface might be the right thing to do in this case. Anyways, I'm keeping this issue open for any discussion.

Best wishes,
Gil.

[gilmaimon]

An idea:
make .data return std::string and make WebsocketsMessage printable. So users could:

Serial.println(msg);

Most users could just:

auto data = msg.data();

and work with it without actually knowing what it contains (similar enough interface as Arduino's String)

[gunhaxxor]

I have the same issue.
Have an esp32 receiving binary data, and haven't found a way to extract all the 492 bytes into a buffer. Just some way to do memcpy() from the message would be sufficient for me. I think it's rather counter intuitive that the .data() function returns a String. Especially in those cases when .isBinary() is true.
Would it perhaps make sense to have an additional function called something like .rawData()? It could just return a pointer directly to the data (char *). That wouldn't break backwards compatibilty, right?

Personal opinion:
I believe, as already discussed by @adelin-mcbsoft, that the String class is evil in terms of performance and memory management. I always try to avoid using it if I can.

[gilmaimon]

Thanks for all the feedback, I'm on it guys.
There will be a fix that won't break any backward compatibility.

[gilmaimon]

A solution was merged to master a few moments ago. I will appreciate if any of you could check if your issues are solved by using rawData() and c_str() on the message. I ran some tests, things seem to work fine.

I also documented the change here: https://github.com/gilmaimon/ArduinoWebsockets#binary-data

[gilmaimon]

I published release 0.4.14 which solves this issue. I mark this as solved, and will close this soon (unless anyone can reproduce the bug with the new interfaces, which shouldn't happen 😄 )

Thank you everyone for helping and contributing.
Gil.

(I'm keeping this issue open for few days)