-
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
ESP: Add chip shell engine initializing for esp32 platform #13364
ESP: Add chip shell engine initializing for esp32 platform #13364
Conversation
Ok. We should also check the high watermark for the stack (if not already done) and accordingly increase the size |
PR #13364: Size comparison from b81facc to a22bdbf Increases (1 build for esp32)
Full report (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
a22bdbf
to
ca94852
Compare
PR #13364: Size comparison from b81facc to ca94852 Full report (4 builds for qpg, telink)
|
ca94852
to
2dd7af0
Compare
Seems the stack overflow is caused by the missing of chip shell engine initializing. I have changed the describe of this PR. |
PR #13364: Size comparison from b81facc to 2dd7af0 Increases above 0.2%:
Increases (1 build for esp32)
Full report (31 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
@wqx6 @shubhamdp Will it cause issues in all the ESP32 examples that use the shell? If yes, should we fastrack it @andy31415 ? |
ToT is broken, #13368 attempts to fix. We can fast track this, but it also has sufficient checkmarks to merge (except CI will not pass due to ToT failure) |
I admin merged - darwin failure is not caused by this PR and the PR passed ESP32 CI. |
Has this PR broken the ESP32 workflow?
|
Yes, I see it has broken CI for ESP32 on other PRs, with the same error. E.g.: |
* [shell] Add command to start/stop zcl app server. - Add 'server port' and 'server udcport' commands. - Add 'server sessions' command. - Add 'server endpoints' and 'server clusters' commands. - Add docs for the new 'server' command subcommands (README_SERVER.md). - Add 'server' command to build of linux and mac platforms only initially. - Improve clean exit paths for 'server' command. - Update 'ForEachSessionHandle' to use new Loop construct. - Fix build failure in ota-provider (warning when logs disabled). * [shell] Correct order of atexit and shutdown handling. * [shell] Add OTA stubs. * Apply suggestions from code review Co-authored-by: Łukasz Duda <[email protected]> * [session] Update ForEachSessionHandle to new API. - Revert erroneous shutdown protection in Server.cpp - Add app-level shutdown protection in cmd_server.cpp * Bugfix: Add chip shell engine initializing for esp32 platform (#13364) * [shell] Code review fixes. Co-authored-by: Łukasz Duda <[email protected]> Co-authored-by: Wang Qixiang <[email protected]>
* [shell] Add command to start/stop zcl app server. - Add 'server port' and 'server udcport' commands. - Add 'server sessions' command. - Add 'server endpoints' and 'server clusters' commands. - Add docs for the new 'server' command subcommands (README_SERVER.md). - Add 'server' command to build of linux and mac platforms only initially. - Improve clean exit paths for 'server' command. - Update 'ForEachSessionHandle' to use new Loop construct. - Fix build failure in ota-provider (warning when logs disabled). * [shell] Correct order of atexit and shutdown handling. * [shell] Add OTA stubs. * Apply suggestions from code review Co-authored-by: Łukasz Duda <[email protected]> * [session] Update ForEachSessionHandle to new API. - Revert erroneous shutdown protection in Server.cpp - Add app-level shutdown protection in cmd_server.cpp * Bugfix: Add chip shell engine initializing for esp32 platform (project-chip#13364) * [shell] Code review fixes. Co-authored-by: Łukasz Duda <[email protected]> Co-authored-by: Wang Qixiang <[email protected]>
Problem
The chip shell engine initializing was missing for esp32 platform. It cause a stack overflow for chip shell task.
Change overview
And chip shell engine initializing.
Testing
Test with all-clusters-app.