Bugfix + test code - if a property is manipulated in its onChange callback function to its origin state the change is still propagated to the cloud

Author: aentinger

src/ArduinoCloudProperty.hpp

@@ -77,7 +77,7 @@ class ArduinoCloudProperty {
   inline bool   isReadableByCloud () const { return (_permission == Permission::Read ) || (_permission == Permission::ReadWrite); }
   inline bool   isWriteableByCloud() const { return (_permission == Permission::Write) || (_permission == Permission::ReadWrite); }
 
-  bool shouldBeUpdated        () const;
+  bool shouldBeUpdated        ();
   void execCallbackOnChange   ();
 
   void append                 (CborEncoder * encoder, CloudProtocol const cloud_protocol);
@@ -91,7 +91,8 @@ class ArduinoCloudProperty {
   UpdateCallbackFunc _update_callback_func;
 
   UpdatePolicy       _update_policy;
-  bool               _has_been_updated_once;
+  bool               _has_been_updated_once,
+                     _has_been_modified_in_callback;
   /* Variables used for UpdatePolicy::OnChange */
   T                  _min_delta_property;
   unsigned long      _min_time_between_updates_millis;

src/ArduinoCloudProperty.ipp

@@ -11,6 +11,7 @@ ArduinoCloudProperty<T>::ArduinoCloudProperty(T & property, String const & name,
   _update_callback_func(NULL),
   _update_policy(UpdatePolicy::OnChange),
   _has_been_updated_once(false),
+  _has_been_modified_in_callback(false),
   _min_delta_property(getInitialMinDeltaPropertyValue()),
   _min_time_between_updates_millis(0),
   _last_updated_millis(0),
@@ -52,9 +53,14 @@ ArduinoCloudProperty<T> & ArduinoCloudProperty<T>::publishEvery(unsigned long co
 }
 
 template <typename T>
-bool ArduinoCloudProperty<T>::shouldBeUpdated() const {
+bool ArduinoCloudProperty<T>::shouldBeUpdated() {
   if(!_has_been_updated_once) return true;
 
+  if(_has_been_modified_in_callback) {
+    _has_been_modified_in_callback = false;
+    return true;
+  }
+
   if     (_update_policy == UpdatePolicy::OnChange) {
     return (isValueDifferent(_property, _shadow_property) && ((millis() - _last_updated_millis) >= (_min_time_between_updates_millis)));
   }
@@ -72,6 +78,10 @@ void ArduinoCloudProperty<T>::execCallbackOnChange() {
     if(_update_callback_func != NULL) {
       _update_callback_func();
     }
+
+    if(!isValueDifferent(_property, _shadow_property)) {
+      _has_been_modified_in_callback = true;
+    }
   }
 }
 

test/src/test_callback.cpp

@@ -64,3 +64,43 @@ SCENARIO("A callback is registered via 'onUpdate' to be called on property chang
 
   /************************************************************************************/
 }
+
+/**************************************************************************************/
+
+static bool switch_turned_on       = false;
+static bool switch_callback_called = false;
+
+void switch_callback()
+{
+  switch_turned_on       = false;
+  switch_callback_called = true;
+}
+
+SCENARIO("A (boolean) property is manipulated in the callback to its origin state", "[ArduinoCloudThing::decode]")
+{
+  GIVEN("CloudProtocol::V1")
+  {
+    ArduinoCloudThing thing(CloudProtocol::V1);
+    thing.begin();
+    encode(thing);
+
+    thing.addPropertyReal(switch_turned_on, "switch_turned_on", Permission::ReadWrite).onUpdate(switch_callback);
+
+    /* [{"n": "switch_turned_on", "vb": true}] = 81 A2 61 6E 70 73 77 69 74 63 68 5F 74 75 72 6E 65 64 5F 6F 6E 62 76 62 F5 */
+    uint8_t const payload[] = {0x81, 0xA2, 0x61, 0x6E, 0x70, 0x73, 0x77, 0x69, 0x74, 0x63, 0x68, 0x5F, 0x74, 0x75, 0x72, 0x6E, 0x65, 0x64, 0x5F, 0x6F, 0x6E, 0x62, 0x76, 0x62, 0xF5};
+    int const payload_length = sizeof(payload)/sizeof(uint8_t);
+    thing.decode(payload, payload_length);
+
+    REQUIRE(switch_callback_called == true);
+
+    /* Since the property was reset to its origin state in the callback we
+     * expect that on the next call to encode this change is propagated to
+     * the cloud.
+     */
+
+    /* [{"n": "switch_turned_on", "vb": false}] = 81 BF 61 6E 70 73 77 69 74 63 68 5F 74 75 72 6E 65 64 5F 6F 6E 62 76 62 F4 FF */
+    std::vector<uint8_t> const expected = {0x81, 0xBF, 0x61, 0x6E, 0x70, 0x73, 0x77, 0x69, 0x74, 0x63, 0x68, 0x5F, 0x74, 0x75, 0x72, 0x6E, 0x65, 0x64, 0x5F, 0x6F, 0x6E, 0x62, 0x76, 0x62, 0xF4, 0xFF};
+    std::vector<uint8_t> const actual = encode(thing);
+    REQUIRE(actual == expected);
+  }
+}