-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[Demo] [ESP32] Enable the BLE stack and get the code to compile #1241
[Demo] [ESP32] Enable the BLE stack and get the code to compile #1241
Conversation
@@ -110,15 +87,15 @@ CHIP_ERROR GenericConfigurationManagerImpl<ImplClass>::_ConfigureChipStack() | |||
err = CHIP_NO_ERROR; | |||
} | |||
} | |||
exit: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we want to move this exit:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree this is hacky but I moved it on purposes. The main reason is that the current code does not compile if CHIP_DEVICE_CONFIG_LOG_PROVISIONING_HASH is not defined since the only call sites that call 'exit' are inside it and the compiler complains otherwise.
The solutions I have handy are:
- Do what I did
- Remove the code inside CHIP_DEVICE_CONFIG_LOG_PROVISIONING
- remove the 'exit:' label and rewrite the code accordingly
// Singleton instance of Chip Group Key Store for the ESP32 | ||
// | ||
// NOTE: This is declared as a private global variable, rather than a static | ||
// member of ConfigurationManagerImpl, to reduce the number of headers that | ||
// must be included by the application when using the ConfigurationManager API. | ||
// | ||
GroupKeyStoreImpl gGroupKeyStore; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a lot of removed code in this PR that I'm not seeing as directly related to "enabling the BLE stack". Were these changes intended to be included in the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a fair point. This file was not used and so there was a lot of dead code that does not even compile and was not used.
For example CHIP does not have all those namespaces:
::chip::Profiles::Security::AppKeys;
::chip::Profiles::NetworkProvisioning;
::chip::Profiles::DeviceDescription
I was unsure about what is the best strategy with this code.
Should I remove it, or should I just try to go and fix it, even if this code is completely unrelated to the BLE thing ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think removing is fine, as long as we annotate why here.
For the record, while digging into the opened PR I realised that this PR shares some similarities with #1192 since both bring ConfigurationManagerImpl into the build (#1192 for nRF5 and this one for ESP32). I believe the comments related to GroupKeyStoreImpl there will apply here: basically, add a TODO with an issue # |
I opened #1266 for the GroupKeyStore part and will add a TODO in my next update to the code. |
6160eb0
to
cd0d221
Compare
Is the 300+ KB increase on .text and +79KB .data increase for the ESP32 examples really correct? OUCH! |
That's likely due to Bluedroid as BT got pulled in. I am working on a NimBLE implementation for the BT layer, that should help bring this down. Hopefully will raise a PR early next week. |
cd0d221
to
3b39ee5
Compare
56d2c62
to
f7da2ff
Compare
f7da2ff
to
a18c62c
Compare
@vivien-apple merge conflicts |
7ec72ea
to
3da5d19
Compare
Rebased and the ESP32 CI is finally green... |
3da5d19
to
46f4ba3
Compare
Size increase report for "Build Examples [nRF]"
Full report output
|
Size increase report for "Build Examples [main-build]"
Full report output
|
Size increase report for "Build Examples [ESP32]"
Full report output
|
Ok. It is fully green now. |
…due to data-model update from CSA Merge in WMN_TOOLS/matter from bugfix/mpc-build-failure-fix to silabs Squashed commit of the following: commit 4702c471222d7ebf9e16a13d6e899a444e62a823 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 17:33:39 2023 +0530 Update .zap and .matter file with feature-level 98 commit d1ed41ba38e9c6bc6f8303458285b77370c1bd29 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 14:19:35 2023 +0530 Fixes build failure due to data-model update
…due to data-model update from CSA Merge in WMN_TOOLS/matter from bugfix/mpc-build-failure-fix to silabs Squashed commit of the following: commit 4702c471222d7ebf9e16a13d6e899a444e62a823 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 17:33:39 2023 +0530 Update .zap and .matter file with feature-level 98 commit d1ed41ba38e9c6bc6f8303458285b77370c1bd29 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 14:19:35 2023 +0530 Fixes build failure due to data-model update
…due to data-model update from CSA Merge in WMN_TOOLS/matter from bugfix/mpc-build-failure-fix to silabs Squashed commit of the following: commit 4702c471222d7ebf9e16a13d6e899a444e62a823 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 17:33:39 2023 +0530 Update .zap and .matter file with feature-level 98 commit d1ed41ba38e9c6bc6f8303458285b77370c1bd29 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 14:19:35 2023 +0530 Fixes build failure due to data-model update
…due to data-model update from CSA Merge in WMN_TOOLS/matter from bugfix/mpc-build-failure-fix to silabs Squashed commit of the following: commit 4702c471222d7ebf9e16a13d6e899a444e62a823 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 17:33:39 2023 +0530 Update .zap and .matter file with feature-level 98 commit d1ed41ba38e9c6bc6f8303458285b77370c1bd29 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 14:19:35 2023 +0530 Fixes build failure due to data-model update
…due to data-model update from CSA Merge in WMN_TOOLS/matter from bugfix/mpc-build-failure-fix to silabs Squashed commit of the following: commit 4702c471222d7ebf9e16a13d6e899a444e62a823 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 17:33:39 2023 +0530 Update .zap and .matter file with feature-level 98 commit d1ed41ba38e9c6bc6f8303458285b77370c1bd29 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 14:19:35 2023 +0530 Fixes build failure due to data-model update
…due to data-model update from CSA Merge in WMN_TOOLS/matter from bugfix/mpc-build-failure-fix to silabs Squashed commit of the following: commit 4702c471222d7ebf9e16a13d6e899a444e62a823 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 17:33:39 2023 +0530 Update .zap and .matter file with feature-level 98 commit d1ed41ba38e9c6bc6f8303458285b77370c1bd29 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 14:19:35 2023 +0530 Fixes build failure due to data-model update
…due to data-model update from CSA Merge in WMN_TOOLS/matter from bugfix/mpc-build-failure-fix to silabs Squashed commit of the following: commit 4702c471222d7ebf9e16a13d6e899a444e62a823 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 17:33:39 2023 +0530 Update .zap and .matter file with feature-level 98 commit d1ed41ba38e9c6bc6f8303458285b77370c1bd29 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 14:19:35 2023 +0530 Fixes build failure due to data-model update
…due to data-model update from CSA Merge in WMN_TOOLS/matter from bugfix/mpc-build-failure-fix to silabs Squashed commit of the following: commit 4702c471222d7ebf9e16a13d6e899a444e62a823 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 17:33:39 2023 +0530 Update .zap and .matter file with feature-level 98 commit d1ed41ba38e9c6bc6f8303458285b77370c1bd29 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 14:19:35 2023 +0530 Fixes build failure due to data-model update
…due to data-model update from CSA Merge in WMN_TOOLS/matter from bugfix/mpc-build-failure-fix to silabs Squashed commit of the following: commit 4702c471222d7ebf9e16a13d6e899a444e62a823 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 17:33:39 2023 +0530 Update .zap and .matter file with feature-level 98 commit d1ed41ba38e9c6bc6f8303458285b77370c1bd29 Author: Suhas Shankar <[email protected]> Date: Fri Oct 13 14:19:35 2023 +0530 Fixes build failure due to data-model update
Problem
Turning on CHIPoBLE on menuconfig for the ESP32 does not work.
Summary of Changes
The proposed patch get the M5Stack demo to advertise BLE. It will still needs some love in order to communicate with an app.
fixes #1240