Skip to content

Commit

Permalink
Enable more attributes in TestCluster. (#12355)
Browse files Browse the repository at this point in the history
This enables attributes of the following types:

* Signed and unsigned 24, 40, 48, 56-bit integers.
* float and double.
* A named enum.
* A struct.
* 8-bit and 16-bit signed and unsigned integers with min/max values set.  Other
  sizes don't have min/max support yet.
* Nullable versions of all of the above.

This should hopefully let us start trying to write yaml tests for
these things so we can figure out which parts work and which parts
don't work.

Other changes needed to make the above successfully go through codegen
and generate code that compiles:

* Add type mappings for float/double for java.
* Add min/max definitions (set to -Infinity and Infinity) for
  float/double in chip-tool.
* Remove the broken StructHelper.js file, which was not correctly
  identifying structs, and replace its use with "if_is_struct" in the
  one template that uses this codepath.
* Fix checking for lists in asChipCallback to actually work properly.
  Most code that dealt with lists checked for isList explicitly and
  never actually used the output of asChipCallback, but we're using it
  in more places now.
* Add checks for the "Unsupported" chipCallback type in various
  templates so we can add attributes of types not supported by that
  machinery (doubles, floats, structs, named enums) without codegen
  completely failing.

Some things that are known not to work:

1) A bunch of clients (command-line chip-tool, java, "old-style"
   python, darwin) rely on having typed read/report callback functions
   for all attribute types they support, and we don't have those yet
   for float, double, named enums, and structs.  Will need followups
   to make those work.  For now the relevant codepaths are effectively
   disabled by conditioning on chipCallback.name being "Unsupported",
   so we can try to address these types one at a time as desired.

2) The ember-compatibility-functions code for reading/writing
   attributes does not support float and double.  Those bits would
   need to be added to test the float/double attributes in a
   meaningful way.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Jun 5, 2023
1 parent 0f21a3f commit 998a0bf
Show file tree
Hide file tree
Showing 52 changed files with 18,448 additions and 3,132 deletions.
486 changes: 483 additions & 3 deletions examples/all-clusters-app/all-clusters-common/all-clusters-app.zap

Large diffs are not rendered by default.

12 changes: 11 additions & 1 deletion examples/chip-tool/templates/commands.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,8 @@ private:
{{/chip_cluster_commands}}

