Implement the codegen-data-model Write support (#34139)

* Initial copy/merge of the codegendatamodel::write support * Restyle * Remove the error translation for ACL checks for attribute writes * Comment correction after special access error code guarantees were removed * Set the namespace for DataModel to resolve nameclash for android builds * Restyle * Some changes to make darwin builds happy * Do not clang-tidy on CodegenDataModel_Write * Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Use little endian encoding for pascal long strings, since this is what ember-strings uses * Restyle * Fix code to compile and pass tests * Code review comments * Comment update * Update based on code review feedback * Wrong condition. Fixed * Return invalid value to match ember-compatibility-functions * switch invalid data point to constraint error for return codes * Fix code review comments: comments and return unsupportedaccess * Remove useless comment - error check should be clear enough * Comment update * Re-arrange code for read only and timed * Re-format the read only checks a bit * Use CHIP_ERROR_NOT_FOUND * Separate out variable names * Slight updated code layout * Updated return value for chip error * Updated test to verifyordie instead of just logging errors * Update src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Update based on review feedback * Fix endianess and copying in test code * Restyle * Updated comment * Update src/app/codegen-data-model/tests/TestCodegenModelViaMocks.cpp Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> * Add unit test for "lowest signed value write" * Restyle * A constraint error update and better tests for AAI returning errors * One more test for invalid ember usage * Restyle * more tests for more coverage * Comment update * Fix comment * One more test for more coverage * Also cover writing non-null value to nullable attribute * Fix the ember string usage * Update src/app/codegen-data-model/CodegenDataModel_Read.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/codegen-data-model/CodegenDataModel_Read.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Remove duplicate code * Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Remove chip::app:: prefix in unit test since we have a top level using * Fix copy & paste encode to decode * Replace decoded with converted * Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Start using Failure for invalid data type instead of unsupported read. I do not expect this code path to actually be hit much * Fix comments * Updated encode/decode comment * Use failure instead of constraint error * Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Update src/app/codegen-data-model/CodegenDataModel_Write.cpp Co-authored-by: Boris Zbarsky <[email protected]> * Use dataversion mismatch for write without a version * Add extra IsGlobalAttribute check * use external writes for the ember write logic, so that we have extra size and validations * Updated comments * more comments * Restyle * Use emberAfWriteAttribute * Add comment about ember-string * Restyle * Add context to unit tests, make write do the marking of dirty paths * Add some unit tests for dirty path handling * Move the change callback around a bit * Restyle * Fixed unit tests to support size checks * Add unit test for invalid data * Restyle * Fix linter errors * Update to make size enforcement and guarantees clearer * use size_t for getlength sizes * Review comments and updated code to compile for android * make datamodel unambiguous * More fixes for clang compilation for DataModel scoping * Restyle * Try to make darwin compiler happy ... ssize_t vs size_t * Fix typo * Restyle * Code review updates * Undo submodule update --------- Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Tennessee Carmel-Veilleux <[email protected]> Co-authored-by: Boris Zbarsky <[email protected]>
project-chip · Jul 5, 2024 · fec6c7e · fec6c7e
1 parent e8bc7e7
commit fec6c7e
Show file tree

Hide file tree

Showing 18 changed files with 1,547 additions and 125 deletions.
diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml
@@ -204,11 +204,13 @@ jobs:
               run: |
                   ./scripts/run_in_build_env.sh "./scripts/run_codegen_targets.sh out/sanitizers"
             - name: Clang-tidy validation
+              # NOTE: clang-tidy crashes on CodegenDataModel_Write due to Nullable/std::optional check.
+              #       See https://github.com/llvm/llvm-project/issues/97426
               run: |
                   ./scripts/run_in_build_env.sh \
                     "./scripts/run-clang-tidy-on-compile-commands.py \
                        --compile-database out/sanitizers/compile_commands.json \
-                       --file-exclude-regex '/(repo|zzz_generated|lwip/standalone)/|-ReadImpl|-InvokeSubscribeImpl' \
+                       --file-exclude-regex '/(repo|zzz_generated|lwip/standalone)/|-ReadImpl|-InvokeSubscribeImpl|CodegenDataModel_Write' \
                        check \
                     "
             - name: Clean output
@@ -422,10 +424,13 @@ jobs:
               run: |
                   ./scripts/run_in_build_env.sh "./scripts/run_codegen_targets.sh out/default"
             - name: Clang-tidy validation
+              # NOTE: clang-tidy crashes on CodegenDataModel_Write due to Nullable/std::optional check.
+              #       See https://github.com/llvm/llvm-project/issues/97426
               run: |
                   ./scripts/run_in_build_env.sh \
                     "./scripts/run-clang-tidy-on-compile-commands.py \
                        --compile-database out/default/compile_commands.json \
+                       --file-exclude-regex '/(repo|zzz_generated|lwip/standalone)/|CodegenDataModel_Write' \
                        check \
                     "
             - name: Uploading diagnostic logs

diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
@@ -288,7 +288,17 @@ jobs:
                   type-safe setters
               if: always()
               run: |
-                  git grep -I -n 'emberAfWriteAttribute' -- './*' ':(exclude).github/workflows/lint.yml' ':(exclude)zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp' ':(exclude)src/app/zap-templates/templates/app/attributes/Accessors-src.zapt' ':(exclude)src/app/util/attribute-table.cpp' ':(exclude)examples/common/pigweed/rpc_services/Attributes.h' ':(exclude)src/app/util/attribute-table.h' ':(exclude)src/app/util/ember-compatibility-functions.cpp' && exit 1 || exit 0
+                  git grep -I -n 'emberAfWriteAttribute' -- './*'                                            \
+                      ':(exclude).github/workflows/lint.yml'                                                 \
+                      ':(exclude)examples/common/pigweed/rpc_services/Attributes.h'                          \
+                      ':(exclude)src/app/codegen-data-model/CodegenDataModel_Write.cpp'                      \
+                      ':(exclude)src/app/codegen-data-model/tests/EmberReadWriteOverride.cpp'                \
+                      ':(exclude)src/app/util/attribute-table.cpp'                                           \
+                      ':(exclude)src/app/util/attribute-table.h'                                             \
+                      ':(exclude)src/app/util/ember-compatibility-functions.cpp'                             \
+                      ':(exclude)src/app/zap-templates/templates/app/attributes/Accessors-src.zapt'          \
+                      ':(exclude)zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp' \
+                      && exit 1 || exit 0
 
             # Run ruff python linter
             - name: Check for errors using ruff Python linter

diff --git a/src/app/codegen-data-model/BUILD.gn b/src/app/codegen-data-model/BUILD.gn
@@ -20,8 +20,11 @@ import("//build_overrides/chip.gni")
 #
 # Use `model.gni` to get access to:
 #   CodegenDataModel.cpp
-#   CodegenDataModel_Read.cpp
 #   CodegenDataModel.h
+#   CodegenDataModel_Read.cpp
+#   CodegenDataModel_Write.cpp
+#   EmberMetadata.cpp
+#   EmberMetadata.h
 #
 # The above list of files exists to satisfy the "dependency linter"
 # since those files should technically be "visible to gn" even though we

diff --git a/src/app/codegen-data-model/CodegenDataModel.cpp b/src/app/codegen-data-model/CodegenDataModel.cpp
@@ -231,13 +231,6 @@ bool CodegenDataModel::EmberCommandListIterator::Exists(const CommandId * list,
     return (*mCurrentHint == toCheck);
 }
 
-CHIP_ERROR CodegenDataModel::WriteAttribute(const InteractionModel::WriteAttributeRequest & request,
-                                            AttributeValueDecoder & decoder)
-{
-    // TODO: this needs an implementation
-    return CHIP_ERROR_NOT_IMPLEMENTED;
-}
-
 CHIP_ERROR CodegenDataModel::Invoke(const InteractionModel::InvokeRequest & request, TLV::TLVReader & input_arguments,
                                     InteractionModel::InvokeReply & reply)
 {

diff --git a/src/app/codegen-data-model/CodegenDataModel_Read.cpp b/src/app/codegen-data-model/CodegenDataModel_Read.cpp
@@ -14,7 +14,6 @@
  *    See the License for the specific language governing permissions and
  *    limitations under the License.
  */
-#include "lib/core/CHIPError.h"
 #include <app/codegen-data-model/CodegenDataModel.h>
 
 #include <optional>
@@ -29,6 +28,7 @@
 #include <app/AttributeValueEncoder.h>
 #include <app/GlobalAttributes.h>
 #include <app/RequiredPrivilege.h>
+#include <app/codegen-data-model/EmberMetadata.h>
 #include <app/data-model/FabricScoped.h>
 #include <app/util/af-types.h>
 #include <app/util/attribute-metadata.h>
@@ -49,56 +49,6 @@ namespace app {
 namespace {
 using namespace chip::app::Compatibility::Internal;
 
-// Fetch the source for the given attribute path: either a cluster (for global ones) or attribute
-// path.
-//
-// if returning a CHIP_ERROR, it will NEVER be CHIP_NO_ERROR.
-std::variant<const EmberAfCluster *,           // global attribute, data from a cluster
-             const EmberAfAttributeMetadata *, // a specific attribute stored by ember
-             CHIP_ERROR                        // error, this will NEVER be CHIP_NO_ERROR
-             >
-FindAttributeMetadata(const ConcreteAttributePath & aPath)
-{
-    for (auto & attr : GlobalAttributesNotInMetadata)
-    {
-
-        if (attr == aPath.mAttributeId)
-        {
-            const EmberAfCluster * cluster = emberAfFindServerCluster(aPath.mEndpointId, aPath.mClusterId);
-            if (cluster == nullptr)
-            {
-                return (emberAfFindEndpointType(aPath.mEndpointId) == nullptr) ? CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)
-                                                                               : CHIP_IM_GLOBAL_STATUS(UnsupportedCluster);
-            }
-
-            return cluster;
-        }
-    }
-    const EmberAfAttributeMetadata * metadata =
-        emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId);
-
-    if (metadata == nullptr)
-    {
-        const EmberAfEndpointType * type = emberAfFindEndpointType(aPath.mEndpointId);
-        if (type == nullptr)
-        {
-            return CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint);
-        }
-
-        const EmberAfCluster * cluster = emberAfFindClusterInType(type, aPath.mClusterId, CLUSTER_MASK_SERVER);
-        if (cluster == nullptr)
-        {
-            return CHIP_IM_GLOBAL_STATUS(UnsupportedCluster);
-        }
-
-        // Since we know the attribute is unsupported and the endpoint/cluster are
-        // OK, this is the only option left.
-        return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);
-    }
-
-    return metadata;
-}
-
 /// Attempts to read via an attribute access interface (AAI)
 ///
 /// If it returns a CHIP_ERROR, then this is a FINAL result (i.e. either failure or success).
