-
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
Move diagnostic operations from ConnectivityMgr to DiagnosticDataProvider #12139
Move diagnostic operations from ConnectivityMgr to DiagnosticDataProvider #12139
Conversation
PR #12139: Size comparison from b83502b to 83ca866 Increases above 0.2%:
Increases (8 builds for k32w, p6, qpg, telink)
Full report (9 builds for k32w, p6, qpg, telink)
|
PR #12139: Size comparison from b83502b to 5d03400 Increases above 0.2%:
Increases (33 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (8 builds for linux)
Full report (36 builds for efr32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
Switching from CRTP to virtual interfaces comes at a cost of increased flash usage which may really become a blocker at some point. It is because, the linker can't easily optimize out unused functions as they are still referenced by a vtable. In my opinion, if the direction is to use virtual interfaces we should keep the interfaces small so that platforms may choose which interfaces they want to implement. For example, I think we should have separate |
PR #12139: Size comparison from 71a3fb2 to 9644f00 Increases above 0.2%:
Increases (16 builds for k32w, linux, p6, qpg, telink)
Decreases (8 builds for linux)
Full report (17 builds for k32w, linux, p6, qpg, telink)
|
@tcarmelveilleux @bzbarsky-apple The diagnostic APIs are complete new, keep adding tons of methods to PlatformManager/ConnectivityManager will make it hard to override and cause system dependencies against all drivers in those two classes. And we will have various delegate interfaces to be added later to support event. Obviously, current route is not scale-able and very difficult to maintain. After discussion, we would like to separate this out from giant PlatformManager/ConnectivityManager classes with the cost of virtual interface just for diagnostic operations. I agree with your proposal to split it further, but we have more than three diagnostic clusters now, log, general, software, wifi, ethernet, thread, make all of them separate? |
PR #12139: Size comparison from 2490197 to e0e2bb8 Increases above 0.2%:
Increases (35 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
Decreases (10 builds for esp32, linux)
Full report (38 builds for efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
@yufengwangca I'm not sure if |
Maybe one solution is to split it to following sub-interfaces in the follow-ups:
|
Problem
What is being fixed? Examples:
Change overview
Testing
How was this tested? (at least one bullet point required)