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.

Sign up for GitHub

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

Jump to bottom

Fixed Window App Refactor #2

Merged
merged 1 commit into from
Jun 20, 2023
Merged

Fixed Window App Refactor #2

merged 1 commit into from
Jun 20, 2023

Conversation

andygallay-silabs
Copy link
Owner

New cleaned PR after solving merge issue.

@andygallay-silabs andygallay-silabs changed the title Window app rework Fixed Window App Refactor Jun 12, 2023
jepenven-silabs
jepenven-silabs requested changes Jun 13, 2023
View reviewed changes
Copy link
Collaborator

@jepenven-silabs jepenven-silabs left a comment

Choose a reason for hiding this comment

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

I feel like the cleanup is not complete. There is a lot of useless defines, methods that are never called, unnecessary wrapped that can cause code bloats... It definitively needs a second pass. I did not commented on everything that needs to be changed since there is still too much to fix.

examples/lighting-app/silabs/include/AppEvent.h Outdated
@@ -22,6 +22,11 @@
struct AppEvent;
typedef void (*EventHandler)(AppEvent *);

#include <app/clusters/window-covering-server/window-covering-server.h>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This files should not be modified. Please remove it from this PR.

examples/window-app/silabs/efr32/data_model/BUILD.gn Outdated
@@ -0,0 +1,23 @@
# Copyright (c) 2020 Project CHIP Authors
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please move this out of the efr32 folder and delete it. all path should now have only /examples_name/silabs/

examples/window-app/silabs/SiWx917/args.gni
@@ -20,5 +20,3 @@ silabs_sdk_target = get_label_info(":sdk", "label_no_toolchain")

Copy link
Collaborator

Choose a reason for hiding this comment

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

The complete SiWx917 folder should be deleted

examples/window-app/silabs/efr32/data_model/window-app.matter Outdated
// This IDL was generated automatically by ZAP.
// It is for view/code review purposes only.

struct ApplicationStruct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above

examples/window-app/silabs/efr32/data_model/window-app.zap Outdated
@@ -0,0 +1,9350 @@
{
"featureLevel": 96,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as above

examples/window-app/silabs/src/WindowManager.cpp Outdated

#include <platform/silabs/platformAbstraction/SilabsPlatform.h>

#define EXAMPLE_VENDOR_ID 0xcafe
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be removed

examples/window-app/silabs/src/WindowManager.cpp
AppTask::GetAppTask().PostEvent(&event);
}

void WindowManager::Timer::Start()
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm not mistaken the timer is actually used twice. and we seems to only use a single timer. That doesn't seems to justify all of this wrapping. Please removed appropriately

examples/window-app/silabs/src/WindowManager.cpp Outdated
}
}

void WindowManager::Button::Press()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removed these method and add them directly in the handler

examples/window-app/silabs/include/WindowManager.h Outdated Show resolved Hide resolved
examples/window-app/silabs/src/WindowManager.cpp Outdated
}
}

void WindowManager::Timer::IsrStart()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be removed

andygallay-silabs added a commit that referenced this pull request Jun 14, 2023
@andygallay-silabs
Revisions based on jepenven comments on PR #2
b285b3a
andygallay-silabs added a commit that referenced this pull request Jun 19, 2023
@andygallay-silabs
First commit with buildable window-app.
e02f99c 
Merged WindowApp and WindowAppImpl into WindowManager.

Buildable WindowApp example from AppTask. Needs debug.

Update AppTask.cpp

Working refactor for Window App example with common AppTask

Clean up #1 for AppTask

Clean up #2 for AppTask

Refactor Window-app
@andygallay-silabs andygallay-silabs force-pushed the window-app-rework branch 2 times, most recently from e02f99c to 4445bb5 Compare June 19, 2023 19:48
@github-actions github-actions bot removed the scripts label Jun 19, 2023
jepenven-silabs
jepenven-silabs reviewed Jun 20, 2023
View reviewed changes
examples/window-app/silabs/include/WindowManager.h
WindowManager();

CHIP_ERROR Init();
CHIP_ERROR UpdateState();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This complete method should be deleted. It's duplication from the base App

@jepenven-silabs jepenven-silabs merged commit f3112fb into silabs_window_app_refactor_fix Jun 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jepenven-silabs jepenven-silabs jepenven-silabs approved these changes

Assignees
No one assigned
Labels
examples platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@andygallay-silabs @jepenven-silabs