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

Update allocation strategy to stack only #180

Merged
merged 127 commits into from
Jun 7, 2024

Conversation

MathewHDYT
Copy link
Contributor

@MathewHDYT MathewHDYT commented Dec 25, 2023

Internal library implementation as well as internal private members have been adjusted to utilise the stack as far as possible.

Meaning if THINGSBOARD_ENABLE_DYNAMIC is not enabled, then the internal callbacks will be saved into an array on the stack. Additionally, internally subscribed strings for Attribute_Request_Callback and Shared_Attribute_Callback are now on the stack as well. Furthermore the internal implementations have been adjusted to not use std::string or String, but instead allocate on the stack and use the appropriate c-variant of those methods whenever feasible (strncat instead of substring).

The only remaining heap allocations are when sending JSON data if the total size of the produced string is bigger than 1024 characters, but that heap allocation is very short lifed, as well as heap allocation when receiving binary OTA payload. Same here really short lived and only allocated on the heap if it is bigger than 1024 characters. Lastly the internal implementation of the MQTT buffer is also always on the heap, but we do not implement that and that size is allocated once at startup and normally selldomly changed so it should not cause fragmentation either.

All those integration does increase the difficulty of using the library slightly (additional template parameters), but all examples and the ReadMe.md have been adjusted accordingly to reflect the changes and the overall decrease in heap usage should make the library more stable and faster.


Also I added the automatic compilation of the internal documentation (doxygen comments) into a doxygen webpage. That should help people read the aforementioned comments and inform themselves about features and how to use the library. The only thing that would be required to display that documentation is to enable GitHub Pages.


Furthermore, multiple bugs have been fixed.

Fixes #193

Usage of the reserve method has been removed, because depending on the amount of MaxFieldAmount if allocated a lot of memory on the heap even if it was never required. Replaced with Arrays and fixed size allocation and dynamic growing in Dynamic Thingsboard mode.

Fixes #159 and fixes #191

Adjust internal implementation to take OTA_Update_Callback class by value again. Fixing imcompatiblites with the examples which expect that value is copied, because it is only created locally and shortly in the method on the stack.

Fixes #167 and fixes #189 and fixes #194 and fixes #200

Thanks to the library now being the owner of the response JSON, instead of the callback method it removed the possibility for dangling reference. Especially because the examples produced that issue as soon as you attempted to return more than one value from the RPC callback.

Fixes #164

The THINGSBOARD_ENABLE_STREAM_UTILS produced some issues, with the internally used macros that caused methods to not be linked correctly InstrFetchProhibited exception. I adjusted the internal macro to fix the aforementioned issue and additional had to slightly adjust the implementation of how data is sent with StreamUtils. After those changes it is now possible again to compile with the aforementioned flag enabled and it does send bigger data than the configured buffer size correctly.

Fixes #169

Added check if invalid firmware size is received to immediately fail the received packet and attempt to reestablish a correct connection. Makes clearer that the received behaviour is invalid and something was configured wrongly.

Fixes #170

Updated internal library documentation to clear up misunderstanding of difference between Process OTA MQTT and Subscribe OTA MQTT.

Fixes #171

Adjust espressif components to only be enabled if they additionally have the required version (atleast v4.X.X).

Fixes #179

Once the library has been integrated and released as v0.13.0 the registries should be updated as well and contain the fixed dependency files.

Fixes #176

Documentation on custom logger implementation has been improved.

Fixes #174

To remove the need to update the espressif registry by hand. I've added a workflow that will automatically push the current code if a tag is created, which happens if a new package is released.

The only remaining thing that needs to be done is that this repository in security needs to have IDF_COMPONENT_API_TOKEN registered so it is possible for the action to upload. See Managing secrets for more information on how to add the secret.

Fixes #183

Added additional log messages to print the firmware title and version if the comparison failed and additional ensured that the ESP8266 always disables PROGMEM support. Has to be specifically enabled for the ESP8266 now instead.

Fixes #186

Fixed the newest version of ArduinoJson being used to compile the examples which is a new major version and includes breaking changes. Instead the version is now fixed to the latest release of v6.X.X. The same has been done for ArduinoJson library that is automatically installed when using Arduino IDE. Read this comment for more information on why upgrading to v7.X.X is not a good idea and why the version has been fixed to v6.21.4 for now.

Fixes #190

Adjusted menuconfig to be cleared and shorter.

Fixes #201

Add note in example where THINGSBOARD_ENABLE_DYNAMIC is used to make clear that if DeserializationError occur it might be because the board does not support or does not have PSRAM and how to fix it.

Fixes #202

Because the library does not force the use of ThingsBoard PubSubClient anymore allowing to use forks that fix specific issues with the aforementioned library.

Fixes #155

