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

Add emberAf functions definition for the new network commissioning cluster #15189

Closed

Conversation

wqx6
Copy link
Contributor

@wqx6 wqx6 commented Feb 15, 2022

Problem

The updated network commissioning needs definitions of the emberAf functions and MatterNetworkCommissioningPluginServerInitCallback.

Change overview

Add the definitions for the new network commissioning cluster

Testing

Tested on ESP32C3 with all-clusters-app, the commissioning works well and the scannetwork command also works.

…atterNetworkCommissioningPluginServerInitCallback
@github-actions
Copy link

github-actions bot commented Feb 15, 2022

PR #15189: Size comparison from a35b95a to ed8b330

Decreases (3 builds for esp32, telink)
platform target config section a35b95a ed8b330 change % change
esp32 all-clusters-app c3devkit (read only) 945792 944894 -898 -0.1
(read/write) 1398562 1397850 -712 -0.1
.dram0.bss 66248 65712 -536 -0.8
.flash.rodata 199144 198976 -168 -0.1
.flash.text 945792 944894 -898 -0.1
m5stack (read only) 995651 995139 -512 -0.1
(read/write) 463840 463240 -600 -0.1
.dram0.bss 71392 70856 -536 -0.8
.flash.rodata 226256 226192 -64 -0.0
.flash.text 990267 989755 -512 -0.1
telink lighting-app tlsr9518adk80d (read/write) 875938 873802 -2136 -0.2
bss 88580 87420 -1160 -1.3
text 615704 614880 -824 -0.1
Full report (3 builds for esp32, telink)
platform target config section a35b95a ed8b330 change % change
esp32 all-clusters-app c3devkit (read only) 945792 944894 -898 -0.1
(read/write) 1398562 1397850 -712 -0.1
.dram0.bss 66248 65712 -536 -0.8
.dram0.data 14268 14268 0 0.0
.flash.rodata 199144 198976 -168 -0.1
.flash.text 945792 944894 -898 -0.1
.iram0.text 62056 62056 0 0.0
m5stack (read only) 995651 995139 -512 -0.1
(read/write) 463840 463240 -600 -0.1
.dram0.bss 71392 70856 -536 -0.8
.dram0.data 34064 34064 0 0.0
.flash.rodata 226256 226192 -64 -0.0
.flash.text 990267 989755 -512 -0.1
.iram0.text 123399 123399 0 0.0
telink lighting-app tlsr9518adk80d (read/write) 875938 873802 -2136 -0.2
bss 88580 87420 -1160 -1.3
noinit 37160 37160 0 0.0
text 615704 614880 -824 -0.1

@@ -468,3 +468,47 @@ void Instance::OnCommissioningComplete(CHIP_ERROR err)
} // namespace Clusters
} // namespace app
} // namespace chip

bool emberAfNetworkCommissioningClusterAddOrUpdateThreadNetworkCallback(
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we do something to not generate calls to these at all, since it's a waste of codesize? Followup needed for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but now there are still platforms which don't have full network commissioning driver support, and they need these functions. We could remove the generate calls after all the platforms add the updated network commissioning driver and the network-commissioning-old is removed.

Copy link
Contributor

Choose a reason for hiding this comment

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

That is why I said "followup needed".

return false;
}

void MatterNetworkCommissioningPluginServerInitCallback() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably go over where the other stubs like this live.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I move this function to callback-stub.cpp?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, these are in src/app/util/util.cpp around line 240.

@@ -96,8 +96,6 @@ target_sources(app PRIVATE
${CHIP_ROOT}/src/app/clusters/color-control-server/color-control-server.cpp
${CHIP_ROOT}/src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.cpp
${CHIP_ROOT}/src/app/clusters/network-commissioning/network-commissioning.cpp
${CHIP_ROOT}/src/app/clusters/network-commissioning-old/network-commissioning-ember.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit confused for the change to telink build file since other files are all under esp32 domain.

Copy link
Contributor

@erjiaqing erjiaqing left a comment

Choose a reason for hiding this comment

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

Thanks

@wqx6
Copy link
Contributor Author

wqx6 commented Feb 23, 2022

Close this PR since #15184 merged.

@wqx6 wqx6 closed this Feb 23, 2022
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.

3 participants