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

Implement non blocking OTA update over MQTT #142

Merged
merged 50 commits into from
Sep 3, 2023

Conversation

MathewHDYT
Copy link
Contributor

@MathewHDYT MathewHDYT commented Jul 14, 2023

This pull request includes adjustment to allow the sending of messages of an arbitrary size with the usage of the StreamUtils. This is only done if the THINGSBOARD_ENABLE_STREAM_UTILS flag is set. This feature has been added to the documentation and to the error messages that gets printed if the given payload is bigger than the internal PubSubClient buffer size would allow.


Additionally it includes a complete overhaul of the OTA mechanism, separating it into it's own class and making it non blocking, this additionally makes it possible to implement the reconnect on failure and simply continue the OTA update at the same point where it was disconnected.

Fixes #132. Furthermore this makes it possible to do other stuff besides the OTA update and should be overall less CPU intensive, because other tasks might get some more runtime as well.

This also allowed integrating the functionality of stopping the OTA update if wanted by the user, for that a new method Stop_Firmware_Update can be called.


Furthermore the provision example has been fixed to make the MAC_FALLBACK actually work fixes #141.


Also some OTA states have been changed to be more useful, fixes #144.


The overhaul and removal of some duplicate code additionally allowed for the Send_Json and Send_Json_String method, which can now be used to send data to any topic, resulting in enabling users to implement their own functionality easier fixes #46 to some degree.


Functionality to send booleans with the Telemetry data has been readded to fix the issue of them being implicitly converted to integers and therefore sent as 0 and 1 instead of true and false. Fixes #143. Additionally the library now allows for sending bigger data types with the sendTelemetry or sendAttribute method, where each integer type is up to int64_t and each floating type is up to double precision.


Additionally the OTA Update can now be used with any board that supports the C++ STL base functionality, as long as the user implements their own way to write the given binary data on the device. This can be done with an interface implementation that is passed into the OTA_Update_Callback class. This should also fixes #100 to some degree.

Lastly I bumped the version number to 0.11.0, because there have been some breaking API changes @imbeacon would be nice if this pull request could be merged.

@MathewHDYT
Copy link
Contributor Author

MathewHDYT commented Jul 14, 2023

For the internal buffer size, that would need to be tested as well, because at least on my device it failed to allocate the memory with the malloc call if it was more than 64KB.

If this is an actual issue than we might need to implement the direct streaming from ArduinoJson data to the PubSubClient anyway, but it could perhaps only be done if the memory passes a certain threshold. For example how it is currently done under 1KB is on the stack over 1KB is on the heap and perhaps in the future over 64KB could use the ArduinoStreamUtils.

@imbeacon
Copy link
Member

Hi @MathewHDYT ,

Thanks for your contribution.
According to Streams implementation - we can use them as we tried before to increase the size of the message that we can deliver. I have tried and 65535 is a size limit for one message and we cannot pass more. Looks like it is necessary to use streams if big messages should be send.
It looks like ArduinoJson can be used with PubSubClient, according to ArduinoJson documentation.
What do you think about sending all messages as streams? It may be useful and removes limitation from the library side.

@MathewHDYT
Copy link
Contributor Author

@imbeacon Not necesserality sure about always using streams, I think making it an option that has to be enabled is probably the best idea.

Mainly because as we saw simply sending the data itself is a lot faster and the additional library does require more space in the library.

I think the best idea would be to make the usage of Stream an configurable option, meaning the library is only included if the option is set and if it is the passable amount of data is up to the uint32_t max, instead of the uint16_t max. Where from the uint16_t max to the uint32_t max the Stream is used to send the data.

This allows devices to not use this feature if it isn't needed a lot of devices will never need to send more than 65'000 characters at once.

And the devices that do need it can simply enable it once via configuration flags, similar to how the other flags are handled.

@imbeacon
Copy link
Member

@MathewHDYT ,

Yes, I agree, it may be optional, we can use it if message size > 65000 and we received a json with size more then 65000.

@MathewHDYT
Copy link
Contributor Author

MathewHDYT commented Jul 17, 2023

@imbeacon The part with receiving might be a bigger problem to change, because that would need to be done in the PubSubClient, and receiving that amount of data also doesn't seem to work.

Atleast it didn't work for my ESP32 when trying the OTA update with a packet size of over 64KB.

I don't think we are able to deserialize that amount of data because we have to store it somewhere and the buffer can't be increased that much because the malloc call seems to fail.

Perhaps it is possible to fix that underlying issue, but if it isn't than receiving that amount of data will not work only sending it will be possible.


For the serialisation that's the relevant information, as said in it directly, serialising would be 100-200 times slower than copying and sending directly. If the Stream classes are used it can be increased but it also increases the complexity by a lot.

https://arduinojson.org/v6/how-to/use-arduinojson-with-pubsubclient/

I think the best case would be if the PubSubClient would work with the size_t buffer, but I'm not even sure why I a malloc call of 64KB doesn't fail but one byte more does.

@imbeacon
Copy link
Member

@MathewHDYT

But to control chunk size we can use publish to firmware topic - v2/fw/request/${requestId}/chunk/${chunkIndex}
If this publish contains a number - this number will be taken and set as chunkSize. And chunk will have lower or requested size.

@MathewHDYT
Copy link
Contributor Author

MathewHDYT commented Jul 17, 2023

@imbeacon What I meant is that I know how big the requested chunks are, but if we receive more than 64KB from the server we can't handle that message, because the buffer fails to allocate enough memory for it.

Meaning packets bigger than 64KB can not be received.

So if possible the issue that would perhaps be the best idea to fix is the malloc call with more than 64KB because theoretically there should be enough memory to allocate that much in one continuous block.

Especially if 64KB works but 1 byte more doesn't anymore, that is the part that confuses me.


Because if the above mentioned fix is implemented the bigger messages will work but only for sending but not for receiving.

There is a heap_caps_register_failed_alloc_callback, perhaps that can help find out why the allocation fails.

There is also heap_caps_get_largest_free_block, which might indicate the actual memory that could be used and if it is bigger than the requested amount but the allocation still fails there might be another issue alotogether.

@imbeacon
Copy link
Member

It is a maximum size of a TCP packet.

@MathewHDYT MathewHDYT mentioned this pull request Jul 24, 2023
@MathewHDYT
Copy link
Contributor Author

@imbeacon Would be nice if you could test the current version on your device and if it works merge the pull request.

@MathewHDYT
Copy link
Contributor Author

@imbeacon Would be nice if this could be merged sometime soon, because I am currently working on version 0.12.0. Which has separated the direct dependency on PubSubClient with interfaces.

The goal would be to allow using this library for Espressif IDF uses or other boards more easily as well.

@imbeacon imbeacon merged commit 5a5d27a into thingsboard:master Sep 3, 2023
3 of 6 checks passed
@MathewHDYT
Copy link
Contributor Author

@imbeacon Thanks a lot for the merge, would be nice if you could make a release on GitHub and Arduino as well so that people can fetch the newest version 0.11.0.

@MathewHDYT
Copy link
Contributor Author

@imbeacon Would be nice if you could make a release of version 0.11.0 so people can fetch the newest changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants