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

ESP32 RPC response reply only 1.695175409 #167

Closed
sirapol opened this issue Dec 8, 2023 · 32 comments · Fixed by #180
Closed

ESP32 RPC response reply only 1.695175409 #167

sirapol opened this issue Dec 8, 2023 · 32 comments · Fixed by #180

Comments

@sirapol
Copy link

sirapol commented Dec 8, 2023

I using examples/0010-esp8266_esp32_rpc/0010-esp8266_esp32_rpc.ino .

ESP32 can receive data from rpc request from server.
When ESP32 reply , It reply only 1.695175409.

Screenshot from 2023-12-08 15-07-14

Screenshot from 2023-12-08 15-07-45

I want it echo data or another thing.
Is it bug or my bad ?

@munya-gwena
Copy link

I am also facing the same issue with the examples. I just get a different number.

@MathewHDYT
Copy link
Contributor

MathewHDYT commented Dec 8, 2023

The serializeJson call should work and simply print a json with a key of "method" and the value "switch_state" and the same again but with the key "params".

For the return value I think there is some kind of misunderstanding, because you don't have to add the "method", or "params" key yourself. That is also never done in the examples. Because that is done automatically over the topic, so those values do not need to be added again.

What happens if you run your code again, but with the other lines commented out and the size decreased to 1, and only doc[RPC_RESPONSE_KEY] = switch_state;, do you then see the correct value?

Because the RPC_Response is meant to return a single value.

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

The serializeJson call should work and simply print a json with a key of "method" and the value "switch_state" and the same again but with the key "params".

For the return value I think there is some kind of misunderstanding, because you don't have to add the "method", or "params" key yourself. That is also never done in the examples. Because that is done automatically over the topic, so those values do not need to be added again.

What happens if you run your code again, but with the other lines commented out and the size decreased to 1, and only doc[RPC_RESPONSE_KEY] = switch_state;, do you then see the correct value?

Because the RPC_Response is meant to return a single value.

I try with your suggestion.
It same result like picture.

please suggest me.

Screenshot from 2023-12-08 17-14-12
Screenshot from 2023-12-08 17-14-02
Screenshot from 2023-12-08 17-13-55

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

And this Tenant is new one very clean.
I just add rule node for debug.
Rule node is working.

And i try with js client. It working normaly.
image
image
image

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

I found something in RPC_Response.cpp
Why it nothing to do ?

What should i do ?
image

@MathewHDYT
Copy link
Contributor

MathewHDYT commented Dec 8, 2023

The aforementioned code is correct, because nothing to do is the case, because we do not need to do anything in the constructor body, we do pass the given variable to the base class tough. In this case JsonVariant.

You can ignore that code section, that is not why it is not working correctly.


For testing purposes can you replace your return statement with this return RPC_Response(nullptr, switch_state);. It should now show true or false like expected instead of the number as before.

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

The aforementioned code is correct, because nothing to do is the case, because we do not need to do anything in the constructor body, we do pass the given variable to the base class tough. In this case JsonVariant.

You can ignore that code section, that is not why it is not working correctly.

For testing purposes can you replace your return statement with this return RPC_Response(nullptr, switch_state);. It should now show true or false like expected instead of the number as before.

It not work.
Not replay anything

image

image

By the way, I try to debug thingsboard.
And I using already code. It log about json. Is it help you ?
Screenshot from 2023-12-08 20-49-36
image

@MathewHDYT
Copy link
Contributor

MathewHDYT commented Dec 8, 2023

For the previous example, return RPC_Response(nullptr, switch_state);, which value does it show in the cloud? Because from the log message it seems like it should show 1.

If it is 1, then adjust the return to return RPC_Response{nullptr, switch_state}; and see if the value in the cloud is not true.

Additionally the debug messages are helpful, can you attach the debug message for the initial example you had, with the additionally enabled debug mode.

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

For the previous example, return RPC_Response(nullptr, switch_state);, which value does it show in the cloud? Because from the log message it seems like it should show 1.

If it is 1, then adjust the return to return RPC_Response{nullptr, switch_state}; and see if the value in the cloud is not true.

