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

Config restructure: distribute module configs via MQTT #201

Merged
merged 125 commits into from
Dec 6, 2024

Conversation

hikinggrass
Copy link
Contributor

@hikinggrass hikinggrass commented Aug 19, 2024

Changes to reduce configuration parsing effort by parsing it once by the manager and then distribute it to the modules via MQTT.

This is achieved by first parsing the MQTT settings from the config, passing these to the modules whilst spawning them. The modules themselves then ask for their module config via MQTT, which is in turn provided to them from the manager.

schemas, manifests, types, interfaces, error definitions are published as retained topics that modules can access when needed.

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
get logging config and mqtt settings from command line or environment variables

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Retain these topics so the modules can get then if they need them

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…nd MQTTAbstraction ctors

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass changed the title Cconfig restructure: distribute module configs via MQTT Config restructure: distribute module configs via MQTT Aug 19, 2024
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass marked this pull request as ready for review August 20, 2024 08:48
Fix bug in has_extension function

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
This is identical to how it's done for interfaces to keep message sizes as small as possible

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Move this configuration to tests/CMakeLists.txt

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
This checks if broker_socket_path is set

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…cture

# Conflicts:
#	include/utils/config.hpp
#	lib/config.cpp
#	lib/everest.cpp

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
This prevents everest-testing from breaking until it is refactored to use the new ctor + environment variables

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass requested a review from a-w50 December 2, 2024 10:32
a-w50 and others added 10 commits December 2, 2024 23:40
Signed-off-by: aw <aw@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Set coverage flags directly on the appropriate targets
To make this work from the tests directory set CMake policy CMP0079

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Don't load the same yaml file twice

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…cture

# Conflicts:
#	include/utils/message_queue.hpp
#	lib/everest.cpp
#	lib/message_queue.cpp
#	lib/mqtt_abstraction_impl.cpp
#	src/manager.cpp

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
…feature

Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
CMakeLists.txt Outdated
Comment on lines 33 to 37
# this policy allows us to link gcov to targets defined in other directories
if(POLICY CMP0079)
cmake_policy(SET CMP0079 NEW)
set(CMAKE_POLICY_DEFAULT_CMP0079 NEW)
endif()
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't need this, as the required cmake version is already > 3.13.
Furthermore, set(CMAKE_POLICY_DEFAULT_CMP0079 NEW) already enforces cmake_policy(SET CMP0079 NEW), so only the latter would be necessary. I also don't get the point of if(POLICY CMP0079)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm also very confused why this is needed, since it should be the default. However on CMake 3.31 I get a

Attempt to add link library "gcov" to target "manager" which is not built
  in this directory.

  This is allowed only when policy CMP0079 is set to NEW.
Call Stack (most recent call first):
  tests/CMakeLists.txt:69 (append_coverage_compiler_flags_to_target)

error as if that policy isn't set.
the if(POLICY CMP0079) just ensures that the policy is available before setting it, maybe not necessary on CMake >= 3.13

lib/everest.cpp Outdated
@@ -42,7 +42,7 @@ Everest::Everest(std::string module_id_, const Config& config_, bool validate_da
std::shared_ptr<MQTTAbstraction> mqtt_abstraction, const std::string& telemetry_prefix,
bool telemetry_enabled) :
mqtt_abstraction(mqtt_abstraction),
config(std::move(config_)),
Copy link
Contributor

Choose a reason for hiding this comment

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

moving a const is indeed pointless. But it might be a better choice to make the constructor argument Config config_ instead of const ref and dropping the move.

lib/everest.cpp Outdated Show resolved Hide resolved
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
Signed-off-by: Kai-Uwe Hermann <kai-uwe.hermann@pionix.de>
@hikinggrass hikinggrass merged commit 72dec12 into main Dec 6, 2024
4 of 5 checks passed
@hikinggrass hikinggrass deleted the feature/config-restructure branch December 6, 2024 11:04
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 this pull request may close these issues.

2 participants