Merge faac4fb into e26859c

.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

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

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)
 {

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,27 @@ struct ShortPascalString
 {
     using LengthType                        = uint8_t;
     static constexpr LengthType kNullLength = 0xFF;
+
+    static LengthType GetLength(const uint8_t * buffer)
+    {
+        // NOTE: we do NOT use emberAfLongStringLength because that will result in 0 length
+        //       for null strings
+        return *buffer;
+    }
 };
 
 /// 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 LengthType GetLength(const uint8_t * buffer)
+    {
+        // NOTE: we do NOT use emberAfLongStringLength because that will result in 0 length
+        //       for null strings
+        return Encoding::LittleEndian::Read16(buffer);
+    }
 };
 
 // ember assumptions ... should just work
@@ -157,12 +121,8 @@ 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));
+    VerifyOrDie(sizeof(typename ENCODING::LengthType) <= data.size());
+    auto len = ENCODING::GetLength(data.data());
 
     if (len == ENCODING::kNullLength)
     {
@@ -282,7 +242,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 +271,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;
     }