From 2cd5a28d8eba3f29f16b577f24449d03e34cf077 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Cornelius=20K=C3=B6pp?= <121643070+cornelius-koepp@users.noreply.github.com> Date: Mon, 18 Nov 2024 20:29:02 +0100 Subject: [PATCH] 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 --- README.md | 5 +++++ src/knx/group_object.cpp | 45 ++++++++++++++++++++++++++-------------- src/knx/group_object.h | 24 ++++++++++++++------- 3 files changed, 50 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index ff042114..2acdb3ac 100644 --- 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 index 7b172d5c..ece6d706 100644 --- 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,18 +278,21 @@ 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) @@ -292,15 +300,20 @@ 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; } \ No newline at end of file diff --git a/src/knx/group_object.h b/src/knx/group_object.h index 344717ed..838b692c 100644 --- 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