Provide destination and endpoint while requesting for reports #9466

pan-apple · 2021-09-03T17:58:54Z

Problem

Reporting should not require a client to setup a binding table entry.

Change overview

Add target destination (node and endpoint) to ConfigureAttribute commands. This command is used for turning on/off reporting for an attribute. The node generating the report will use the target information to dispatch the reports.

Testing

Removed the binding steps from the current unit test, and ensure it continues to work (receive reports).

kghost · 2021-09-03T18:20:23Z

src/app/reporting/reporting.cpp

+ * 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,


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

@pan-apple?

kghost · 2021-09-03T18:29:23Z

src/app/reporting/reporting.cpp

@@ -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);


If we have implemented emberAfGetDestinationByBinding, here it can be:

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

kghost · 2021-09-03T18:32:58Z

src/app/reporting/reporting.cpp

@@ -354,17 +365,27 @@ void emberAfPluginReportingTickEventHandler(void)

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


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 ?

andy31415 · 2021-09-03T18:40:42Z

We are having discussion required regarding the PRs for subscriptions regarding usage or not usage of reporting.

Marking this discussion required: I assume this is the PR (or part of the PR) that will be considered for a proposal for subscription implementation.

andy31415

Marking requested changes: please provide a complete PR for anything related to subscriptions, so we can compare approaches.

github-actions · 2021-09-03T18:47:47Z

Size increase report for "gn_qpg-example-build" from 8f80988

File	Section	File	VM
chip-qpg6100-lighting-example.out	.text	212	212
chip-qpg6100-lighting-example.out	.bss	0	96
chip-qpg6100-lighting-example.out	.heap	0	-96

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-qpg6100-lighting-example.out.map and ./pull_artifact/chip-qpg6100-lighting-example.out.map:

BLOAT EXECUTION FAILED WITH CODE 1:
bloaty: unknown file type for file './pull_artifact/chip-qpg6100-lighting-example.out.map'

Comparing ./master_artifact/chip-qpg6100-lighting-example.out and ./pull_artifact/chip-qpg6100-lighting-example.out:

sections,vmsize,filesize
.debug_info,0,3680
.debug_str,0,838
.debug_loc,0,817
.debug_line,0,399
.debug_ranges,0,216
.text,212,212
.bss,96,0
.debug_abbrev,0,74
.symtab,0,32
.strtab,0,26
.debug_frame,0,24
.shstrtab,0,-2
.heap,-96,0
[Unmapped],0,-212

woody-apple · 2021-09-03T19:04:50Z

We should discuss and next week’s meeting about how to proceed here, however we’ll need to timebox this so as to unblock subscription work.

github-actions · 2021-09-03T19:14:34Z

Size increase report for "nrfconnect-example-build" from 8f80988

