Optimize codesize so the cost of passing the MarkAttributeDirty argument is paid only by those who neeed it.

Author: bzbarsky-apple

src/app/util/af.h

@@ -96,10 +96,20 @@ bool emberAfContainsClient(chip::EndpointId endpoint, chip::ClusterId clusterId)
  * data type (as Accessors.h/cpp have this correct by default).
  * TODO: this not checking seems off - what if this is run without Accessors.h ?
  */
-chip::Protocols::InteractionModel::Status
-emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * dataPtr,
-                      EmberAfAttributeType dataType,
-                      chip::app::MarkAttributeDirty markDirty = chip::app::MarkAttributeDirty::kIfChanged);
+chip::Protocols::InteractionModel::Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster,
+                                                                chip::AttributeId attributeID, uint8_t * dataPtr,
+                                                                EmberAfAttributeType dataType);
+
+/**
+ * A version of emberAfWriteAttribute that allows controlling when the attribute
+ * should be marked dirty.  This is an overload, not an optional argument, to
+ * reduce codesize at all the callsites that want to write without doing
+ * anything special to control the dirty marking.
+ */
+chip::Protocols::InteractionModel::Status emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster,
+                                                                chip::AttributeId attributeID, uint8_t * dataPtr,
+                                                                EmberAfAttributeType dataType,
+                                                                chip::app::MarkAttributeDirty markDirty);
 
 /**
  * @brief Read the attribute value, performing all the checks.

src/app/util/attribute-table.cpp

@@ -31,10 +31,53 @@ using chip::Protocols::InteractionModel::Status;
 using namespace chip;
 using namespace chip::app;
 
+namespace {
+/**
+ * @brief write an attribute, performing all the checks.
+ *
+ * This function will attempt to write the attribute value from
+ * the provided pointer. This function will only check that the
+ * attribute exists. If it does it will write the value into
+ * the attribute table for the given attribute.
+ *
+ * This function will not check to see if the attribute is
+ * writable since the read only / writable characteristic
+ * of an attribute only pertains to external devices writing
+ * over the air. Because this function is being called locally
+ * it assumes that the device knows what it is doing and has permission
+ * to perform the given operation.
+ *
+ * if true is passed in for overrideReadOnlyAndDataType then the data type is
+ * not checked and the read-only flag is ignored. This mode is meant for
+ * testing or setting the initial value of the attribute on the device.
+ *
+ * this returns:
+ * - Status::UnsupportedEndpoint: if endpoint isn't supported by the device.
+ * - Status::UnsupportedCluster: if cluster isn't supported on the endpoint.
+ * - Status::UnsupportedAttribute: if attribute isn't supported in the cluster.
+ * - Status::InvalidDataType: if the data type passed in doesnt match the type
+ *           stored in the attribute table
+ * - Status::UnsupportedWrite: if the attribute isnt writable
+ * - Status::ConstraintError: if the value is set out of the allowable range for
+ *           the attribute
+ * - Status::Success: if the attribute was found and successfully written
+ */
+Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * data,
+                          EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, MarkAttributeDirty markDirty);
+} // anonymous namespace
+
 Status emAfWriteAttributeExternal(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
                                   EmberAfAttributeType dataType)
 {
-    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, false /* override read-only */);
+    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, false /* override read-only */,
+                              MarkAttributeDirty::kIfChanged);
+}
+
+Status emberAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
+                             EmberAfAttributeType dataType)
+{
+    return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */,
+                              MarkAttributeDirty::kIfChanged);
 }
 
 Status emberAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
@@ -149,8 +192,6 @@ Status AttributeValueIsChanging(EndpointId endpoint, ClusterId cluster, Attribut
     return Status::Success;
 }
 
-} // anonymous namespace
-
 Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * data,
                           EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, MarkAttributeDirty markDirty)
 {
@@ -301,6 +342,8 @@ Status emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId at
     return Status::Success;
 }
 
+} // anonymous namespace
+
 Status emberAfReadAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr, uint16_t readLength)
 {
     const EmberAfAttributeMetadata * metadata = nullptr;

src/app/util/attribute-table.h

@@ -30,38 +30,3 @@
 chip::Protocols::InteractionModel::Status emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster,
                                                                      chip::AttributeId attributeID, uint8_t * dataPtr,
                                                                      EmberAfAttributeType dataType);
