-
-
Notifications
You must be signed in to change notification settings - Fork 512
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
No proper check of memory allocation when using DynamicJsonDocument results in sometimes temporary unexpected behavior during runtime #1615
Comments
Thanks for the hint!
I don't think so. The objects etc. are allocated in the RAM not in the flash memory. But I could imagine that there is not enough free memory in one piece. (Keyword memory fragmentation)
Will implement some logging, but in the end, it seems that in a long term, the live data response has to be split into different requests or websocket responses (e.g. just send one inverter). Regarding Configuration handling, I have already implemented the check. Will be released soon. |
Another observation: void WebApiWsLiveClass::loop()
{
.
.
.
// Update on every inverter change or at least after 10 seconds
if (millis() - _lastWsPublish > (10 * 1000) || (maxTimeStamp != _newestInverterTimestamp)) {
try {
std::lock_guard<std::mutex> lock(_mutex);
MessageOutput.printf("Calling /api/livedata/status FreeHeap = %d, MaxAllocHeap = %d, MinFreeHeap = %d.\r\n", ESP.getFreeHeap(), ESP.getMaxAllocHeap(), ESP.getMinFreeHeap());
DynamicJsonDocument root(4200 * INV_MAX_COUNT); // TODO(helge) check if this calculation is correct
JsonVariant var = root;
MessageOutput.printf("Calling /api/livedata/status jsonDocument capacity = %d.\r\n", root.capacity());
generateJsonResponse(var);
MessageOutput.printf("Calling /api/livedata/status JsonDocumentSize = %d (allocated %d).\r\n", root.memoryUsage(), 4200 * INV_MAX_COUNT);
String buffer;
if (buffer) {
serializeJson(root, buffer);
if (Configuration.get().Security.AllowReadonly) {
_ws.setAuthentication("", "");
} else {
_ws.setAuthentication(AUTH_USERNAME, Configuration.get().Security.Password);
}
_ws.textAll(buffer);
}
} catch (const std::bad_alloc& bad_alloc) {
MessageOutput.printf("Calling /api/livedata/status has temporarily run out of resources. Reason: \"%s\".\r\n", bad_alloc.what());
} catch (const std::exception& exc) {
MessageOutput.printf("Unknown exception in /api/livedata/status. Reason: \"%s\".\r\n", exc.what());
}
_lastWsPublish = millis();
}
} Shows in the log:
Now I enable TLS in MQTT
|
Yes, it's clear that TLS requries a lot of memory... As mentioned before, on the long term the repsonse has to be changed. |
I know, that there is heap fragmentation. I dump MaxAllocHeap which shows the largest memory block, which could be allocated. The size of the JsonDocument is significant smaller than that block. But I agree, logging some warnings would be good and then split the live data in pieces, or stream it in smaller chunks over the websocket. |
Sorry my fault, I talked about the RAM, too. My Esp32 shows some strange hardware behavior, so I believe it is going to die soon. |
Made several experiments this evening to reduce the heap usage on the The {
"inverters": [
{
"serial": "116480148266",
"name": "HMS-2000-01",
"order": 0,
"data_age": 405,
"poll_enabled": true,
"reachable": false,
"producing": false,
"limit_relative": 0,
"limit_absolute": -1
},
{
"serial": "116480145267",
"name": "teste 1",
"order": 1,
"data_age": 405,
"poll_enabled": false,
"reachable": false,
"producing": false,
"limit_relative": 0,
"limit_absolute": -1
},
{
"serial": "116480145268",
"name": "test 2",
"order": 2,
"data_age": 405,
"poll_enabled": false,
"reachable": false,
"producing": false,
"limit_relative": 0,
"limit_absolute": -1
}
],
"total": {
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
}
},
"hints": {
"time_sync": false,
"radio_problem": false,
"default_password": true
}
} This is enough to generate the basic structure of the live data page. The real data are coming from the websocket. Not at once but each inverter after another {
"inverters": [
{
"serial": "116480148266",
"name": "HMS-2000-01",
"order": 0,
"data_age": 550,
"poll_enabled": true,
"reachable": false,
"producing": false,
"limit_relative": 0,
"limit_absolute": -1,
"AC": {
"0": {
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"Power DC": {
"v": 0,
"u": "W",
"d": 1
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
},
"Frequency": {
"v": 0,
"u": "Hz",
"d": 2
},
"PowerFactor": {
"v": 0,
"u": "",
"d": 3
},
"ReactivePower": {
"v": 0,
"u": "var",
"d": 1
},
"Efficiency": {
"v": 0,
"u": "%",
"d": 3
}
}
},
"DC": {
"0": {
"name": {
"u": "Panel 1"
},
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
},
"Irradiation": {
"v": 0,
"u": "%",
"d": 3,
"max": 400
}
},
"1": {
"name": {
"u": ""
},
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
}
},
"2": {
"name": {
"u": ""
},
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
}
},
"3": {
"name": {
"u": ""
},
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
}
}
},
"INV": {
"0": {
"Temperature": {
"v": 0,
"u": "°C",
"d": 1
}
}
},
"events": 0
}
],
"total": {
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
}
},
"hints": {
"time_sync": false,
"radio_problem": false,
"default_password": true
}
} The different websocket events are merged in the WebUI. I will also extend the e.g. {
"inverters": [
{
"serial": "116480148266",
"name": "HMS-2000-01",
"order": 0,
"data_age": 15,
"poll_enabled": true,
"reachable": true,
"producing": false,
"limit_relative": 0,
"limit_absolute": -1,
"AC": {
"0": {
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"Power DC": {
"v": 0,
"u": "W",
"d": 1
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
},
"Frequency": {
"v": 0,
"u": "Hz",
"d": 2
},
"PowerFactor": {
"v": 0,
"u": "",
"d": 3
},
"ReactivePower": {
"v": 0,
"u": "var",
"d": 1
},
"Efficiency": {
"v": 0,
"u": "%",
"d": 3
}
}
},
"DC": {
"0": {
"name": {
"u": "Panel 1"
},
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
},
"Irradiation": {
"v": 0,
"u": "%",
"d": 3,
"max": 400
}
},
"1": {
"name": {
"u": ""
},
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
}
},
"2": {
"name": {
"u": ""
},
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
}
},
"3": {
"name": {
"u": ""
},
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"Voltage": {
"v": 0,
"u": "V",
"d": 1
},
"Current": {
"v": 0,
"u": "A",
"d": 2
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
}
}
},
"INV": {
"0": {
"Temperature": {
"v": 0,
"u": "°C",
"d": 1
}
}
},
"events": 0
}
],
"total": {
"Power": {
"v": 0,
"u": "W",
"d": 1
},
"YieldDay": {
"v": 0,
"u": "Wh",
"d": 0
},
"YieldTotal": {
"v": 0,
"u": "kWh",
"d": 3
}
},
"hints": {
"time_sync": false,
"radio_problem": false,
"default_password": true
}
} To get a overview list of all inverters just call |
note to other testers: version v24.1.21 doesn't have /api/livedata/status?inv= implemented yet, and will still output the old /api/livedata/status instead of a single inverter. |
@tbnobody thx and greetings |
Not yet.... Most likley I will include it in the next version. I'm anxious as it will break the setup for users who are using the web api (regardless the fact that I wrote it on multiple locations that the API can change). But I think the advantage of a more stable system is worth it. |
If you like to test it.... |
thx 🙏 public note: web live view does not show inverters yet, but it's imho irrelevant for first api testing.
hopefully a little anxiety relief: additional thought: in this current case, it's a bit more tricky, as the endpoint name here is still called but thats imho more work than it would be, to use another parallel endpoint name for the list, e.g. thx and whatever it will be, it's fine with me 👍 |
Sorry I included the wrong version of the WebApp. (You maybe have to manually clear the browser cache or reload with CTRL+F5 now as the git hash is not updated in this test version. |
Am already testing it with openEMS now ! Greetings |
usage info regarding the next api and webapp changes: in case you have an opendtu behind a http reverse proxy, e.g. for data protection,
you can test out, whether your proxy or forwarding supports websocket with following curl command:
same with line breaks for easier reading:
(replace user, password, yoururl and yourport, with your corresponding data) this will make a call to sideinfo: if using https you have to request explicit --http1.1 as the http2 default will bypass the |
removing the HTTP API is not a straight forward process and will result in a non functionable Module in openEMS :( |
this works well for me. now. please tell me the workflow as in the Discussion of the API development there is nothing mentioned about a websocket ? |
That's not true. The Websocket was also used in the previous versions for updating the values. With the new version you can also get all the data using the GET request. You just have to perform it per inverter. You will not get all inverters at once. But on the other hand you are able to get a list of all available inverters. Then you have to loop over all inverter serial numbers and perform a request for each serial. And yes, I am aware that this change will break some other integrations which rely on this json output. But we can either stop further development and leave it as it is or we can change it, and therefor also the different connectors e.g. for openEMS. I also mentioned on every Documentation of the WebAPI that it is mainly for the WebUI and may change in a way that it suits best for the WebUI. |
So basically the Endpoint /api/inverter/id/<inverter_id> will remain the same as i already implemented it now in a dev version using your dev version: https://github.com/tbnobody/OpenDTU/files/14075558/opendtu-generic_esp32.zip Greetz |
Not sure if we understand us correctly... I never had the intention to remove the WebAPI. Just the result and the parameters are different. (As mentioned here) |
never mind, just posted a prophylactic how to, if others might have their dtu behind reverse proxies, as this was needed to get the inverter details visible outside of home, as it was working with the current and past versions. @Sn0w3y i think you've confused the api projects |
@MetaChuh if you have a full vhost as an example I would add it in the howto section in the documentation (apache vs. nginx does not matter) |
I think this issue can be closed. Memory checks are (moreless) implemented and also the memory consumption is much less since the updated livedata API. |
yes thx. |
This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion or issue for related concerns. |
What happened?
I observed this error on openDTU-onBattery, but it should be fixed in the upstream project. So I open the issue here.
DynamicJsonDocument
is used to create datasets for the Web-API, MQTT messaging and to backup config data.Problem is, that
DynamicJsonDocument
doesn't throw an exception, if memory allocation fails. This results in strange and unexpected (sometimes temporary) behavior of openDTU during runtime.null
Since this error is not handled, there is no logging of this resource problem.
To Reproduce Bug
Extend
INV_MAX_COUNT
significantly and you will never see live data, nor you will get any logging.The try block
std::bad_alloc
catches errors fromserializeJason()
, when there is not enough memory forDynamicJsonDocument
but not enough memory for serialization.Expected Behavior
At least I would expect some logging for live view and mqtt that there is not enough memory.
Config files should never be saved when they are corrupted.
Install Method
Self-Compiled
What git-hash/version of OpenDTU?
7946dfc
Relevant log/trace output
No response
Anything else?
After each call of
DynamicJsonDocument()
there should be a test withDynamicJsonDocument::capacity != 0
.During my tests I used:
ESP.getFreeHeap()
ESP.getMaxAllocHeap()
While both functions tells me that there is enough memory
DynamicJsonDocument::capacity()
returns 0. This may be due to my particular Esp32, as I suspect that the flash memory is no longer working properly. I have to desolder it and replace it with a new Esp32.The text was updated successfully, but these errors were encountered: