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.

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

[TE6] Read Ethernet Network Diagnostic attributes from platform at run-time #9587

Merged
merged 3 commits into from
Sep 10, 2021
Merged

[TE6] Read Ethernet Network Diagnostic attributes from platform at run-time #9587

merged 3 commits into from
Sep 10, 2021

Conversation

yufengwangca
Copy link
Contributor

@yufengwangca yufengwangca commented Sep 10, 2021

Problem

What is being fixed? Examples:

  • Currently, the Ethernet Network Diagnostic attributes are fully managed by the attribute store, we can only specify the default values in its xml definitions. We need to populate those attributes with the actual value from platform in run-time.

Change overview

  • Change Ethernet Network Diagnostic to 'External' in lighting-app.
  • Read Ethernet Network Diagnostic attributes from platform at run-time.

Testing

How was this tested? (at least one bullet point required)

  • ./chip-tool ethernetnetworkdiagnostics read packet-rx-count 0
[1631236896.763861][1715471:1715478] CHIP:EM: Removed CHIP MsgId:00000000 from RetransTable
[1631236896.763876][1715471:1715478] CHIP:DMG: ReportData =
[1631236896.763883][1715471:1715478] CHIP:DMG: {
[1631236896.763888][1715471:1715478] CHIP:DMG: 	AttributeDataList =
[1631236896.763894][1715471:1715478] CHIP:DMG: 	[
[1631236896.763902][1715471:1715478] CHIP:DMG: 		AttributeDataElement =
[1631236896.763910][1715471:1715478] CHIP:DMG: 		{
[1631236896.763917][1715471:1715478] CHIP:DMG: 			AttributePath =
[1631236896.763924][1715471:1715478] CHIP:DMG: 			{
[1631236896.763932][1715471:1715478] CHIP:DMG: 				NodeId = 0x47a899eac0fe5bfa,
[1631236896.763941][1715471:1715478] CHIP:DMG: 				EndpointId = 0x0,
[1631236896.763950][1715471:1715478] CHIP:DMG: 				ClusterId = 0x37,
[1631236896.763958][1715471:1715478] CHIP:DMG: 				FieldTag = 0x2,
[1631236896.763966][1715471:1715478] CHIP:DMG: 			}
[1631236896.763976][1715471:1715478] CHIP:DMG: 				
[1631236896.763985][1715471:1715478] CHIP:DMG: 			Data = 4346, 
[1631236896.763992][1715471:1715478] CHIP:DMG: 			DataElementVersion = 0x0,
[1631236896.764000][1715471:1715478] CHIP:DMG: 		},
[1631236896.764010][1715471:1715478] CHIP:DMG: 		
[1631236896.764017][1715471:1715478] CHIP:DMG: 	],
[1631236896.764027][1715471:1715478] CHIP:DMG: 	
[1631236896.764033][1715471:1715478] CHIP:DMG: }
[1631236896.764042][1715471:1715478] CHIP:DMG: ProcessReportData handles the initial report
[1631236896.764069][1715471:1715478] CHIP:ZCL: ReadAttributesResponse:
[1631236896.764075][1715471:1715478] CHIP:ZCL:   ClusterId: 0x0000_0037
[1631236896.764083][1715471:1715478] CHIP:ZCL:   attributeId: 0x0000_0002
[1631236896.764089][1715471:1715478] CHIP:ZCL:   status: Success                (0x0000)
[1631236896.764096][1715471:1715478] CHIP:ZCL:   attribute TLV Type: 0x04
[1631236896.764103][1715471:1715478] CHIP:TOO: Int64u attribute Response: 4346

@yufengwangca yufengwangca requested review from andy31415, woody-apple and msandstedt and removed request for msandstedt, woody-apple and andy31415 September 10, 2021 13:51
Copy link
Contributor

@andy31415 andy31415 left a comment

Choose a reason for hiding this comment

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

Approved, however with the note that needing to copy&paste the implementation between all-clusters-app and lighting-app makes me sad:

  • other examples will NOT have this
  • this is a 'base cluster' so I would rather expect our code to have this working with as simple as some 'enableEthernetDiagnostics' call in main in an example app. Copy&pasting of 100 lines of code should not be required.

@github-actions
Copy link

Size increase report for "esp32-example-build" from 2b2622f

File Section File VM
chip-ipv6only-app.elf .flash.text -172 -172
chip-bridge-app.elf .flash.text -72 -72
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

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

sections,vmsize,filesize
.debug_info,0,5158
.debug_str,0,1057
.debug_loc,0,338
.debug_line,0,311
.debug_frame,0,120
.debug_aranges,0,40
.debug_ranges,0,40
.strtab,0,10
.shstrtab,0,-2

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

sections,vmsize,filesize
.debug_info,0,4449
.debug_str,0,1053
.debug_line,0,311
.debug_loc,0,199
.debug_frame,0,120
.debug_aranges,0,40
.debug_ranges,0,40

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

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

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-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,4449
.debug_str,0,1053
.debug_loc,0,351
.debug_line,0,311
.debug_frame,0,120
[Unmapped],0,72
.debug_aranges,0,40
.debug_ranges,0,40
.strtab,0,10
.shstrtab,0,-2
.flash.text,-72,-72

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

sections,vmsize,filesize
.debug_info,0,4940
.debug_str,0,1056
.debug_loc,0,226
.debug_line,0,150
.debug_frame,0,80
.debug_aranges,0,40
.debug_ranges,0,40
.debug_abbrev,0,18
.strtab,0,10
.riscv.attributes,0,2
.shstrtab,0,2

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

sections,vmsize,filesize

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

sections,vmsize,filesize
.debug_info,0,4455
.debug_str,0,1126
.debug_loc,0,329
.debug_line,0,310
.debug_frame,0,120
.debug_aranges,0,40
.debug_ranges,0,40
.strtab,0,10
.shstrtab,0,-2


@woody-apple woody-apple marked this pull request as draft September 10, 2021 14:58
@yufengwangca
Copy link
Contributor Author

yufengwangca commented Sep 10, 2021

Approved, however with the note that needing to copy&paste the implementation between all-clusters-app and lighting-app makes me sad:

  • other examples will NOT have this
  • this is a 'base cluster' so I would rather expect our code to have this working with as simple as some 'enableEthernetDiagnostics' call in main in an example app. Copy&pasting of 100 lines of code should not be required.

These implementations are platform specific code and currently different examples do not share the code for this part. We do have a lot of dup in this area, need to figure out a common solution to address all of them together. We may need to reactor the current example architecture to achieve this goal.

Investigated more, currently each example has its own zap file, and manage its attribute by itself, this implementation is the external attribute callbacks and need to be added to the example which enable the attribute as 'External'.

@woody-apple woody-apple marked this pull request as ready for review September 10, 2021 14:58
@andy31415 andy31415 merged commit eda07d5 into project-chip:master Sep 10, 2021
@yufengwangca yufengwangca deleted the pr/cluster/eth branch September 10, 2021 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants