[EFR32] Windows application modifications #22182
Conversation
|
|
|
Exact same things as #22124 with the same problems |
|
PR #22182: Size comparison from 7870328 to 05fe3d8 Increases above 0.2%:
Increases (3 builds for bl602, cc13x2_26x2, efr32)
Decreases (3 builds for cc13x2_26x2, psoc6, telink)
Full report (43 builds for bl602, cc13x2_26x2, cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, psoc6, telink)
|
|
After some attentive reading some problems were fixed in this one, but not all of them. Re-opening it. |
| @@ -157,6 +157,7 @@ class WindowApp | |||
|
|
|||
| virtual ~WindowApp() = default; | |||
| virtual CHIP_ERROR Init(); | |||
| virtual CHIP_ERROR StartAppTask(); | |||
There was a problem hiding this comment.
This need to be remove since it's not needed.
There was a problem hiding this comment.
Called StartApptask function from the main() which will create AppTaskMain task and the Event Queue.
The AppTaskMain will initializing the window application and run the event handler loop.Removed the redundant task exist before and now having only one task and the flow is similar for both WiFi and Thread.
The design flow is similar to the Light and door-lock apps except the base application class reference.
There was a problem hiding this comment.
@jepenven-silabs In main function calling startAppTask() through window object called "app" hence we declare and define virtual startAppTask() in windowApp.h and windowApp.cpp file respectively
If removed it will get linking error
There was a problem hiding this comment.
The goal is to remove it completely since it breaks the abstraction layer. There shouldn't be any reference to any AppTask anywhere in this demo...
| @@ -109,10 +120,26 @@ WindowApp::Cover * WindowApp::GetCover(chip::EndpointId endpoint) | |||
| return nullptr; | |||
| } | |||
|
|
|||
| CHIP_ERROR WindowApp::StartAppTask() { return CHIP_NO_ERROR;} | |||
There was a problem hiding this comment.
Please remove, it's not needed.
There was a problem hiding this comment.
This function reference is needed for the below design, otherwise getting build error.
Called StartApptask function from the main() which will create AppTaskMain task and the Event Queue.
The AppTaskMain will initializing the window application and run the event handler loop.Removed the redundant task exist before and now having only one task and the flow is similar for both WiFi and Thread.
The design flow is similar to the Light and door-lock apps except the base application class reference.
There was a problem hiding this comment.
@jepenven-silabs In main function calling startAppTask() through window object called "app" hence we declare and define virtual startAppTask() in windowApp.h and windowApp.cpp file respectively
If removed it, will get linking error
| @@ -36,6 +36,8 @@ class WindowAppImpl : public WindowApp | |||
| static WindowAppImpl sInstance; | |||
|
|
|||
| WindowAppImpl(); | |||
| CHIP_ERROR StartAppTask(void); | |||
There was a problem hiding this comment.
Please remove both as it is not needed.
There was a problem hiding this comment.
Called StartApptask function from the main() which will create AppTaskMain task and the Event Queue.
The AppTaskMain will initializing the window application and run the event handler loop.Removed the redundant task exist before and now having only one task and the flow is similar for both WiFi and Thread.
The design flow is similar to the Light and door-lock apps except the base application class reference.
| #ifdef DISPLAY_ENABLED | ||
| #include <LcdPainter.h> | ||
| SilabsLCD slLCD; | ||
| #endif | ||
|
|
||
| #define APP_TASK_STACK_SIZE (4096) | ||
| #define APP_MAIN_TASK_SIZE (APP_TASK_STACK_SIZE + 2048) |
There was a problem hiding this comment.
The main task is already defined. no need to create a new one.
| @@ -125,7 +121,9 @@ void WindowAppImpl::Timer::TimerCallback(TimerHandle_t xTimer) | |||
| //------------------------------------------------------------------------------ | |||
|
|
|||
| StackType_t sAppStack[APP_TASK_STACK_SIZE / sizeof(StackType_t)]; | |||
| StackType_t sAppMainStack[APP_MAIN_TASK_SIZE / sizeof(StackType_t)]; | |||
There was a problem hiding this comment.
Please remove duplication
There was a problem hiding this comment.
removed duplication
| StaticTask_t sAppTaskStruct; | ||
| StaticTask_t sAppMainTaskStruct; |
There was a problem hiding this comment.
Please remove duplication
There was a problem hiding this comment.
removed duplication
| @@ -156,38 +154,30 @@ void WindowAppImpl::OnIconTimeout(WindowApp::Timer & timer) | |||
| #endif | |||
| } | |||
|
|
|||
| CHIP_ERROR WindowAppImpl::Init() | |||
| CHIP_ERROR WindowAppImpl::StartAppTask() | |||
There was a problem hiding this comment.
please revert these changes
There was a problem hiding this comment.
Called StartApptask function from the main() which will create AppTaskMain task and the Event Queue.
The AppTaskMain will initializing the window application and run the event handler loop.Removed the redundant task exist before and now having only one task and the flow is similar for both WiFi and Thread.
The design flow is similar to the Light and door-lock apps except the base application class reference.
| @@ -57,10 +57,9 @@ int main(void) | |||
| chip::DeviceLayer::SetDeviceInfoProvider(&gExampleDeviceInfoProvider); | |||
|
|
|||
| WindowApp & app = WindowApp::Instance(); | |||
|
|
|||
| EFR32_LOG("Starting App"); | |||
There was a problem hiding this comment.
Please revert changes on this files.
There was a problem hiding this comment.
Called StartApptask function from the main() which will create AppTaskMain task and the Event Queue.
The AppTaskMain will initializing the window application and run the event handler loop.Removed the redundant task exist before and now having only one task and the flow is similar for both WiFi and Thread.
The design flow is similar to the Light and door-lock apps except the base application class reference.
There was a problem hiding this comment.
Like I said, in order to preserve the abstraction layer of this app, please remove ALL AppTask reference accross ALL files.
As stated by @rcasallas-silabs #22182 (comment)
| chip::app::Clusters::NetworkCommissioning::Instance | ||
| sWiFiNetworkCommissioningInstance(0 /* Endpoint Id */, &(chip::DeviceLayer::NetworkCommissioning::SlWiFiDriver::GetInstance())); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
Add this to the windowapp/efr32/src/windowAppImpl.cpp folder not in the common folder
jepenven-silabs
changed the title
| /* We will init server when we get IP */ | ||
| sWiFiNetworkCommissioningInstance.Init(); | ||
| #endif | ||
|
|
There was a problem hiding this comment.
This is platform-dependent, so it belongs to WindowAppImpl. Since you're using vTaskDelay(), it looks like it should be placed in WindowAppImpl::Sart(). I think that should avoid most of the changes.
There was a problem hiding this comment.
placed in WindowAppImpl::Sart()
|
Accepted for 1.0 changes: this is a delta in |
pullapprove
bot
requested review from
anush-apple,
arkq,
Byungjoo-Lee,
bzbarsky-apple,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi and
electrocucaracha
pullapprove
bot
requested review from
gjc13,
harsha-rajendran,
hawk248,
isiu-apple,
jelderton,
jmartinez-silabs,
jtung-apple,
kghost,
lazarkov,
LuDuda,
mlepage-google,
mrjerryjohns,
msandstedt,
mspang,
robszewczyk,
saurabhst,
selissia,
tecimovic,
tehampson,
turon,
vijs,
vivien-apple,
wbschiller,
woody-apple,
xylophone21 and
yufengwangca
| @@ -109,6 +109,11 @@ WindowApp::Cover * WindowApp::GetCover(chip::EndpointId endpoint) | |||
| return nullptr; | |||
| } | |||
|
|
|||
| CHIP_ERROR WindowApp::StartAppTask() | |||
There was a problem hiding this comment.
Please remove this completely.
| // Initialize App Task | ||
| mHandle = xTaskCreateStatic(OnTaskCallback, APP_TASK_NAME, ArraySize(sAppStack), NULL, 1, sAppStack, &sAppTaskStruct); |
There was a problem hiding this comment.
This shouldn't be removed please put it back
| @@ -57,10 +57,9 @@ int main(void) | |||
| chip::DeviceLayer::SetDeviceInfoProvider(&gExampleDeviceInfoProvider); | |||
|
|
|||
| WindowApp & app = WindowApp::Instance(); | |||
|
|
|||
| EFR32_LOG("Starting App"); | |||
There was a problem hiding this comment.
Like I said, in order to preserve the abstraction layer of this app, please remove ALL AppTask reference accross ALL files.
As stated by @rcasallas-silabs #22182 (comment)
|
Closed with duplicate : #22451 |
Created two PRs for this
This PR consists of below App level changes
Testing
Perform core cluster test cases
Perform sanity test cases
Perform all test cases which is required cluster for window app