watcher started only when needed - #4369
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refines OVMS model monitoring so the watcher thread starts only when required (config file monitoring and/or filesystem version polling), and gates version polling work behind a dynamically re-evaluated versionPollingNeeded flag.
Changes:
- Introduces
versionPollingNeededandevaluatePollingState()to decide whether filesystem version polling is required and whether to start the watcher. - Updates the watcher loop to run
updateConfigurationWithoutConfigFile()only when version polling is needed, and re-evaluates polling state after config reloads. - Updates a test JSON config to explicitly set
target_deviceto CPU.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/test/configs/config_string.json | Adds target_device: CPU to the string passthrough test config. |
| src/modelmanager.hpp | Adds versionPollingNeeded state and declares evaluatePollingState(). |
| src/modelmanager.cpp | Implements polling-state evaluation and gates watcher/version polling behavior accordingly. |
| /** | ||
| * @brief Evaluates whether filesystem version polling is needed | ||
| * and starts the watcher thread if at least one model uses version directories | ||
| */ | ||
| void evaluatePollingState(); |
| } else { | ||
| SPDLOG_LOGGER_DEBUG(modelmanager_logger, "Filesystem version polling disabled - no model uses version directories"); | ||
| } |
| configFileReloadNeeded(isNeeded); | ||
| if (isNeeded) { | ||
| loadConfig(); | ||
| evaluatePollingState(); | ||
| } |
| void ModelManager::evaluatePollingState() { | ||
| static const std::set<std::string> singleFileExtensions = { | ||
| ".xml", ".onnx", ".pdmodel", ".pdiparams", ".pb", ".tflite"}; | ||
|
|
||
| versionPollingNeeded = false; |
| static const std::set<std::string> singleFileExtensions = { | ||
| ".xml", ".onnx", ".pdmodel", ".pdiparams", ".pb", ".tflite"}; |
|
Could you add unit tests for this change? |
| const std::string& basePath = config.getBasePath(); | ||
| auto ext = std::filesystem::path(basePath).extension().string(); | ||
| if (singleFileExtensions.count(ext) == 0) { | ||
| SPDLOG_LOGGER_TRACE(modelmanager_logger, "Model {} with path {} requires version polling", name, basePath); |
There was a problem hiding this comment.
I would put it on debug or even info if this is a one time call during model loading phase
| SPDLOG_LOGGER_DEBUG(modelmanager_logger, "Filesystem version polling enabled - at least one model uses version directories"); | ||
| } else { | ||
| SPDLOG_LOGGER_DEBUG(modelmanager_logger, "Filesystem version polling disabled - no model uses version directories"); | ||
| } | ||
| startWatcher(isStartedWithConfigFile()); | ||
| } else { | ||
| SPDLOG_LOGGER_DEBUG(modelmanager_logger, "Filesystem version polling disabled - no model uses version directories"); |
There was a problem hiding this comment.
maybe it could be on INFO level logging
| } | ||
| } | ||
|
|
||
| void ModelManager::evaluatePollingState() { |
There was a problem hiding this comment.
The name of this method hides the fact that it may launch watcher thread. I would either rename it or break it in two, so we have evaluate method that returns information if we need to start watcher or not and the second one that starts it.
| configFileReloadNeeded(isNeeded); | ||
| if (isNeeded) { | ||
| loadConfig(); | ||
| evaluatePollingState(); |
There was a problem hiding this comment.
In it's current implementation, that method can start a watcher thread. Does not look like something that should be run in a while loop.
🛠 Summary
Now the design is:
versionPollingNeeded — member variable re-evaluated by evaluatePollingState() after every config load
Watcher starts if either config file monitoring is needed OR version polling is needed
updateConfigurationWithoutConfigFile() only runs when versionPollingNeeded is true
After each loadConfig(), evaluatePollingState() is called again — so if the new config adds a versioned model, version polling activates dynamically
🧪 Checklist
``