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

Restructure MQTT config distribution #234

Merged
merged 11 commits into from
Jan 31, 2025
Merged

Conversation

hikinggrass
Copy link
Contributor

  • publish manifests individually on retained topics instead in one big blob
  • change module/impl config entry format to include its type, this removes the need for the whole config schema just to determine the type
  • reorder the config handler before the ready handler in manager to reflect the order of operations taken during initialization of a module
  • smaller optimizations

The same functionality can be achieved without first including the main config in the serialized config and then deleting it...

Signed-off-by: Kai-Uwe Hermann <[email protected]>

# Conflicts:
#	src/manager.cpp

Signed-off-by: Kai-Uwe Hermann <[email protected]>
Skip storing it in a temporary variable

Signed-off-by: Kai-Uwe Hermann <[email protected]>
…etain

Previously this was sent to every module individually which isn't necessary

Signed-off-by: Kai-Uwe Hermann <[email protected]>
Add type to parsed_config_map and remove config entry from manifest sent via MQTT

Signed-off-by: Kai-Uwe Hermann <[email protected]>
Signed-off-by: Kai-Uwe Hermann <[email protected]>
@hikinggrass hikinggrass requested a review from a-w50 January 8, 2025 11:44
Signed-off-by: Kai-Uwe Hermann <[email protected]>
…qtt-config

# Conflicts:
#	src/manager.cpp

Signed-off-by: Kai-Uwe Hermann <[email protected]>
@corneliusclaussen
Copy link
Contributor

retained topics: Could this lead to potential left-over problems of previous EVerest runs? In the default mosquitto config it is even stored permanently surviving reboots. Is it ensured that there can't be race conditions where e.g. one module subscribes to mqtt before the manager publishes the new config?

@hikinggrass
Copy link
Contributor Author

hikinggrass commented Jan 20, 2025

retained topics: Could this lead to potential left-over problems of previous EVerest runs? In the default mosquitto config it is even stored permanently surviving reboots.

Yes, and this is partially addressed with #233 (at least for successful starts, there is be more cleanup needed in cases the manager exits early, I'll address this over there). However the retained topics behavior is already in v0.19 and v0.19.1, so #233 is already an improvement as is

Is it ensured that there can't be race conditions where e.g. one module subscribes to mqtt before the manager publishes the new config?

For configs this should not be an issue since a module explicitly requests a config from the manager and this doesn't involve retained topics. For manifest/interfaces/etc. there may be scenarios where this is a problem. It seems pretty stable thought (eg. I run the ocpp-tests of everest-core in parallel with all cores of my machine reliably all the time) but I will investigate this a bit further

@corneliusclaussen
Copy link
Contributor

just a random thought: would it make sense to clear those topics as a last will from the manager?

@hikinggrass
Copy link
Contributor Author

just a random thought: would it make sense to clear those topics as a last will from the manager?

yes, sounds like a good idea. I plan on opening a PR with some signal handling for the manager that could then trigger this as well. During normal operation #233 will clean them up after every module is ready

@hikinggrass hikinggrass requested review from corneliusclaussen and removed request for a-w50 January 29, 2025 13:10
@hikinggrass hikinggrass merged commit 26e31ac into main Jan 31, 2025
5 checks passed
@hikinggrass hikinggrass deleted the feature/restructure-mqtt-config branch January 31, 2025 10:29
hikinggrass added a commit that referenced this pull request Feb 6, 2025
#234 changed the format of the config passed to a module which wasn't properly implemented in everestpy

Signed-off-by: Kai-Uwe Hermann <[email protected]>
hikinggrass added a commit that referenced this pull request Feb 6, 2025
* Fix config parsing for everestpy

#234 changed the format of the config passed to a module which wasn't properly implemented in everestpy

* refactor duplicate code in everestjs and everestpy

* Bump everest version to 0.20.2

---------

Signed-off-by: Kai-Uwe Hermann <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants