Avoid multivalue properties split #293
Conversation
Codecov Report
@@ Coverage Diff @@
## master #293 +/- ##
==========================================
- Coverage 95.40% 94.89% -0.51%
==========================================
Files 26 27 +1
Lines 1022 1116 +94
==========================================
+ Hits 975 1059 +84
- Misses 47 57 +10
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
I think my most important feedback it to try and redesign the encoding logic into a FSM with named states, so that it's very clear in which state you are at all times and also the transitioning paths between the states are clearly visible. For a sample FSM pattern see here. 🙇 As always it's up to you to act on my feedback, please see it as a recommendation, not a request.
src/cbor/CBOREncoder.cpp
Outdated
| for(; iter != property_container.end(); iter++) | ||
| { | ||
| Property * p = * iter; | ||
| bool maximum_number_of_properties_reached = (num_encoded_properties >= encoded_properties_message_limit) && (encoded_properties_message_limit != -1); |
There was a problem hiding this comment.
| bool maximum_number_of_properties_reached = (num_encoded_properties >= encoded_properties_message_limit) && (encoded_properties_message_limit != -1); | |
| bool const maximum_number_of_properties_reached = (num_encoded_properties >= encoded_properties_message_limit) && (encoded_properties_message_limit != -1); |
Use const-correctness as much as possible 😉
src/cbor/CBOREncoder.cpp
Outdated
| { | ||
| Property * p = * iter; | ||
| bool maximum_number_of_properties_reached = (num_encoded_properties >= encoded_properties_message_limit) && (encoded_properties_message_limit != -1); | ||
| bool cbor_encoder_error = (error != CborNoError); |
There was a problem hiding this comment.
| bool cbor_encoder_error = (error != CborNoError); | |
| bool const cbor_encoder_error = (error != CborNoError); |
Same as above. Btw, I love it that you are "encoding" the meaning of a boolean operation in the name of the boolean variable. Makes the if statement below so much more easily readable.
There was a problem hiding this comment.
Same as above also fo me.
src/cbor/CBOREncoder.cpp
Outdated
| if (CborNoError != error) | ||
| { | ||
| /* Trim the number of properties to be included in the next message to avoid error closing container */ | ||
| encoded_properties_message_limit = num_encoded_properties - 1; |
There was a problem hiding this comment.
The change feels like it uses encoded_properties_message_limit as both counter AND status variable. Can you think of a way to change the PR to encoded_properties_message_limit only as a counter (and possibly rename it) and use a boolean variable to cover the cases CBOR_ENCODER_NO_PROPERTIES_LIMIT and -1. If num_encoded_properties - 1 is also a valid state than this should be encoded as a FSM making the states and state transitions explicit. Right now states and state transitionings are hidden in the business logic and really hard to see.
There was a problem hiding this comment.
In the FSM here eedffb3 i've added a bool variable property_limit_active separate from the value encoded_property_limit.
And the two condition are handled in two separate states : TrimAppend and TrimClose
got it! i will take some time to think about it, but i agree that splitting the encoding process in basic operation would make the code more readable and the logic more explicit!. Thanks. |
|
Great 👍 There's one thing that came to my mind additionally, which is that I'd suggest to capture the desired "output" within a unit test, i.e. you replicate your scenario where a multi-value property would be cut off, but isn't due to this change. Then you've got a much easier time refactoring this code, since you can run the tests after every change and be assured, that everything is still working as it should 😉 |
pennam
force-pushed
the
avoid_multi_split
branch
3 times, most recently
from
1856d57 to
8105d69
Compare
|
Memory usage change @ 8105d69
Click for full report table
Click for full report CSV |
Added a test here c620ad1 |
|
Hi @aentinger did you had a chance to look at this commit eedffb3 |
per1234
added
topic: code
|
Good Morning @pennam ☕ 👋 An example: Neither void handle_Prepare()
{
handle_InitPropertyEncoder(propertyEncoder);
handle_OpenCBORContainer(propertyEncoder, data, size);
/* ... */
}Otherwise you are pretty much there. |
src/ArduinoIoTCloudLPWAN.cpp
Outdated
| @@ -149,8 +149,9 @@ void ArduinoIoTCloudLPWAN::sendPropertiesToCloud() | |||
| { | |||
| int bytes_encoded = 0; | |||
| uint8_t data[CBOR_LORA_MSG_MAX_SIZE]; | |||
| static unsigned int last_checked_property_index = 0; | |||
There was a problem hiding this comment.
This now is where you save the state for the CBOREncoder::encode state machine. I'd suggest to rather store it as a protected (not private, because both ArduinoIoTCloudLPWAN and ArduinoIoTCloudTCP need to access it) member variable of class ArduinoIoTCloud. Otherwise you've just created a hidden state variable. In my experience, everything that controls state in a C++ class should be listed as a member in the class body.
There was a problem hiding this comment.
now i got it! plese see 5576a60
library.properties
Outdated
| @@ -1,5 +1,5 @@ | |||
| name=ArduinoIoTCloud | |||
| version=1.3.1 | |||
| version=1.4.0 | |||
There was a problem hiding this comment.
Do you bump the library version number in this PR? It's of course possible, but the/my ususal flow is to merge the PR, than make a dedicated "release" commit.
There was a problem hiding this comment.
ups, this was due to a wrong rebase, 1.4.0 is already released
src/ArduinoIoTCloudTCP.cpp
Outdated
| @@ -596,12 +596,14 @@ void ArduinoIoTCloudTCP::sendPropertyContainerToCloud(PropertyContainer & proper | |||
|
|
|||
| void ArduinoIoTCloudTCP::sendPropertiesToCloud() | |||
| { | |||
| sendPropertyContainerToCloud(_property_container); | |||
| static unsigned int last_checked_property_index = 0; | |||
There was a problem hiding this comment.
Again, this is hidden state. Please consider moving it to the class body.
…f the whole property cannot be appended to the CBOR buffer. Doing so the complete CBOR message will be discarded.
…essage that may be discarded
…e property, do not advance the property index and mark discarded properties has appended but not sended. Compute the exact number of properties that would fit the CBOR buffer and retry send next loop. When no error occurs restore property limit to NO_LIMIT and advance property index.
… properties to be appended by one and retry send
…loud even if not changed in callback
pennam
force-pushed
the
avoid_multi_split
branch
from
8105d69 to
78b3158
Compare
I did not group this two states because even if they cannot fail they are doing different things. I've grouped |
|
Memory usage change @ 78b3158
Click for full report table
Click for full report CSV |
|
Makes sense 👍 Thank you very much for acting on my feedback 🙇. |
Thanks for your help @aentinger 💪 |
Problem:
with the current logic of
CBORmessage creation is actually possible that a multivalue property is splitted in two consecutive messages. This would break any multivalue property widget in create dashboard because is expected that all the values of the same multivalue property are updated at the same time.Proposed solution:
Adding
CHECK_CBOR_MULTImacro is possible to throw aCborErrorTooManyItemserror when the whole multivalue property does not fit into theCBORmessage. As a consequence theCBORmessage is discarded (not sended to the cloud) and all the properties included in the discarded message and markes asappended_but_not_sendedare scheduled for re-send. Furthermore theencoded_properties_message_limitis updated in order to send the correct number of properties to avoid re-trigger the error. The excluded property will be then sended in the next message. The same logic applies when the error occurs closing theCBORcontainer.First run:
Second run:
Third run:
Other changes:
Commits eb41da8 and ab527a1 are addressing a different but in some way correlated issue. If the propety container has a number of high change rate properties in the first positions and this properties are filling the
CBORmessage buffer these high change rate properties are updated eachsendPropertiesToCloudcall leaving out the other properties.This can be easily reproduced creating a sketch with 18 float properties and a loop like the one below.
atest_12is never updated.Lastly since i've added the
appended_but_not_sendedi've also included commit 14eb479 that does not change any behaviour of the library, but it makes a bit clear that we need to echo back the received properties to the cloud even if unchanged./cc @eclipse1985