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

[OpenThread] Layering inversion in OpenThread platforms #30596

Closed
andy31415 opened this issue Nov 21, 2023 · 2 comments · Fixed by #30607
Closed

[OpenThread] Layering inversion in OpenThread platforms #30596

andy31415 opened this issue Nov 21, 2023 · 2 comments · Fixed by #30607
Assignees

Comments

@andy31415
Copy link
Contributor

Seems to be since #19474:

When CHIP_SYSTEM_CONFIG_USE_OPEN_THREAD_ENDPOINT is enabled, we have src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp including src/app/Server.h.

The reason it does that is so it runs:

Server::GetInstance().RejoinExistingMulticastGroups();

This makes platform (one of the lowest level things in our code) depend on app (which is the top level thing). We should instead switch to some sort of event system or figure out how to move this RejoinExistingMulticastGroups around to fix dependency.

Sideffect of this is that things like ${chip_root}/src/lib/address_resolve:default_address_resolve_config has to be added to every thread platform (because Server needs address resolution) and we do not want that.

@andy31415
Copy link
Contributor Author

src/platform/nxp/common/ConfigurationManagerImpl.cpp also seems to have #include <app/server/Server.h> but that may be unrelated ... seems to be the only platform doing that

@andy31415
Copy link
Contributor Author

❯ rg 'app/server/Server.h' src/platform/
src/platform/nxp/common/ConfigurationManagerImpl.cpp
30:#include <app/server/Server.h>
                                                                                                                                                                                                                                                                               
src/platform/silabs/efr32/wifi/wfx_notify.cpp
44:#include <app/server/Server.h>
                                                                                                                                                                                                                                                                               
src/platform/silabs/SiWx917/wifi/wfx_notify.cpp
37:#include <app/server/Server.h>
                                                                                                                                                                                                                                                                               
src/platform/OpenThread/GenericThreadStackManagerImpl_OpenThread.hpp
69:#include <app/server/Server.h>

These seem to be the wrong inlcudes. OpenThread.hpp needs to be fixed, the others may need case-by-case handling.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants