Fix Sending Wrong Values on Failed Conversion to DPT (#30)

* Fix `valueNoSendCompare`: DPT Conversion Fail Resulted in Setting KO to 0 * Fix `valueNoSend`: Do NOT End KO Uninitialized when Value Conversion Failed * Fix: Do NOT Send when Updating KO Failed on Value Conversion * Group-Object Value Update: Return Indication for Success/Fail of Conversion to DPT * Update Changelog: #30 Fix and Signature Change of `GroupObject` Value Update
OpenKNX · Nov 18, 2024 · 2cd5a28 · 2cd5a28
1 parent 677a645
commit 2cd5a28
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 24 deletions.
diff --git a/README.md b/README.md
@@ -16,6 +16,11 @@ See the examples for basic usage options
 ## Changelog
 
 ### v1dev (replace this with version and date when releasing to v1)
+- Fix [#30](https://github.com/OpenKNX/knx/pull/30): Unexpected behaviour of `GroupObject` on failed conversion to DPT
+  - `GroupObject::value[No]SendCompare(..)` resulted in value 0 (and returned change based on this value)
+  - `GroupObject::valueNoSend(..)` updated state from unitialized to OK, without updating the value
+  - `GroupObject::value(..)` wrote to GA without setting the KO value
+- Extension [#30](https://github.com/OpenKNX/knx/pull/30): Return successful conversion to DPT on values update operations in `GroupObject` (changed result-type of some methods from `void` to `bool`) 
 - only set pinMode of Prog button pin if valid (PROG_BUTTON_PIN >= 0)
 - Strings are now \0 terminated in group objects (#25)
 - change defines in the rp2040 plattform for LAN / WLAN usage to KNX_IP_LAN or KNX_IP_WIFI, remove KNX_IP_GENERIC

diff --git a/src/knx/group_object.cpp b/src/knx/group_object.cpp
@@ -223,10 +223,15 @@ GroupObjectUpdatedHandler GroupObject::callback()
 }
 #endif
 
-void GroupObject::value(const KNXValue& value, const Dpt& type)
+bool GroupObject::value(const KNXValue& value, const Dpt& type)
 {
-    valueNoSend(value, type);
-    objectWritten();
+    if (valueNoSend(value, type))
+    {
+        // write on successful conversion/setting value only
+        objectWritten();
+        return true;
+    }
+    return false;
 }
 
 
@@ -261,9 +266,9 @@ bool GroupObject::tryValue(KNXValue& value)
 }
 
 
-void GroupObject::value(const KNXValue& value)
+bool GroupObject::value(const KNXValue& value)
 {
-    this->value(value, _datapointType);
+    return this->value(value, _datapointType);
 }
 
 
@@ -273,34 +278,42 @@ KNXValue GroupObject::value()
 }
 
 
-void GroupObject::valueNoSend(const KNXValue& value)
+bool GroupObject::valueNoSend(const KNXValue& value)
 {
-    valueNoSend(value, _datapointType);
+    return valueNoSend(value, _datapointType);
 }
 #endif
 
-void GroupObject::valueNoSend(const KNXValue& value, const Dpt& type)
+bool GroupObject::valueNoSend(const KNXValue& value, const Dpt& type)
 {
-    if (_commFlagEx.uninitialized)
+    const bool encodingDone = KNX_Encode_Value(value, _data, _dataLength, type);
+
+    // initialize on succesful conversion only
+    if (encodingDone && _commFlagEx.uninitialized)
         commFlag(Ok);
 
-    KNX_Encode_Value(value, _data, _dataLength, type);
+    return encodingDone;
 }
 
 bool GroupObject::valueNoSendCompare(const KNXValue& value, const Dpt& type)
 {
     if (_commFlagEx.uninitialized)
     {
         // always set first value
-        this->valueNoSend(value, type);
-        return true;
+        return valueNoSend(value, type);
     }
     else
     {
-        // convert new value to given dtp
+        // convert new value to given DPT
         uint8_t newData[_dataLength];
         memset(newData, 0, _dataLength);
-        KNX_Encode_Value(value, newData, _dataLength, type);
+        const bool encodingDone = KNX_Encode_Value(value, newData, _dataLength, type);
+        if (!encodingDone)
+        {
+            // value conversion to DPT failed
+            // do NOT update the value of the KO!
+            return false;
+        }
 
         // check for change in converted value / update value on change only
         const bool dataChanged = memcmp(_data, newData, _dataLength);
@@ -315,8 +328,8 @@ bool GroupObject::valueCompare(const KNXValue& value, const Dpt& type)
 {
     if (valueNoSendCompare(value, type))
     {
-         objectWritten();
-         return true;
+        objectWritten();
+        return true;
     }
     return false;
 }
diff --git a/src/knx/group_object.h b/src/knx/group_object.h
@@ -172,8 +172,10 @@ class GroupObject
      * @param type the datapoint type used for the conversion.
      * 
      * The parameters must fit the group object. Otherwise it will stay unchanged.
+     * 
+     * @returns true if the value was converted successfully to the datapoint type and the group object was updated.
      */
-    void value(const KNXValue& value, const Dpt& type);
+    bool value(const KNXValue& value, const Dpt& type);
 
     /**
      * Check if the value (after conversion to dpt) will differ from current value of the group object and changes the state of the group object to ::WriteRequest if different.
@@ -183,18 +185,20 @@ class GroupObject
      * 
      * The parameters must fit the group object. Otherwise it will stay unchanged.
      * 
-     * @returns true if the value of the group object has changed
+     * @returns true if the value of the group object has changed, false if conversion results in same value as stored in group object or failed.
      */
     bool valueCompare(const KNXValue& value, const Dpt& type);
 
     /**
-     * set the current value of the group object.
+     * set the current value of the group object and show success.
      * @param value the value the group object is set to
      * @param type the datapoint type used for the conversion.
      * 
      * The parameters must fit the group object. Otherwise it will stay unchanged.
+     * 
+     * @returns true if value was converted successfully to the datapoint type and the group object was updated.
      */
-    void valueNoSend(const KNXValue& value, const Dpt& type);
+    bool valueNoSend(const KNXValue& value, const Dpt& type);
 
     /**
      * Check if the value (after conversion to dpt) will differ from current value of the group object and update if necessary.
@@ -204,7 +208,7 @@ class GroupObject
      * 
      * The parameters must fit the group object. Otherwise it will stay unchanged.
      * 
-     * @returns true if the value of the group object has changed
+     * @returns true if the value of the group object has changed, false if conversion results in same value as stored in group object or failed.
      */
     bool valueNoSendCompare(const KNXValue& value, const Dpt& type);
 
@@ -230,15 +234,19 @@ class GroupObject
      * @param value the value the group object is set to
      * 
      * The parameters must fit the group object and dhe datapoint type must be set with dataPointType(). Otherwise it will stay unchanged.
+     * 
+     * @returns true if the value was converted successfully to the datapoint type and the group object was updated.
      */
-    void value(const KNXValue& value);
+    bool value(const KNXValue& value);
     /**
      * set the current value of the group object.
      * @param value the value the group object is set to
      * 
-     * The parameters must fit the group object and dhe datapoint type must be set with dataPointType(). Otherwise it will stay unchanged.
+     * The parameters must fit the group object and the datapoint type must be set with dataPointType(). Otherwise it will stay unchanged.
+     * 
+     * @returns true if the value was converted successfully to the datapoint type and the group object was updated.
      */
-    void valueNoSend(const KNXValue& value);
+    bool valueNoSend(const KNXValue& value);
     /**
      * set the current value of the group object.
      * @param value the value the group object is set to