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 app configuration to enable/disable IM write #7791

Closed
bzbarsky-apple opened this issue Jun 21, 2021 · 6 comments
Closed

Add app configuration to enable/disable IM write #7791

bzbarsky-apple opened this issue Jun 21, 2021 · 6 comments
Assignees
Labels
Interaction Model Work maybe done? Isssue may be complete/obsolete/not relevant anymore. Needs review to determine if to close or not. V1.0

Comments

@bzbarsky-apple
Copy link
Contributor

Problem

After #7730 the code for WriteClient is included in all apps, including those that never issue any writes. This seems to be because a lot of that code is driven from WriteClient::OnMessageReceived, which is a virtual function (from ExchangeContext), hence always included by the linker and then it pulls in all the rest.

Proposed Solution

Since we can't rely on the linker to strip this out, we may want to decide whether to include WriteClient based on the app configuration. If the app does not include the client sides of any clusters that have writable attributes, it should not be including anything related to WriteClient. Other ideas welcome!

@yunhanw-google @andy31415

@bzbarsky-apple
Copy link
Contributor Author

To be clear, this is ~1KB of codesize, if I am measuring it right, that we don't need to be paying for a lot of constrained apps.

@yunhanw-google
Copy link
Contributor

maybe we can introduce the feature flag so that we don't include it in constrained apps?

@bzbarsky-apple
Copy link
Contributor Author

Right, that is my proposal: a feature flag, and then separately work on auto-deducing it at codegen time.

@yunhanw-google yunhanw-google changed the title WriteClient code cost is paid by all apps Add app configuration to enable/disable IM write Jul 14, 2021
@stale
Copy link

stale bot commented Jan 26, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale Stale issue or PR label Jan 26, 2022
@bzbarsky-apple bzbarsky-apple removed the stale Stale issue or PR label Jan 26, 2022
@tcarmelveilleux
Copy link
Contributor

@bzbarsky-apple Is this done?

@tcarmelveilleux tcarmelveilleux added maybe done? Isssue may be complete/obsolete/not relevant anymore. Needs review to determine if to close or not. and removed v1_triage_split_3 labels Jan 31, 2022
@bzbarsky-apple
Copy link
Contributor Author

Looks fixed to me, by getting rid of the WriteClient pool. Now only apps that actually create a WriteClient pay the cost.

In particular, the .map for the esp32 version of chip-all-clusters-app does not mention WriteClient.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Interaction Model Work maybe done? Isssue may be complete/obsolete/not relevant anymore. Needs review to determine if to close or not. V1.0
Projects
Development

No branches or pull requests

6 participants