Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide destination and endpoint while requesting for reports #9466

Closed
wants to merge 1 commit into from
Closed

Provide destination and endpoint while requesting for reports #9466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion examples/chip-tool/templates/commands.zapt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ public:
return err;
}

return cluster.ConfigureAttribute{{asUpperCamelCase name}}(onSuccessCallback->Cancel(), onFailureCallback->Cancel(), mMinInterval, mMaxInterval{{#if isAnalog}}, mChange{{/if}});
return cluster.ConfigureAttribute{{asUpperCamelCase name}}(onSuccessCallback->Cancel(), onFailureCallback->Cancel(), GetExecContext()->localId, 1, mMinInterval, mMaxInterval{{#if isAnalog}}, mChange{{/if}});
}

private:
Expand Down
71 changes: 58 additions & 13 deletions src/app/reporting/reporting.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,13 @@ using namespace chip;

#define NULL_INDEX 0xFF

static void conditionallySendReport(EndpointId endpoint, ClusterId clusterId);
/*
* This method sends the report to the provided destination node at destinationEndpoint.
* If the reportDestination is kUndefinedNodeId, the method uses binding table to identify
* destination nodes and endpoints
*/
static void conditionallySendReport(EndpointId endpoint, ClusterId clusterId, NodeId reportDestination,
Copy link
Contributor

@kghost kghost Sep 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the comment, It is confusing that the code works this way. Can it take a MessageSendDestination argument instead of NodeId reportDestination

We can extract a function out of emberAfSendUnicastToBindingsWithCallback:

MessageSendDestination emberAfGetDestinationByBinding(EmberApsFrame * apsFrame)
{
    ...
}

It can help to eliminate lots of if condition tests inside conditionallySendReport

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pan-apple?

EndpointId destinationEndpoint);
static void scheduleTick(void);
static void removeConfiguration(uint8_t index);
static void removeConfigurationAndScheduleTick(uint8_t index);
Expand Down Expand Up @@ -206,6 +212,9 @@ void emberAfPluginReportingTickEventHandler(void)
uint8_t index;
uint16_t currentPayloadMaxLength = 0, smallestPayloadMaxLength = 0;

NodeId reportDestination = kUndefinedNodeId;
EndpointId destinationEndpoint = 0;

for (i = 0; i < REPORT_TABLE_SIZE; i++)
{
EmberAfPluginReportingEntry entry;
Expand Down Expand Up @@ -259,15 +268,17 @@ void emberAfPluginReportingTickEventHandler(void)
{
emberAfReportingPrintln("Reporting Entry Full - creating new report");
}
conditionallySendReport(apsFrame->sourceEndpoint, apsFrame->clusterId);
conditionallySendReport(apsFrame->sourceEndpoint, apsFrame->clusterId, reportDestination, destinationEndpoint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have implemented emberAfGetDestinationByBinding, here it can be:

conditionallySendReport(apsFrame->sourceEndpoint, apsFrame->clusterId, emberAfGetDestinationByBinding(apsFrame), destinationEndpoint);

apsFrame = NULL;
}

// If we haven't made the message header, make it.
if (apsFrame == NULL)
{
apsFrame = emberAfGetCommandApsFrame();
clientToServer = emberAfClusterIsClient(&entry);
apsFrame = emberAfGetCommandApsFrame();
clientToServer = emberAfClusterIsClient(&entry);
reportDestination = entry.data.reported.reportDestination;
destinationEndpoint = entry.data.reported.destinationEndpoint;
// The manufacturer-specfic version of the fill API only creates a
// manufacturer-specfic command if the manufacturer code is set. For
// non-manufacturer-specfic reports, the manufacturer code is unset, so
Expand Down Expand Up @@ -354,17 +365,27 @@ void emberAfPluginReportingTickEventHandler(void)

if (apsFrame != NULL)
{
conditionallySendReport(apsFrame->sourceEndpoint, apsFrame->clusterId);
conditionallySendReport(apsFrame->sourceEndpoint, apsFrame->clusterId, reportDestination, destinationEndpoint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little confused about why there is another if (apsFrame != NULL) test, is it able to move this statement inside if (apsFrame == NULL) branch ?

}
scheduleTick();
}

static void conditionallySendReport(EndpointId endpoint, ClusterId clusterId)
static void conditionallySendReport(EndpointId endpoint, ClusterId clusterId, NodeId reportDestination,
EndpointId destinationEndpoint)
{
EmberStatus status;
if (emberAfIsDeviceEnabled(endpoint) || clusterId == ZCL_IDENTIFY_CLUSTER_ID)
{
status = emberAfSendCommandUnicastToBindingsWithCallback(&retrySendReport);
if (reportDestination == kUndefinedNodeId)
{
status = emberAfSendCommandUnicastToBindingsWithCallback(&retrySendReport);
}
else
{
emAfCommandApsFrame->destinationEndpoint = destinationEndpoint;
status = emberAfSendUnicastWithCallback(MessageSendDestination::Direct(reportDestination), emAfCommandApsFrame,
*emAfResponseLengthPtr, emAfZclBuffer, &retrySendReport);
}

// If the callback table is full, attempt to send the message with no
// callback. Note that this could lead to a message failing to transmit
Expand All @@ -373,9 +394,16 @@ static void conditionallySendReport(EndpointId endpoint, ClusterId clusterId)
// all because the system hits its callback queue limit.
if (status == EMBER_TABLE_FULL)
{
emberAfSendCommandUnicastToBindings();
if (reportDestination == kUndefinedNodeId)
{
emberAfSendCommandUnicastToBindings();
}
else
{
emberAfSendUnicast(MessageSendDestination::Direct(reportDestination), emAfCommandApsFrame, *emAfResponseLengthPtr,
emAfZclBuffer);
}
}

#ifdef EMBER_AF_PLUGIN_REPORTING_ENABLE_GROUP_BOUND_REPORTS
emberAfSendCommandMulticastToBindings();
#endif // EMBER_AF_PLUGIN_REPORTING_ENABLE_GROUP_BOUND_REPORTS
Expand Down Expand Up @@ -435,6 +463,8 @@ bool emberAfConfigureReportingCommandCallback(const EmberAfClusterCommand * cmd)
EmberAfAttributeType dataType;
uint16_t minInterval, maxInterval;
uint32_t reportableChange = 0;
uint64_t reportDestination;
uint16_t destinationEndpoint;
EmberAfPluginReportingEntry newEntry;

dataType = (EmberAfAttributeType) emberAfGetInt8u(cmd->buffer, bufIndex, cmd->bufLen);
Expand All @@ -444,7 +474,13 @@ bool emberAfConfigureReportingCommandCallback(const EmberAfClusterCommand * cmd)
maxInterval = emberAfGetInt16u(cmd->buffer, bufIndex, cmd->bufLen);
bufIndex = static_cast<uint16_t>(bufIndex + 2);

emberAfReportingPrintln(" type:%x, min:%2x, max:%2x", dataType, minInterval, maxInterval);
reportDestination = emberAfGetInt64u(cmd->buffer, bufIndex, cmd->bufLen);
bufIndex = static_cast<uint16_t>(bufIndex + 8);
destinationEndpoint = emberAfGetInt16u(cmd->buffer, bufIndex, cmd->bufLen);
bufIndex = static_cast<uint16_t>(bufIndex + 2);

emberAfReportingPrintln(" type:%x, min:%2x, max:%2x, destination:" ChipLogFormatX64 ", destEP:%2x", dataType,
minInterval, maxInterval, ChipLogValueX64(reportDestination), destinationEndpoint);
emberAfReportingFlush();

if (emberAfGetAttributeAnalogOrDiscreteType(dataType) == EMBER_AF_DATA_TYPE_ANALOG)
Expand Down Expand Up @@ -489,7 +525,11 @@ bool emberAfConfigureReportingCommandCallback(const EmberAfClusterCommand * cmd)
newEntry.data.reported.minInterval = minInterval;
newEntry.data.reported.maxInterval = maxInterval;
newEntry.data.reported.reportableChange = reportableChange;
status = emberAfPluginReportingConfigureReportedAttribute(&newEntry);

newEntry.data.reported.reportDestination = reportDestination;
newEntry.data.reported.destinationEndpoint = destinationEndpoint;

status = emberAfPluginReportingConfigureReportedAttribute(&newEntry);
}
break;
}
Expand Down Expand Up @@ -882,8 +922,10 @@ EmberAfStatus emberAfPluginReportingConfigureReportedAttribute(const EmberAfPlug
{
emAfPluginReportingGetEntry(i, &entry);
if (entry.direction == EMBER_ZCL_REPORTING_DIRECTION_REPORTED && entry.endpoint == newEntry->endpoint &&
entry.clusterId == newEntry->clusterId && entry.attributeId == newEntry->attributeId && entry.mask == newEntry->mask &&
entry.manufacturerCode == newEntry->manufacturerCode)
entry.clusterId == newEntry->clusterId && entry.attributeId == newEntry->attributeId &&
entry.data.reported.reportDestination == newEntry->data.reported.reportDestination &&
entry.data.reported.destinationEndpoint == newEntry->data.reported.destinationEndpoint &&
entry.mask == newEntry->mask && entry.manufacturerCode == newEntry->manufacturerCode)
{
initialize = false;
index = i;
Expand Down Expand Up @@ -938,6 +980,9 @@ EmberAfStatus emberAfPluginReportingConfigureReportedAttribute(const EmberAfPlug
entry.attributeId = newEntry->attributeId;
entry.mask = newEntry->mask;
entry.manufacturerCode = newEntry->manufacturerCode;

entry.data.reported.reportDestination = newEntry->data.reported.reportDestination;
entry.data.reported.destinationEndpoint = newEntry->data.reported.destinationEndpoint;
if (index < REPORT_TABLE_SIZE)
{
emAfPluginReportVolatileData[index].lastReportTimeMs =
Expand Down
4 changes: 4 additions & 0 deletions src/app/util/af-types.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1065,6 +1065,10 @@ typedef struct
* being sent.
*/
uint32_t reportableChange;
/** The node id to which the report should be sent. */
chip::NodeId reportDestination;
/** The endpoint to which the report should be sent. */
chip::EndpointId destinationEndpoint;
} reported;
struct
{
Expand Down
6 changes: 4 additions & 2 deletions src/app/zap-templates/templates/app/CHIPClusters-src.zapt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::WriteAttribute{{asUpperCamel

{{/if}}
{{#if isReportableAttribute}}
CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::ConfigureAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, uint16_t minInterval, uint16_t maxInterval{{#if isAnalog}}, {{chipType}} change{{/if}})
CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::ConfigureAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, NodeId reportDestination, EndpointId destinationEndpoint, uint16_t minInterval, uint16_t maxInterval{{#if isAnalog}}, {{chipType}} change{{/if}})
{
COMMAND_HEADER("Report{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}", {{asUpperCamelCase parent.name}}::Id);
buf.Put8(kFrameControlGlobalCommand)
Expand All @@ -169,7 +169,9 @@ CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::ConfigureAttribute{{asUpperC
.Put32({{#if isGlobalAttribute}}Globals{{else}}{{asUpperCamelCase parent.name}}{{/if}}::Attributes::Ids::{{asUpperCamelCase name}})
.Put8({{atomicTypeId}})
.Put16(minInterval)
.Put16(maxInterval);
.Put16(maxInterval)
.Put64(reportDestination)
.Put16(destinationEndpoint);
{{#if isAnalog}}
buf.Put{{chipTypePutLength}}(static_cast<{{chipTypePutCastType}}>(change));
{{/if}}
Expand Down
2 changes: 1 addition & 1 deletion src/app/zap-templates/templates/app/CHIPClusters.zapt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public:
{{/chip_server_cluster_attributes}}
{{#chip_server_cluster_attributes}}
{{#if isReportableAttribute}}
CHIP_ERROR ConfigureAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, uint16_t minInterval, uint16_t maxInterval{{#if isAnalog}}, {{chipType}} change{{/if}});
CHIP_ERROR ConfigureAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, NodeId reportDestination, EndpointId destinationEndpoint, uint16_t minInterval, uint16_t maxInterval{{#if isAnalog}}, {{chipType}} change{{/if}});
CHIP_ERROR ReportAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onReportCallback);
{{/if}}
{{/chip_server_cluster_attributes}}
Expand Down
Loading