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

[WiFi Diagnostics Cluster] Enable WiFi Network Diagnostics cluster on the controllers #7365

Merged
merged 3 commits into from
Jun 29, 2021
Merged

[WiFi Diagnostics Cluster] Enable WiFi Network Diagnostics cluster on the controllers #7365

merged 3 commits into from
Jun 29, 2021

Conversation

yufengwangca
Copy link
Contributor

@yufengwangca yufengwangca commented Jun 3, 2021

Problem

What is being fixed?
WiFi Network Diagnostics cluster is not enabled on the controllers.

Change overview

  1. Enable Wifi diagnostics cluster in controller-clusters.zap

Testing

How was this tested? (at least one bullet point required)
Run ./scripts/tools/zap_regen_all.py and conform codegen can re-generate the code without error.
Run ./chip-tool and confirm wifinetworkdiagnostics is in the cluster list.

@bzbarsky-apple
Copy link
Contributor

Currently, codegen does not generate code for the items if we set optional to 'true' in the cluster xml definition file.

Even if the .zap file says they are enabled? @vivien-apple that sounds odd....

@yufengwangca yufengwangca changed the title [WiFi Diagnostics Cluster] Set optional to false for items whose Conformance level not set to 'O' [WiFi Diagnostics Cluster] Enable WiFi Network Diagnostics cluster on the controllers Jun 3, 2021
@yufengwangca
Copy link
Contributor Author

Currently, codegen does not generate code for the items if we set optional to 'true' in the cluster xml definition file.

Even if the .zap file says they are enabled? @vivien-apple that sounds odd....

This is because the ZAP UI tools does not mark those items as enabled if optional is set to 'true' in xml definition file. With latest version of ZAP UI tools, we can manually enable those items and update the .zap file.

@bzbarsky-apple
Copy link
Contributor

That sounds like a better approach: the XML should say what the spec says (ideally not just optional but what feature enables it), the UI should expose the toggling ability, we should turn on which things we want in the UI, and the zap file should capture those decisions.

@yufengwangca
Copy link
Contributor Author

That sounds like a better approach: the XML should say what the spec says (ideally not just optional but what feature enables it), the UI should expose the toggling ability, we should turn on which things we want in the UI, and the zap file should capture those decisions.

Yes, the XML does not fully match with the spec, that is the cause of current confusion.

@woody-apple
Copy link
Contributor

/rebase

2 similar comments
@woody-apple
Copy link
Contributor

/rebase

@woody-apple
Copy link
Contributor

/rebase

@github-actions
Copy link

Size increase report for "esp32-example-build" from b9dad95

File Section File VM
chip-all-clusters-app.elf .flash.rodata 112 112
chip-all-clusters-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
.flash.rodata,112,112
.dram0.bss,32,0
[Unmapped],0,-112

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

sections,vmsize,filesize


@yufengwangca
Copy link
Contributor Author

@bzbarsky-apple
Copy link
Contributor

@yufengwangca This is still changing the XML to mark optional things as non-optional, instead of enabling them in the ZAP UI if we want them enabled. Why are we doing that?

That is, why are we not doing the thing #7365 (comment) proposed doing?

@yufengwangca
Copy link
Contributor Author

@yufengwangca This is still changing the XML to mark optional things as non-optional, instead of enabling them in the ZAP UI if we want them enabled. Why are we doing that?

That is, why are we not doing the thing #7365 (comment) proposed doing?

@bzbarsky-apple I am confused, which optional item is marked as non-optional? This is only one item "Current MaxRate" is optional in the spec, this PR marked the items whose Conformance level not set to 'O' as non-optional to align with other XMLs.

@woody-apple
Copy link
Contributor

@yufengwangca conflicts :(

@yufengwangca
Copy link
Contributor Author

@yufengwangca This is still changing the XML to mark optional things as non-optional, instead of enabling them in the ZAP UI if we want them enabled. Why are we doing that?
That is, why are we not doing the thing #7365 (comment) proposed doing?

@bzbarsky-apple I am confused, which optional item is marked as non-optional? This is only one item "Current MaxRate" is optional in the spec, this PR marked the items whose Conformance level not set to 'O' as non-optional to align with other XMLs.

Synced offline, decided to leave items whose Conformance level not set to 'O' nor 'M' optional in XML and enable those items only in ZAP.

@saurabhst saurabhst merged commit f014dd5 into project-chip:master Jun 29, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
… the controllers (project-chip#7365)

* Add WiFiNetworkDiagnostics to helper.js to generate test code in darwin

* Update Wifi diagnostics cluster in controller-clusters.zap and all-clusters-app.zap

* Update gen folders
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.

8 participants