Additionally the debug messages are helpful, can you attach the debug message for the initial example you had, with the additionally enabled debug mode.

I try to using return RPC_Response(nullptr, switch_state); It timeout like picture.
And not have error from ESP32.

image
image

@MathewHDYT
Copy link
Contributor

MathewHDYT commented Dec 8, 2023

I think the problem is that the message, in this case without a key, is not sent. Because there is no log message printing that something was sent.

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

I think the problem is that the message, in this case without a key, is not sent. Because there is no log message printing that something was sent. Which version of the library are you using?

Thingsboard client sdk : 0.12.0

@MathewHDYT
Copy link
Contributor

Can you adjust the code to something like this:

StaticJsonDocument<JSON_OBJECT_SIZE(1)> doc;
JsonVariant variant = doc.to<JsonVariant>();
variant.set(switch_state);
return RPC_Response(doc);

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

Can you adjust the code to something like this:

StaticJsonDocument<JSON_OBJECT_SIZE(1)> doc;
JsonVariant variant = doc.to<JsonVariant>();
variant.set(switch_state);
return RPC_Response(doc);

Ok this work.
By the way, If i want add more json key. Is it possible?
image

@MathewHDYT
Copy link
Contributor

I'll have to try it myself, but as far as I know multiple keys are not possible, you should be able to send one big string containing a json tough.

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

I'll have to try it myself, but as far as I know multiple keys are not possible, you should be able to send one big string containing a json tough.

Ok I see.
But, In this case is reply the raw message. Is it possible convert to json before send over MQTT.
image

@MathewHDYT
Copy link
Contributor

I don't think so I'll check if it is possible to return more than one value.

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

I try to put json via JsonVariant like below code.

RPC_Response processSwitchChange(const RPC_Data &data)
{
    Serial.println(F("Received the set switch method"));
    // Process data
    const bool switch_state = data[RPC_SWITCH_KEY];
    Serial.print(F("Example switch state: "));
    Serial.println(switch_state);

    // example
    // serializeJsonPretty(data,Serial);
    // Just an response example
    // StaticJsonDocument<JSON_OBJECT_SIZE(1)> doc;
    // doc[RPC_RESPONSE_KEY] = 22.02;
    // return RPC_Response(doc);
    // end example

    StaticJsonDocument<JSON_OBJECT_SIZE(2)> doc;
    char xxxx[] = "{\"params\":{\"data\":\"fromESP32\"}}";
    deserializeJson(doc, xxxx);
    serializeJsonPretty(doc, Serial);
    JsonVariant variant = doc;
    // variant.set(xxxx);
    serializeJsonPretty(variant, Serial);
    return RPC_Response(variant);

    // return RPC_Response(nullptr, switch_state);
}

It output like this.
image
image

@sirapol
Copy link
Author

sirapol commented Dec 8, 2023

I found something.
In function process_rpc_message
After #ifdef THINGSBOARD_ENABLE_STL
response is not corrent.
Is it corresponding memory or not ?

I try to pass json to Send_Json_String() it work.

image
image

@sirapol
Copy link
Author

sirapol commented Dec 9, 2023

I verify about this issues. But, I not have idea for solve it. I will tell you.

The first one. RPC_Response before line 1781 is correct from call back.
When pass the #if THINGSBOARD_ENABLE_STL. Something override a little bit.
such as {"param":"data"} to {"par!@@!":"data"}

Then after line 1792 RPC_Response has only {}
Then after line 1794 RPC_Response is null.

I try to by pass this point. It work.
Please help implement it.

image

@MathewHDYT
Copy link
Contributor

MathewHDYT commented Dec 9, 2023

I'm not 100% sure what you mean, so if you skip line 1781 to 1792, it works like expected.

And with work like expected you mean that the RPC_Reponse is still {"param":"data"}? Sorry for the confusion

@sirapol
Copy link
Author

sirapol commented Dec 11, 2023

Hello, After I take my many time.

I think, It has issues about ArduinoJson memory and anything about it.

I let override process_rpc_message like below picture.
image

And in RPC_Response i do like this.
image

I not sure, Is it the best practice ?
But, It work for me.

I think, It will have problem when you dont need reply json format.
But , I don't care lol.

@munya-gwena
Copy link

@sirapol I'm trying to replicate your solution and you made use of a function called "splitString()". I can't find any reference of this function.

@sirapol
Copy link
Author

sirapol commented Dec 12, 2023

@sirapol I'm trying to replicate your solution and you made use of a function called "splitString()". I can't find any reference of this function.

Sorry. It my function.
image

@munya-gwena
Copy link

So I have done all the mods, but it seems there is some key element I'm missing somewhere. The ESP32 reboots after it has built the response topic.

Received the set switch method
Example switch state: 0
{
"params": {
"data": "fromESP32"
}
}{
"params": {
"data": "fromESP32"
}
}strJsonOut={"params":{"data":"fromESP32"}}
resTopic=v1/devices/me/rpc/response/222

abort() was called at PC 0x4013f72b on core 1

Backtrace: 0x40083669:0x3ffcb880 0x4008cd81:0x3ffcb8a0 0x400920f9:0x3ffcb8c0 0x4013f72b:0x3ffcb940 0x4013f772:0x3ffcb960 0x4013f6d3:0x3ffcb980 0x4014125b:0x3ffcb9a0 0x40140302:0x3ffcbc30 0x40140b44:0x3ffcbc50 0x400d4e9a:0x3ffcbc70 0x400d6825:0x3ffcbf10 0x40175029:0x3ffcc050 0x400d7405:0x3ffcc070 0x400d75d5:0x3ffcc0c0 0x400d6261:0x3ffcc0e0 0x400dbe1d:0x3ffcc150

@sirapol
Copy link
Author

sirapol commented Dec 12, 2023

So I have done all the mods, but it seems there is some key element I'm missing somewhere. The ESP32 reboots after it has built the response topic.

Received the set switch method Example switch state: 0 { "params": { "data": "fromESP32" } }{ "params": { "data": "fromESP32" } }strJsonOut={"params":{"data":"fromESP32"}} resTopic=v1/devices/me/rpc/response/222

abort() was called at PC 0x4013f72b on core 1

Backtrace: 0x40083669:0x3ffcb880 0x4008cd81:0x3ffcb8a0 0x400920f9:0x3ffcb8c0 0x4013f72b:0x3ffcb940 0x4013f772:0x3ffcb960 0x4013f6d3:0x3ffcb980 0x4014125b:0x3ffcb9a0 0x40140302:0x3ffcbc30 0x40140b44:0x3ffcbc50 0x400d4e9a:0x3ffcbc70 0x400d6825:0x3ffcbf10 0x40175029:0x3ffcc050 0x400d7405:0x3ffcc070 0x400d75d5:0x3ffcc0c0 0x400d6261:0x3ffcc0e0 0x400dbe1d:0x3ffcc150

Please try to update ArduinoJson Lib.

@munya-gwena
Copy link

After several days of trying out the RPC response, I have managed to get a response back but that is after a small mod in ThingsBoard.h. My observation is it looks like there is a memory leak of some sort. I was able to get it to send responses (with multiple fields) without a problem.
image
The mods I had to do was serialize the "response" to a string the deserialize back to local JsonDocument which is then used to send the response back to the device.
image
image

@MathewHDYT
Copy link
Contributor

MathewHDYT commented Dec 14, 2023

First of all thanks a lot for your detailed descriptions about the issue and the screenshots. I'm relatively sure I know what the underlying issue is know.

I'll work on a fix, but it will take a while, if a quick fix is required there is no need to change the library. Instead the RPC callback methods have to be changed like shown below.


Second of all I am relatively sure it is not a memory leak, that would mean that we are not erasing heap memory even tough we should. I'm pretty sure that is not the case, because the library more or less does not utilise heap memory, especially when THINGSBOARD_ENABLE_DYNAMIC is disabled, which is the case per default.

But what it probably is, is that we are accessing a reference to a local object that has been deleted.

In more detail: We are returning a RPC_Response, which is a JsonVariant. That is nothing more than a reference to a JsonDocument. But the JsonDocument it is pointing too, is erased after we leave the callback scope because it was created only in that method and lies on the stack, therefore we also erase or mangle up our response.

This is the case because stack memory is not really "deleted", but it allows other portions of the code to write into that memory and that would explain the mangled data.

That is probably the reason for the behaviour we saw previously. One way to fix that is to immediately copy the return value from the method to another object, because the data is still on the stack and if we immediately copy it we can "restore" it, but that is also very bad practice and a dirty fix to the problem.

Instead I'll work on a cleaner fix, that instead of returning a reference we instead pass a JsonVariant as one of the arguments and the user can change the sent response over that object. This allows the underlying library to be responsible for the memory instead of the callback method. That should remove the possibility for the aforementioned issue altogether.


To test that hypothesis @munya-gwena . It would be nice if you could simply try to allocate the JsonDocument on the global scope instead, which means it will not be erased after the method finishes.

That should also remove the need for the workaround.

StaticJsonDocument<JSON_OBJECT_SIZE(2)> doc;

RPC_Response processTemperatureChange(const RPC_Data &data) {
  doc.clear();
  doc["example1"] = 42;
  doc["example2"] = "test";
  return RPC_Response(doc);
}

@munya-gwena
Copy link

Seems you were right on the return value issue. I have made doc a global variable and have not had any issue so far.
image

@MathewHDYT
Copy link
Contributor

MathewHDYT commented Dec 14, 2023

Okay perfect thanks a lot for confirming. For now use that workaround.

I will work on a fix and mention this issue in the pull request once the fix has been completed and is going to be merged.

@sirapol
Copy link
Author

sirapol commented Dec 18, 2023

Okay perfect thanks a lot for confirming. For now use that workaround.

I will work on a fix and mention this issue in the pull request once the fix has been completed and is going to be merged.

Let close this issues ?

@MathewHDYT
Copy link
Contributor

It's fine it will be closed once the pull request gets merged anyway and until there isn't a fix in the library itself it's better if it's still open so others can easily find this issue if they have the same problem.

@OptifySudarshanPatil
Copy link

First of all thanks a lot for your detailed descriptions about the issue and the screenshots. I'm relatively sure I know what the underlying issue is know.

I'll work on a fix, but it will take a while, if a quick fix is required there is no need to change the library. Instead the RPC callback methods have to be changed like shown below.

Second of all I am relatively sure it is not a memory leak, that would mean that we are not erasing heap memory even tough we should. I'm pretty sure that is not the case, because the library more or less does not utilise heap memory, especially when THINGSBOARD_ENABLE_DYNAMIC is disabled, which is the case per default.

But what it probably is, is that we are accessing a reference to a local object that has been deleted.

In more detail: We are returning a RPC_Response, which is a JsonVariant. That is nothing more than a reference to a JsonDocument. But the JsonDocument it is pointing too, is erased after we leave the callback scope because it was created only in that method and lies on the stack, therefore we also erase or mangle up our response.

This is the case because stack memory is not really "deleted", but it allows other portions of the code to write into that memory and that would explain the mangled data.

That is probably the reason for the behaviour we saw previously. One way to fix that is to immediately copy the return value from the method to another object, because the data is still on the stack and if we immediately copy it we can "restore" it, but that is also very bad practice and a dirty fix to the problem.

Instead I'll work on a cleaner fix, that instead of returning a reference we instead pass a JsonVariant as one of the arguments and the user can change the sent response over that object. This allows the underlying library to be responsible for the memory instead of the callback method. That should remove the possibility for the aforementioned issue altogether.

To test that hypothesis @munya-gwena . It would be nice if you could simply try to allocate the JsonDocument on the global scope instead, which means it will not be erased after the method finishes.

That should also remove the need for the workaround.

StaticJsonDocument<JSON_OBJECT_SIZE(2)> doc;

RPC_Response processTemperatureChange(const RPC_Data &data) {
  doc.clear();
  doc["example1"] = 42;
  doc["example2"] = "test";
  return RPC_Response(doc);
}

Hey @MathewHDYT !
Thanks for this snippet!
work like a charm.

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 a pull request may close this issue.

4 participants