{{#chip_server_cluster_attributes}}
{{! TODO: Various types (floats, structs) not supported here. }}
{{#unless (isStrEqual chipCallback.name "Unsupported")}}
/*
* Attribute {{asUpperCamelCase name}}
*/
Expand Down Expand Up @@ -475,7 +477,11 @@ public:
Write{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}(): ModelCommand("write")
{
AddArgument("attr-name", "{{asDelimitedCommand (asUpperCamelCase name)}}");
{{#if (isString type)}}
{{#if isArray}}
// {{label}} Array parsing is not supported yet
{{else if isStruct}}
// {{label}} Struct parsing is not supported yet
{{else if (isString type)}}
AddArgument("attr-value", &mValue);
{{else}}
AddArgument("attr-value", {{asTypeMinValue type}}, {{asTypeMaxValue type}}, &mValue);
Expand Down Expand Up @@ -558,6 +564,7 @@ private:

{{/unless}}
{{/if}}
{{/unless}}
{{/chip_server_cluster_attributes}}
{{/chip_client_clusters}}

Expand All @@ -574,6 +581,8 @@ void registerCluster{{asUpperCamelCase name}}(Commands & commands)
make_unique<{{asUpperCamelCase clusterName}}{{asUpperCamelCase name}}>(), //
{{/chip_cluster_commands}}
{{#chip_server_cluster_attributes}}
{{! TODO: Various types (floats, structs) not supported here. }}
{{#unless (isStrEqual chipCallback.name "Unsupported")}}
make_unique<Read{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}>(), //
{{#if isWritableAttribute}}
{{! No list support for writing yet. Need to figure out how to
Expand All @@ -587,6 +596,7 @@ void registerCluster{{asUpperCamelCase name}}(Commands & commands)
make_unique<Report{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}>(), //
{{/unless}}
{{/if}}
{{/unless}}
{{/chip_server_cluster_attributes}}
};

Expand Down
6 changes: 6 additions & 0 deletions examples/chip-tool/templates/helper.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ function asTypeMinValue(type)
case 'uint32_t':
case 'uint64_t':
return '0';
case 'float':
case 'double':
return `-std::numeric_limits<${basicType}>::infinity`;
default:
error = 'asTypeMinValue: Unhandled underlying type ' + zclType + ' for original type ' + type;
throw error;
Expand Down Expand Up @@ -82,6 +85,9 @@ function asTypeMaxValue(type)
case 'uint32_t':
case 'uint64_t':
return 'UINT' + parseInt(basicType.slice(4)) + '_MAX';
case 'float':
case 'double':
return `std::numeric_limits<${basicType}>::infinity`;
default:
return 'err';
error = 'asTypeMaxValue: Unhandled underlying type ' + zclType + ' for original type ' + type;
Expand Down
6 changes: 6 additions & 0 deletions examples/chip-tool/templates/reporting-commands.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@ public:
{{#chip_server_cluster_attributes}}
{{#if isReportableAttribute}}
{{#unless isList}}
{{#unless (isStrEqual chipCallback.name "Unsupported")}}
delete onReport{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}Callback;
{{/unless}}
{{/unless}}
{{/if}}
{{/chip_server_cluster_attributes}}
{{/chip_client_clusters}}
Expand All @@ -34,8 +36,10 @@ public:
{{#chip_server_cluster_attributes}}
{{#if isReportableAttribute}}
{{#unless isList}}
{{#unless (isStrEqual chipCallback.name "Unsupported")}}
callbacksMgr.AddReportCallback(remoteId, endpointId, {{asHex parent.code 4}}, {{asHex code 4}}, onReport{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}Callback->Cancel(), BasicAttributeFilter<{{chipCallback.name}}AttributeCallback>);
{{/unless}}
{{/unless}}
{{/if}}
{{/chip_server_cluster_attributes}}
{{/chip_client_clusters}}
Expand Down Expand Up @@ -81,8 +85,10 @@ private:
{{#chip_server_cluster_attributes}}
{{#if isReportableAttribute}}
{{#unless isList}}
{{#unless (isStrEqual chipCallback.name "Unsupported")}}
chip::Callback::Callback<{{chipCallback.name}}AttributeCallback> * onReport{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}Callback = new chip::Callback::Callback<{{chipCallback.name}}AttributeCallback>(On{{chipCallback.name}}AttributeResponse, this);
{{/unless}}
{{/unless}}
{{/if}}
{{/chip_server_cluster_attributes}}
{{/chip_client_clusters}}
Expand Down
22 changes: 22 additions & 0 deletions src/app/clusters/test-cluster-server/test-cluster-server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,18 @@ class TestAttrAccess : public AttributeAccessInterface
CHIP_ERROR WriteListStructOctetStringAttribute(AttributeValueDecoder & aDecoder);
CHIP_ERROR ReadListNullablesAndOptionalsStructAttribute(AttributeValueEncoder & aEncoder);
CHIP_ERROR WriteListNullablesAndOptionalsStructAttribute(AttributeValueDecoder & aDecoder);
CHIP_ERROR ReadStructAttribute(AttributeValueEncoder & aEncoder);
CHIP_ERROR WriteStructAttribute(AttributeValueDecoder & aDecoder);
};

TestAttrAccess gAttrAccess;
uint8_t gListUint8Data[kAttributeListLength];
OctetStringData gListOctetStringData[kAttributeListLength];
OctetStringData gListOperationalCert[kAttributeListLength];
Structs::TestListStructOctet::Type listStructOctetStringData[kAttributeListLength];
Structs::SimpleStruct::Type gStructAttributeValue = { 0, false, SimpleEnum::EMBER_ZCL_SIMPLE_ENUM_VALUE_A,
ByteSpan(), CharSpan(), BitFlags<SimpleBitmap>(),
0, 0 };

CHIP_ERROR TestAttrAccess::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
{
Expand All @@ -102,6 +107,9 @@ CHIP_ERROR TestAttrAccess::Read(const ConcreteReadAttributePath & aPath, Attribu
case ListNullablesAndOptionalsStruct::Id: {
return ReadListNullablesAndOptionalsStructAttribute(aEncoder);
}
case Struct::Id: {
return ReadStructAttribute(aEncoder);
}
default: {
break;
}
Expand All @@ -126,6 +134,9 @@ CHIP_ERROR TestAttrAccess::Write(const ConcreteDataAttributePath & aPath, Attrib
case ListNullablesAndOptionalsStruct::Id: {
return WriteListNullablesAndOptionalsStructAttribute(aDecoder);
}
case Struct::Id: {
return WriteStructAttribute(aDecoder);
}
default: {
break;
}
Expand Down Expand Up @@ -260,6 +271,17 @@ CHIP_ERROR TestAttrAccess::WriteListNullablesAndOptionalsStructAttribute(Attribu
// TODO Add yaml test case for NullablesAndOptionalsStruct list
return CHIP_NO_ERROR;
}

CHIP_ERROR TestAttrAccess::ReadStructAttribute(AttributeValueEncoder & aEncoder)
{
return aEncoder.Encode(gStructAttributeValue);
}

CHIP_ERROR TestAttrAccess::WriteStructAttribute(AttributeValueDecoder & aDecoder)
{
return aDecoder.Decode(gStructAttributeValue);
}

} // namespace

bool emberAfTestClusterClusterTestCallback(app::CommandHandler *, const app::ConcreteCommandPath & commandPath,
Expand Down
10 changes: 8 additions & 2 deletions src/app/zap-templates/common/ClustersHelper.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ const zclQuery = require(zapPath + 'db/query-zcl.js')
const { Deferred } = require('./Deferred.js');
const ListHelper = require('./ListHelper.js');
const StringHelper = require('./StringHelper.js');
const StructHelper = require('./StructHelper.js');
const ChipTypesHelper = require('./ChipTypesHelper.js');

//
Expand Down Expand Up @@ -161,10 +160,16 @@ function asChipCallback(item)
return { name : 'CharString', type : 'const chip::CharSpan' };
}

if (ListHelper.isList(item.type)) {
if (item.isList) {
return { name : 'List', type : null };
}

if (item.isEnum) {
// Unsupported or now, until we figure out what to do for callbacks for
// strongly typed enums.
return { name : 'Unsupported', type : null };
}

const basicType = ChipTypesHelper.asBasicType(item.chipType);
switch (basicType) {
case 'int8_t':
Expand All @@ -179,6 +184,7 @@ function asChipCallback(item)
return { name : 'Int' + basicType.replace(/[^0-9]/g, '') + 'u', type : basicType };
case 'bool':
return { name : 'Boolean', type : 'bool' };
// TODO: Add float and double
default:
return { name : 'Unsupported', type : null };
}
Expand Down
28 changes: 0 additions & 28 deletions src/app/zap-templates/common/StructHelper.js

This file was deleted.

19 changes: 14 additions & 5 deletions src/app/zap-templates/common/attributes/Accessors.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
const zapPath = '../../../../../third_party/zap/repo/dist/src-electron/';
const ListHelper = require('../../common/ListHelper.js');
const StringHelper = require('../../common/StringHelper.js');
const StructHelper = require('../../common/StructHelper.js');
const cHelper = require(zapPath + 'generator/helper-c.js')
const zclHelper = require(zapPath + 'generator/helper-zcl.js')
const templateUtil = require(zapPath + 'generator/template-util.js')
const zclUtil = require(zapPath + 'util/zcl-util.js')

// Not sure what to do with EUI64 yet.
const unsupportedTypes = [ 'EUI64' ];
Expand All @@ -38,10 +40,8 @@ function canHaveSimpleAccessors(attr)
return false;
}

if (StructHelper.isStruct(attr.type)) {
return false;
}

// We can't check for being a struct synchronously, so that's handled manually
// in the template.
if (isUnsupportedType(attr.type)) {
return false;
}
Expand Down Expand Up @@ -87,9 +87,18 @@ async function accessorTraitType(type)
return zclHelper.asUnderlyingZclType.call(this, type, { 'hash' : {} });
}

async function typeAsDelimitedMacro(type)
{
const { db } = this.global;
const pkgId = await templateUtil.ensureZclPackageId(this);
const typeInfo = await zclUtil.determineType(db, type, pkgId);
return cHelper.asDelimitedMacro.call(this, typeInfo.atomicType);
}

//
// Module exports
//
exports.canHaveSimpleAccessors = canHaveSimpleAccessors;
exports.accessorGetterType = accessorGetterType;
exports.accessorTraitType = accessorTraitType;
exports.typeAsDelimitedMacro = typeAsDelimitedMacro;
2 changes: 2 additions & 0 deletions src/app/zap-templates/templates/app/CHIPClusters-src.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ exit:
{{/chip_cluster_commands}}
// {{asUpperCamelCase name}} Cluster Attributes
{{#chip_server_cluster_attributes}}
{{#unless (isStrEqual chipCallback.name "Unsupported")}}
CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::ReadAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback)
{
app::AttributePathParams attributePath;
Expand Down Expand Up @@ -115,6 +116,7 @@ CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::ReportAttribute{{asUpperCame

{{/unless}}
{{/if}}
{{/unless}}
{{/chip_server_cluster_attributes}}

{{/chip_client_clusters}}
Expand Down
6 changes: 2 additions & 4 deletions src/app/zap-templates/templates/app/CHIPClusters.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -30,22 +30,20 @@ public:

// Cluster Attributes
{{#chip_server_cluster_attributes}}
{{#unless (isStrEqual chipCallback.name "Unsupported")}}
CHIP_ERROR ReadAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback);
{{/chip_server_cluster_attributes}}
{{#chip_server_cluster_attributes}}
{{#if isWritableAttribute}}
{{#unless isList}}
CHIP_ERROR WriteAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, {{chipType}} value);
{{/unless}}
{{/if}}
{{/chip_server_cluster_attributes}}
{{#chip_server_cluster_attributes}}
{{#if isReportableAttribute}}
{{#unless isList}}
CHIP_ERROR SubscribeAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, uint16_t minInterval, uint16_t maxInterval);
CHIP_ERROR ReportAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onReportCallback);
{{/unless}}
{{/if}}
{{/unless}}
{{/chip_server_cluster_attributes}}
{{#chip_cluster_commands}}
{{#first}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ namespace Attributes {

{{/first}}
{{#if clusterRef}}
{{#if (canHaveSimpleAccessors this)}}
{{#if_is_struct type}}
{{else if (canHaveSimpleAccessors this)}}
namespace {{asUpperCamelCase label}} {

{{#*inline "clusterId"}}Clusters::{{asUpperCamelCase parent.label}}::Id{{/inline}}
Expand Down Expand Up @@ -92,7 +93,7 @@ EmberAfStatus Set(chip::EndpointId endpoint, {{asUnderlyingZclType type}} value)
uint8_t zclString[{{maxLength}} + {{>sizingBytes}}];
emberAfCopyInt{{#if (isShortString type)}}8{{else}}16{{/if}}u(zclString, 0, static_cast<{{>lengthType}}>(value.size()));
memcpy(&zclString[{{>sizingBytes}}], value.data(), value.size());
return emberAfWriteServerAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{asDelimitedMacro type}}_ATTRIBUTE_TYPE);
return emberAfWriteServerAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE);
{{else}}
if (!NumericAttributeTraits<{{accessorTraitType type}}>::CanRepresentValue(/* isNullable = */ {{isNullable}}, value))
{
Expand All @@ -101,7 +102,7 @@ EmberAfStatus Set(chip::EndpointId endpoint, {{asUnderlyingZclType type}} value)
NumericAttributeTraits<{{accessorTraitType type}}>::StorageType storageValue;
NumericAttributeTraits<{{accessorTraitType type}}>::WorkingToStorage(value, storageValue);
uint8_t * writable = NumericAttributeTraits<{{accessorTraitType type}}>::ToAttributeStoreRepresentation(storageValue);
return emberAfWriteServerAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{asDelimitedMacro type}}_ATTRIBUTE_TYPE);
return emberAfWriteServerAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE);
{{/if}}
}

Expand All @@ -110,12 +111,12 @@ EmberAfStatus SetNull(chip::EndpointId endpoint)
{
{{#if (isString type)}}
uint8_t zclString[{{>sizingBytes}}] = { {{#if (isShortString type)}}0xFF{{else}}0xFF, 0xFF{{/if}} };
return emberAfWriteServerAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{asDelimitedMacro type}}_ATTRIBUTE_TYPE);
return emberAfWriteServerAttribute(endpoint, {{>clusterId}}, Id, zclString, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE);
{{else}}
NumericAttributeTraits<{{accessorTraitType type}}>::StorageType value;
NumericAttributeTraits<{{accessorTraitType type}}>::SetNull(value);
uint8_t * writable = NumericAttributeTraits<{{accessorTraitType type}}>::ToAttributeStoreRepresentation(value);
return emberAfWriteServerAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{asDelimitedMacro type}}_ATTRIBUTE_TYPE);
return emberAfWriteServerAttribute(endpoint, {{>clusterId}}, Id, writable, ZCL_{{typeAsDelimitedMacro type}}_ATTRIBUTE_TYPE);
{{/if}}
}

Expand All @@ -131,7 +132,7 @@ EmberAfStatus Set(chip::EndpointId endpoint, const DataModel::Nullable<{{asUnder

} // namespace {{asUpperCamelCase label}}

{{/if}}
{{/if_is_struct}}
{{/if}}
{{#last}}
} // namespace Attributes
Expand Down
5 changes: 3 additions & 2 deletions src/app/zap-templates/templates/app/attributes/Accessors.zapt
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ namespace Attributes {

{{/first}}
{{#if clusterRef}}
{{#if (canHaveSimpleAccessors this)}}
{{#if_is_struct type}}
{{else if (canHaveSimpleAccessors this)}}
namespace {{asUpperCamelCase label}} {
EmberAfStatus Get(chip::EndpointId endpoint, {{accessorGetterType this}} value); // {{type}} {{isArray}}
EmberAfStatus Set(chip::EndpointId endpoint, {{asUnderlyingZclType type}} value);
Expand All @@ -34,7 +35,7 @@ EmberAfStatus Set(chip::EndpointId endpoint, const DataModel::Nullable<{{asUnder
{{/if}}
} // namespace {{asUpperCamelCase label}}

{{/if}}
{{/if_is_struct}}
{{/if}}
{{#last}}
} // namespace Attributes
Expand Down
Loading

0 comments on commit 998a0bf

Please sign in to comment.