@@ -138,13 +88,30 @@ struct ShortPascalString
 {
     using LengthType                        = uint8_t;
     static constexpr LengthType kNullLength = 0xFF;
+
+    static size_t GetLength(ByteSpan buffer)
+    {
+        VerifyOrDie(buffer.size() >= 1);
+        // NOTE: we do NOT use emberAfStringLength from ember-strings.h because that will result in 0
+        //       length for null sizes (i.e. 0xFF is translated to 0 and we do not want that here)
+        return buffer[0];
+    }
 };
 
 /// Metadata of what a ember/pascal LONG string means (prepended by a u16 length)
 struct LongPascalString
 {
     using LengthType                        = uint16_t;
     static constexpr LengthType kNullLength = 0xFFFF;
+
+    static size_t GetLength(ByteSpan buffer)
+    {
+        // NOTE: we do NOT use emberAfLongStringLength from ember-strings.h because that will result in 0
+        //       length for null sizes (i.e. 0xFFFF is translated to 0 and we do not want that here)
+        VerifyOrDie(buffer.size() >= 2);
+        const uint8_t * data = buffer.data();
+        return Encoding::LittleEndian::Read16(data);
+    }
 };
 
 // ember assumptions ... should just work
@@ -157,20 +124,17 @@ static_assert(sizeof(LongPascalString::LengthType) == 2);
 template <class OUT, class ENCODING>
 std::optional<OUT> ExtractEmberString(ByteSpan data)
 {
-    typename ENCODING::LengthType len;
-
-    // Ember storage format for pascal-prefix data is specifically "native byte order",
-    // hence the use of memcpy.
-    VerifyOrDie(sizeof(len) <= data.size());
-    memcpy(&len, data.data(), sizeof(len));
+    constexpr size_t kLengthTypeSize = sizeof(typename ENCODING::LengthType);
+    VerifyOrDie(kLengthTypeSize <= data.size());
+    auto len = ENCODING::GetLength(data);
 
     if (len == ENCODING::kNullLength)
     {
         return std::nullopt;
     }
 
-    VerifyOrDie(static_cast<size_t>(len + sizeof(len)) <= data.size());
-    return std::make_optional<OUT>(reinterpret_cast<typename OUT::pointer>(data.data() + sizeof(len)), len);
+    VerifyOrDie(len + sizeof(len) <= data.size());
+    return std::make_optional<OUT>(reinterpret_cast<typename OUT::pointer>(data.data() + kLengthTypeSize), len);
 }
 
 /// Encode a value inside `encoder`
@@ -282,7 +246,7 @@ CHIP_ERROR EncodeEmberValue(ByteSpan data, const EmberAfAttributeMetadata * meta
         return EncodeStringLike<ByteSpan, LongPascalString>(data, isNullable, encoder);
     default:
         ChipLogError(DataManagement, "Attribute type 0x%x not handled", static_cast<int>(metadata->attributeType));
-        return CHIP_IM_GLOBAL_STATUS(UnsupportedRead);
+        return CHIP_IM_GLOBAL_STATUS(Failure);
     }
 }
 
@@ -311,21 +275,26 @@ CHIP_ERROR CodegenDataModel::ReadAttribute(const InteractionModel::ReadAttribute
                                                           RequiredPrivilege::ForReadAttribute(request.path));
         if (err != CHIP_NO_ERROR)
         {
+            ReturnErrorCodeIf(err != CHIP_ERROR_ACCESS_DENIED, err);
+
             // Implementation of 8.4.3.2 of the spec for path expansion
-            if (request.path.mExpanded && (err == CHIP_ERROR_ACCESS_DENIED))
+            if (request.path.mExpanded)
             {
                 return CHIP_NO_ERROR;
             }
-            return err;
+            // access denied has a specific code for IM
+            return CHIP_IM_GLOBAL_STATUS(UnsupportedAccess);
         }
     }
 
-    auto metadata = FindAttributeMetadata(request.path);
+    auto metadata = Ember::FindAttributeMetadata(request.path);
 
     // Explicit failure in finding a suitable metadata
     if (const CHIP_ERROR * err = std::get_if<CHIP_ERROR>(&metadata))
     {
-        VerifyOrDie(*err != CHIP_NO_ERROR);
+        VerifyOrDie((*err == CHIP_IM_GLOBAL_STATUS(UnsupportedEndpoint)) || //
+                    (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedCluster)) ||  //
+                    (*err == CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute)));
         return *err;
     }