-
-/**
- * @brief write an attribute, performing all the checks.
- *
- * This function will attempt to write the attribute value from
- * the provided pointer. This function will only check that the
- * attribute exists. If it does it will write the value into
- * the attribute table for the given attribute.
- *
- * This function will not check to see if the attribute is
- * writable since the read only / writable characteristic
- * of an attribute only pertains to external devices writing
- * over the air. Because this function is being called locally
- * it assumes that the device knows what it is doing and has permission
- * to perform the given operation.
- *
- * if true is passed in for overrideReadOnlyAndDataType then the data type is
- * not checked and the read-only flag is ignored. This mode is meant for
- * testing or setting the initial value of the attribute on the device.
- *
- * this returns:
- * - Status::UnsupportedEndpoint: if endpoint isn't supported by the device.
- * - Status::UnsupportedCluster: if cluster isn't supported on the endpoint.
- * - Status::UnsupportedAttribute: if attribute isn't supported in the cluster.
- * - Status::InvalidDataType: if the data type passed in doesnt match the type
- *           stored in the attribute table
- * - Status::UnsupportedWrite: if the attribute isnt writable
- * - Status::ConstraintError: if the value is set out of the allowable range for
- *           the attribute
- * - Status::Success: if the attribute was found and successfully written
- */
-chip::Protocols::InteractionModel::Status
-emAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * data,
-                   EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType,
-                   chip::app::MarkAttributeDirty markDirty = chip::app::MarkAttributeDirty::kIfChanged);

src/app/zap-templates/templates/app/attributes/Accessors-src.zapt

@@ -86,8 +86,8 @@ Protocols::InteractionModel::Status Get(chip::EndpointId endpoint, {{accessorGet
     {{/if}}
 }
 
-Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name forceNotNullable=true forceNotOptional=true}} value, MarkAttributeDirty markDirty)
-{
+{{! Has a passMarkDirty boolean argument that controls which overload of emberAfWriteAttribute we call }}
+{{#*inline "setBody"}}
     {{~#if (isString type)}}
     {{~#*inline "lengthType"}}uint{{#if (isShortString type)}}8{{else}}16{{/if}}_t{{/inline}}
     static_assert({{maxLength}} < NumericAttributeTraits<{{>lengthType}}>::kNullValue,
@@ -101,7 +101,7 @@ Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEn
       Encoding::LittleEndian::Put16(zclString, length);
     {{/if}}
     memcpy(&zclString[{{>sizingBytes}}], value.data(), value.size());
-    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE, markDirty);
+    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE{{#if passMarkDirty}}, markDirty{{/if}});
     {{else}}
     using Traits = NumericAttributeTraits<{{accessorTraitType type}}>;
     if (!Traits::CanRepresentValue(/* isNullable = */ {{isNullable}}, value))
@@ -111,33 +111,43 @@ Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEn
     Traits::StorageType storageValue;
     Traits::WorkingToStorage(value, storageValue);
     uint8_t * writable = Traits::ToAttributeStoreRepresentation(storageValue);
-    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE, markDirty);
+    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE{{#if passMarkDirty}}, markDirty{{/if}});
     {{/if}}
+{{/inline}}
+
+Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name forceNotNullable=true forceNotOptional=true}} value, MarkAttributeDirty markDirty)
+{
+    {{> setBody passMarkDirty=true}}
 }
 
 Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name forceNotNullable=true forceNotOptional=true}} value)
 {
-    return Set(endpoint, value, MarkAttributeDirty::kIfChanged);
+    {{> setBody passMarkDirty=false}}
 }
 
 {{#if isNullable}}
-Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint, MarkAttributeDirty markDirty)
-{
+{{! Has a passMarkDirty boolean argument that controls which overload of emberAfWriteAttribute we call }}
+{{#*inline "setNullBody"}}
     {{#if (isString type)}}
     uint8_t zclString[{{>sizingBytes}}] = { {{#if (isShortString type)}}0xFF{{else}}0xFF, 0xFF{{/if}} };
-    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE, markDirty);
+    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE{{#if passMarkDirty}}, markDirty{{/if}});
     {{else}}
     using Traits = NumericAttributeTraits<{{accessorTraitType type}}>;
     Traits::StorageType value;
     Traits::SetNull(value);
     uint8_t * writable = Traits::ToAttributeStoreRepresentation(value);
-    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE, markDirty);
+    return emberAfWriteAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE{{#if passMarkDirty}}, markDirty{{/if}});
     {{/if}}
+{{/inline}}
+
+Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint, MarkAttributeDirty markDirty)
+{
+    {{> setNullBody passMarkDirty=true}}
 }
 
 Protocols::InteractionModel::Status SetNull(chip::EndpointId endpoint)
 {
-    return SetNull(endpoint, MarkAttributeDirty::kIfChanged);
+    {{> setNullBody passMarkDirty=false}}
 }
 
 Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name isArgument=true forceNotOptional=true}} value, MarkAttributeDirty markDirty)
@@ -151,7 +161,11 @@ Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEn
 
 Protocols::InteractionModel::Status Set(chip::EndpointId endpoint, {{zapTypeToEncodableClusterObjectType type ns=parent.name isArgument=true forceNotOptional=true}} value)
 {
-    return Set(endpoint, value, MarkAttributeDirty::kIfChanged);
+  if (value.IsNull()) {
+    return SetNull(endpoint);
+  }
+
+  return Set(endpoint, value.Value());
 }
 {{/if}}