File	Section	File	VM
chip-lock.elf	text	108	108
chip-lock.elf	rodata	40	40
chip-lock.elf	[LOAD #3 [RW]]	0	28
chip-lock.elf	bss	0	4
chip-lock.elf	device_handles	4	4

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-lock.elf and ./pull_artifact/chip-lock.elf:

sections,vmsize,filesize
.debug_info,0,3682
.debug_loc,0,984
.debug_str,0,840
.debug_line,0,428
.debug_ranges,0,240
text,108,108
.debug_abbrev,0,58
rodata,40,40
.symtab,0,32
[LOAD #3 [RW]],28,0
.strtab,0,26
.debug_frame,0,24
bss,4,0
device_handles,4,4
.shstrtab,0,2

github-actions · 2021-09-03T19:15:37Z

Size increase report for "esp32-example-build" from 8f80988

File	Section	File	VM
chip-all-clusters-app.elf	.dram0.bss	0	704
chip-all-clusters-app.elf	.flash.rodata	384	384
chip-all-clusters-app.elf	.flash.text	268	268
chip-temperature-measurement-app.elf	.flash.text	164	164
chip-temperature-measurement-app.elf	.flash.rodata	48	48
chip-temperature-measurement-app.elf	.dram0.bss	0	16
chip-shell.elf	.flash.text	48	48
chip-lock-app.elf	.flash.text	132	132
chip-lock-app.elf	.flash.rodata	40	40
chip-lock-app.elf	.dram0.bss	0	16
chip-bridge-app.elf	.flash.text	96	96
chip-bridge-app.elf	.flash.rodata	56	56
chip-bridge-app.elf	.dram0.bss	0	32

Full report output

BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_info,0,3112
.debug_str,0,840
.dram0.bss,704,0
.debug_line,0,536
.debug_loc,0,512
.flash.rodata,384,384
.flash.text,268,268
.debug_ranges,0,64
.debug_abbrev,0,40
.strtab,0,29
.debug_frame,0,28
.symtab,0,16
.shstrtab,0,-1
[Unmapped],0,-652

Comparing ./master_artifact/chip-temperature-measurement-app.elf and ./pull_artifact/chip-temperature-measurement-app.elf:

sections,vmsize,filesize
.debug_info,0,3810
.debug_loc,0,1178
.debug_str,0,838
.debug_line,0,794
.debug_ranges,0,208
.flash.text,164,164
.debug_abbrev,0,60
.flash.rodata,48,48
.strtab,0,26
.dram0.bss,16,0
.symtab,0,16
.shstrtab,0,2
[Unmapped],0,-212

Comparing ./master_artifact/chip-shell.elf and ./pull_artifact/chip-shell.elf:

sections,vmsize,filesize
.flash.text,48,48
[Unmapped],0,-48

Comparing ./master_artifact/chip-lock-app.elf and ./pull_artifact/chip-lock-app.elf:

sections,vmsize,filesize
.debug_info,0,3817
.debug_loc,0,1347
.debug_str,0,841
.debug_line,0,803
.debug_ranges,0,208
.flash.text,132,132
.debug_abbrev,0,44
.flash.rodata,40,40
.strtab,0,26
.dram0.bss,16,0
.symtab,0,16
.shstrtab,0,2
[Unmapped],0,-172

Comparing ./master_artifact/chip-ipv6only-app.elf and ./pull_artifact/chip-ipv6only-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-bridge-app.elf and ./pull_artifact/chip-bridge-app.elf:

sections,vmsize,filesize
.debug_info,0,3795
.debug_loc,0,1161
.debug_str,0,839
.debug_line,0,817
.debug_ranges,0,208
.flash.text,96,96
.debug_abbrev,0,60
.flash.rodata,56,56
.dram0.bss,32,0
.strtab,0,26
.symtab,0,16
.shstrtab,0,2
[Unmapped],0,-152

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-persistent-storage.elf and ./pull_artifact/chip-persistent-storage.elf:

sections,vmsize,filesize

woody-apple · 2021-09-08T16:29:44Z

Per discussion today, closing this PR - given this is now covered here: #9510

Provide destination and endpoint while requesting for reports

8cf73c8

boring-cyborg bot added app controller darwin examples labels Sep 3, 2021

pullapprove bot requested review from chrisdecenzo, jelderton, jmartinez-silabs, LuDuda, mspang, sagar-apple, saurabhst, selissia and woody-apple September 3, 2021 18:03

pullapprove bot added the review - pending label Sep 3, 2021

pan-apple requested review from vivien-apple and bzbarsky-apple September 3, 2021 18:05

kghost reviewed Sep 3, 2021

View reviewed changes

andy31415 added the SDK Discussion Required label Sep 3, 2021

andy31415 requested changes Sep 3, 2021

View reviewed changes

woody-apple requested a review from andy31415 September 3, 2021 19:05

woody-apple approved these changes Sep 7, 2021

View reviewed changes

woody-apple mentioned this pull request Sep 8, 2021

Add and Enable IM subscription support #9510

Merged

woody-apple closed this Sep 8, 2021

pan-apple deleted the reporting branch September 28, 2021 16:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide destination and endpoint while requesting for reports #9466

Provide destination and endpoint while requesting for reports #9466

pan-apple commented Sep 3, 2021

kghost Sep 3, 2021 •

edited

Loading

woody-apple Sep 7, 2021

kghost Sep 3, 2021

kghost Sep 3, 2021

andy31415 commented Sep 3, 2021

andy31415 left a comment

github-actions bot commented Sep 3, 2021

woody-apple commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

woody-apple commented Sep 8, 2021

Provide destination and endpoint while requesting for reports #9466

Provide destination and endpoint while requesting for reports #9466

Conversation

pan-apple commented Sep 3, 2021

Problem

Change overview

Testing

kghost Sep 3, 2021 • edited Loading

Choose a reason for hiding this comment

woody-apple Sep 7, 2021

Choose a reason for hiding this comment

kghost Sep 3, 2021

Choose a reason for hiding this comment

kghost Sep 3, 2021

Choose a reason for hiding this comment

andy31415 commented Sep 3, 2021

andy31415 left a comment

Choose a reason for hiding this comment

github-actions bot commented Sep 3, 2021

woody-apple commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

woody-apple commented Sep 8, 2021

kghost Sep 3, 2021 •

edited

Loading