Added additional default implementation of IUpdater that allows to update to an intermediate file (which can be on the SD card), which then allows to write the binary into flash memory at a later point in time. Adjusted Espressif example to include the required code.


@imbeacon Would be nice if this could be merged and then released on plattformio, arduino and espressif-registry as v0.13.0.

@MathewHDYT
Copy link
Contributor Author

@imbeacon, @ashvayka Would be nice if this Pull Request could be merged, because it fixes a lot of currently open issues.

Additionaly it would be nice if it could then be released as v0.13.0.

Furthermore it would be nice if GitHub Pages could be enabled.

And it would be nice if IDF_COMPONENT_API_TOKEN could be configured, especially before releasing the version.
See Managing secrets for more information on how to add the secret.

@imbeacon
Copy link
Member

Hi @MathewHDYT,

Thank you for your participating, at the moment I'm adopting our examples on devices library to use the new version of the library and then I will be able to merge it.

@MathewHDYT
Copy link
Contributor Author

@imbeacon Ah good to know thanks a lot for the fast response.

@MathewHDYT
Copy link
Contributor Author

@imbeacon How is it going with the merge, because there have been multiple occurences of issues being opened for problems that are already fixed in this Pull Request since it has been opened 2 months ago.

Would be nice if this could be merged as soon as possible.

@leoddias
Copy link

leoddias commented Mar 23, 2024

Internal library implementation as well as internal private members have been adjusted to utilise the stack as far as possible.

Meaning if THINGSBOARD_ENABLE_DYNAMIC is not enabled, then the internal callbacks will be saved into an array on the stack. Additionally, internally subscribed strings for Attribute_Request_Callback and Shared_Attribute_Callback are now on the stack as well. Furthermore the internal implementations have been adjusted to not use std::string or String, but instead allocate on the stack and use the appropriate c-variant of those methods whenever feasible (strncat instead of substring).

The only remaining heap allocations are when sending JSON data if the total size of the produced string is bigger than 1024 characters, but that heap allocation is very short lifed, as well as heap allocation when receiving binary OTA payload. Same here really short lived and only allocated on the heap if it is bigger than 1024 characters. Lastly the internal implementation of the MQTT buffer is also always on the heap, but we do not implement that and that size is allocated once at startup and normally selldomly changed so it should not cause fragmentation either.

All those integration does increase the difficulty of using the library slightly (additional template parameters), but all examples and the ReadMe.md have been adjusted accordingly to reflect the changes and the overall decrease in heap usage should make the library more stable and faster.

Also I added the automatic compilation of the internal documentation (doxygen comments) into a doxygen webpage. That should help people read the aforementioned comments and inform themselves about features and how to use the library. The only thing that would be required to display that documentation is to enable GitHub Pages.

Furthermore, multiple bugs have been fixed.

Fixes #193

Usage of the reserve method has been removed, because depending on the amount of MaxFieldAmount if allocated a lot of memory on the heap even if it was never required. Replaced with Arrays and fixed size allocation and dynamic growing in Dynamic Thingsboard mode.

Fixes #159 and fixes #191

Adjust internal implementation to take OTA_Update_Callback class by value again. Fixing imcompatiblites with the examples which expect that value is copied, because it is only created locally and shortly in the method on the stack.

Fixes #167 and fixes #189 and fixes #194

Thanks to the library now being the owner of the response JSON, instead of the callback method it removed the possibility for dangling reference. Especially because the examples produced that issue as soon as you attempted to return more than one value from the RPC callback.

Fixes #164

The THINGSBOARD_ENABLE_STREAM_UTILS produced some issues, with the internally used macros that caused methods to not be linked correctly InstrFetchProhibited exception. I adjusted the internal macro to fix the aforementioned issue and additional had to slightly adjust the implementation of how data is sent with StreamUtils. After those changes it is now possible again to compile with the aforementioned flag enabled and it does send bigger data than the configured buffer size correctly.

Fixes #169

Added check if invalid firmware size is received to immediately fail the received packet and attempt to reestablish a correct connection. Makes clearer that the received behaviour is invalid and something was configured wrongly.

Fixes #170

Updated internal library documentation to clear up misunderstanding of difference between Process OTA MQTT and Subscribe OTA MQTT.

Fixes #171

Adjust espressif components to only be enabled if they additionally have the required version (atleast v4.X.X).

Fixes #179

Once the library has been integrated and released as v0.13.0 the registries should be updated as well and contain the fixed dependency files.

Fixes #176

Documentation on custom logger implementation has been improved.

Fixes #174

To remove the need to update the espressif registry by hand. I've added a workflow that will automatically push the current code if a tag is created, which happens if a new package is released.

The only remaining thing that needs to be done is that this repository in security needs to have IDF_COMPONENT_API_TOKEN registered so it is possible for the action to upload. See Managing secrets for more information on how to add the secret.

Fixes #183

Added additional log messages to print the firmware title and version if the comparison failed and additional ensured that the ESP8266 always disables PROGMEM support. Has to be specifically enabled for the ESP8266 now instead.

Fixes #186

Fixed the newest version of ArduinoJson being used to compile the examples which is a new major version and includes breaking changes. Instead the version is now fixed to the latest release of v6.X.X. The same has been done for ArduinoJson library that is automatically installed when using Arduino IDE. Read this comment for more information on why upgrading to v7.X.X is not a good idea and why the version has been fixed to v6.21.4 for now.

Fixes #190

Adjusted menuconfig to be cleared and shorter.

@imbeacon Would be nice if this could be merged and then released on plattformio, arduino and espressif-registry as v0.13.0.

Hey @MathewHDYT !! I'm here just to thank you for this, I was struggling on this issue, I found it by searching the "magic number" 1.695175409, your workaround solved my problem.

StaticJsonDocument<JSON_OBJECT_SIZE(1)> relayResponse;

RPC_Response getRelay(const RPC_Data &data)
{
  relayResponse.clear();
  relayResponse["enabled"] = relayState1;
  return RPC_Response(relayResponse);
}

🙏🙏

@alkonosst
Copy link

Hi guys, hope you are doing great. Is there any estimate date to merge this pull request? I've encountered the same people's issues and fixed with workarounds.

Also, is there any plan to include MQTT Gateway API in the future? Now I have a project with multiple LoRa devices and 1 device as a Gateway. I think I will fork the repo and add some methods to use that API capabilities and test it.

Thank you for your work 👌

@MathewHDYT
Copy link
Contributor Author

MathewHDYT commented Apr 8, 2024

Hi guys, hope you are doing great. Is there any estimate date to merge this pull request? I've encountered the same people's issues and fixed with workarounds.

Also, is there any plan to include MQTT Gateway API in the future? Now I have a project with multiple LoRa devices and 1 device as a Gateway. I think I will fork the repo and add some methods to use that API capabilities and test it.

Thank you for your work 👌

For the merge request, I sadly do not now when it will be implemented. Biut I hope it is soon as well @imbeacon, @ashvayka.

For the Gateway API is a planned feature, but I currently do not have a lot of time to add additional features. Therefore It would probably need a few months until I could start.

But if you want you add the basic skeleton for the Gateway API, that is very much appreciated. If help is required I can give some imput :D.

@MathewHDYT
Copy link
Contributor Author

@imbeacon, @ashvayka Any progress on the pull request? Would be nice if this could be merged, most library issues would be solved and the pull request has been open for 6 months now.

@imbeacon imbeacon merged commit 3647e80 into thingsboard:master Jun 7, 2024
5 of 9 checks passed
@MathewHDYT
Copy link
Contributor Author

@imbeacon

Would be nice if GitHub Pages could be enabled for this repository, because this PR added the automatic compilation of the internal documentation (doxygen comments) into a doxygen webpage. That should help people read the aforementioned comments and inform themselves about features and how to use the library.

Additonal to remove the need to update the espressif registry by hand. I've added a workflow that will automatically push the current code if a tag is created, which happens if a new package is released.

The only remaining thing that needs to be done is that this repository in security needs to have IDF_COMPONENT_API_TOKEN registered so it is possible for the action to upload. See Managing secrets for more information on how to add the secret.

@imbeacon
Copy link
Member

imbeacon commented Jun 7, 2024

@MathewHDYT

Yes, I see, but unfortunately it looks like GitHub action is not working due to expiration of node 16 support, I updated esp component manually.

Whenever thank you for your contributions and patience with this PR.

@MathewHDYT
Copy link
Contributor Author

MathewHDYT commented Jun 7, 2024

@imbeacon The automatic deployment does not fail, because of the Node 16 support that is only a warning. It fails because of this error ERROR: Failed to get API Token from the config file.

To fix it you need to add the IDF_COMPONENT_API_TOKEN to the security tab in this repository like seen here Managing secrets.

And adding GitHub Pages should be a simple switch in the respository settings and would be highly appreciated :D.

@imbeacon
Copy link
Member

imbeacon commented Jun 7, 2024

@imbeacon The automatic deployment does not fail, because of the Node 16 support that is only a warning. It fails because of this error ERROR: Failed to get API Token from the config file.

To fix it you need to add the IDF_COMPONENT_API_TOKEN to the security tab in this repository like seen here Managing secrets.

And adding GitHub Pages should be a simple switch in the respository settings and would be highly appreciated :D.

I will ask for permissions to do this.

@MathewHDYT
Copy link
Contributor Author

Sure no problem :D. Would be nice if this could be integrated because it should remove the hassle of deploying for Espressif by hand every time and ensures it is always up to date.

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