From 55a141124f60c951af4f60d71b7b80ad4413696f Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 24 Jun 2025 14:23:18 -0400 Subject: [PATCH 1/6] do reconfigure manually --- src/viam/sdk/module/service.cpp | 8 +++----- src/viam/sdk/resource/reconfigurable.cpp | 9 --------- src/viam/sdk/resource/reconfigurable.hpp | 8 -------- 3 files changed, 3 insertions(+), 22 deletions(-) diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index a815a07bd..f462c1963 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -116,12 +116,10 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { "unable to reconfigure resource " + cfg.resource_name().name() + " as it doesn't exist."); } - try { - Reconfigurable::reconfigure_if_reconfigurable(res, deps, cfg); - res->set_log_level(cfg.get_log_level()); + + if (auto reconfigurable = std::dynamic_pointer_cast(res)) { + reconfigurable->reconfigure(deps, cfg); return grpc::Status(); - } catch (const std::exception& exc) { - return grpc::Status(::grpc::INTERNAL, exc.what()); } // if the type isn't reconfigurable by default, replace it diff --git a/src/viam/sdk/resource/reconfigurable.cpp b/src/viam/sdk/resource/reconfigurable.cpp index 3f65ba516..f55be7b32 100644 --- a/src/viam/sdk/resource/reconfigurable.cpp +++ b/src/viam/sdk/resource/reconfigurable.cpp @@ -8,14 +8,5 @@ namespace sdk { Reconfigurable::~Reconfigurable() = default; Reconfigurable::Reconfigurable() = default; -void Reconfigurable::reconfigure_if_reconfigurable(const std::shared_ptr& resource, - const Dependencies& deps, - const ResourceConfig& cfg) { - auto reconfigurable_res = std::dynamic_pointer_cast(resource); - if (reconfigurable_res) { - reconfigurable_res->reconfigure(deps, cfg); - } -} - } // namespace sdk } // namespace viam diff --git a/src/viam/sdk/resource/reconfigurable.hpp b/src/viam/sdk/resource/reconfigurable.hpp index 013bf10e1..1c85b62dd 100644 --- a/src/viam/sdk/resource/reconfigurable.hpp +++ b/src/viam/sdk/resource/reconfigurable.hpp @@ -15,14 +15,6 @@ class Reconfigurable { /// @param cfg The resource's config. virtual void reconfigure(const Dependencies& deps, const ResourceConfig& cfg) = 0; - /// @brief Reconfigures a resource if it is Reconfigurable. - /// @param resource the Resource to reconfigure. - /// @param deps Dependencies of the resource. - /// @param cfg The resource's config. - static void reconfigure_if_reconfigurable(const std::shared_ptr& resource, - const Dependencies& deps, - const ResourceConfig& cfg); - protected: explicit Reconfigurable(); }; From bebbbf56fc340bae12d723468b2384a2b333e289 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Tue, 24 Jun 2025 14:26:33 -0400 Subject: [PATCH 2/6] remove redundant at calls --- src/viam/sdk/resource/resource_manager.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/viam/sdk/resource/resource_manager.cpp b/src/viam/sdk/resource/resource_manager.cpp index 40dc64ece..b11d9b61a 100644 --- a/src/viam/sdk/resource/resource_manager.cpp +++ b/src/viam/sdk/resource/resource_manager.cpp @@ -24,14 +24,16 @@ namespace sdk { std::shared_ptr ResourceManager::resource(const std::string& name) { const std::lock_guard lock(lock_); - if (resources_.find(name) != resources_.end()) { - return resources_.at(name); + auto res_it = resources_.find(name); + if (res_it != resources_.end()) { + return res_it->second; } - if (short_names_.find(name) != short_names_.end()) { - const std::string short_name = short_names_.at(name); - if (resources_.find(short_name) != resources_.end()) { - return resources_.at(short_name); + auto name_it = short_names_.find(name); + if (name_it != short_names_.end()) { + res_it = resources_.find(name_it->second); + if (res_it != resources_.end()) { + return res_it->second; } } From d04368a87e7c5d07c44faf4b75e9f802652b603f Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 26 Jun 2025 11:37:26 -0400 Subject: [PATCH 3/6] restore deleted log level call --- src/viam/sdk/module/service.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index f462c1963..bb2993a3b 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -119,6 +119,7 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { if (auto reconfigurable = std::dynamic_pointer_cast(res)) { reconfigurable->reconfigure(deps, cfg); + res->set_log_level(cfg.get_log_level()); return grpc::Status(); } From 4358c2902c039dc89a8a149b873c59e4433d057f Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 26 Jun 2025 11:53:44 -0400 Subject: [PATCH 4/6] remove stop if stoppable --- src/viam/sdk/module/service.cpp | 6 ++---- src/viam/sdk/resource/stoppable.cpp | 8 -------- src/viam/sdk/resource/stoppable.hpp | 12 ------------ 3 files changed, 2 insertions(+), 24 deletions(-) diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index bb2993a3b..482ec6fdd 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -124,10 +124,8 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { } // if the type isn't reconfigurable by default, replace it - try { - Stoppable::stop_if_stoppable(res); - } catch (const std::exception& err) { - VIAM_SDK_LOG(error) << "unable to stop resource: " << err.what(); + if (auto stoppable = std::dynamic_pointer_cast(res)) { + stoppable->stop(); } const std::shared_ptr reg = diff --git a/src/viam/sdk/resource/stoppable.cpp b/src/viam/sdk/resource/stoppable.cpp index 4e4c7165e..b34fda57f 100644 --- a/src/viam/sdk/resource/stoppable.cpp +++ b/src/viam/sdk/resource/stoppable.cpp @@ -6,13 +6,5 @@ namespace sdk { Stoppable::~Stoppable() = default; Stoppable::Stoppable() = default; -void Stoppable::stop_if_stoppable(const std::shared_ptr& resource, - const ProtoStruct& extra) { - auto stoppable_res = std::dynamic_pointer_cast(resource); - if (stoppable_res) { - stoppable_res->stop(extra); - } -} - } // namespace sdk } // namespace viam diff --git a/src/viam/sdk/resource/stoppable.hpp b/src/viam/sdk/resource/stoppable.hpp index 58d29f70a..d1b9c4165 100644 --- a/src/viam/sdk/resource/stoppable.hpp +++ b/src/viam/sdk/resource/stoppable.hpp @@ -17,18 +17,6 @@ class Stoppable { return stop({}); } - /// @brief Stops a Resource if it is Stoppable. - /// @param resource The Resource to stop. - /// @param extra Extra arguments to pass to the resource's `stop` method. - static void stop_if_stoppable(const std::shared_ptr& resource, - const ProtoStruct& extra); - - /// @brief Stops a Resource if it is Stoppable. - /// @param resource The Resource to stop. - inline static void stop_if_stoppable(const std::shared_ptr& resource) { - return stop_if_stoppable(resource, {}); - } - protected: explicit Stoppable(); }; From 538e6e02939dbe4e7ff2c9cb6e855712b80d38e2 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 26 Jun 2025 11:59:52 -0400 Subject: [PATCH 5/6] todo comment --- src/viam/sdk/module/service.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index 482ec6fdd..5acd00a37 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -130,6 +130,8 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { const std::shared_ptr reg = Registry::get().lookup_model(cfg.name()); + + // TODO RSDK-11067 new resource gets constructed while old one is still alive. if (reg) { try { const std::shared_ptr resource = reg->construct_resource(deps, cfg); From 9f6b8b5093c482d9728e213888e8e7c5c288f651 Mon Sep 17 00:00:00 2001 From: Lia Stratopoulos <167905060+lia-viam@users.noreply.github.com> Date: Thu, 26 Jun 2025 14:50:19 -0400 Subject: [PATCH 6/6] replace remaining instances of stop if stoppable --- src/viam/sdk/module/service.cpp | 6 ++--- src/viam/sdk/tests/mocks/mock_robot.cpp | 34 +++++++++++++------------ 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index 5acd00a37..658630484 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -185,10 +185,8 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service { "unable to remove resource " + name.to_string() + " as it doesn't exist."); } - try { - Stoppable::stop_if_stoppable(res); - } catch (const std::exception& err) { - VIAM_SDK_LOG(error) << "unable to stop resource: " << err.what(); + if (auto stoppable = std::dynamic_pointer_cast(res)) { + stoppable->stop(); } manager->remove(name); diff --git a/src/viam/sdk/tests/mocks/mock_robot.cpp b/src/viam/sdk/tests/mocks/mock_robot.cpp index 5522c91cf..0b9e0b592 100644 --- a/src/viam/sdk/tests/mocks/mock_robot.cpp +++ b/src/viam/sdk/tests/mocks/mock_robot.cpp @@ -252,31 +252,33 @@ ::grpc::Status MockRobotService::StopAll(::grpc::ServerContext*, const std::shared_ptr resource = r.second; const ResourceName rn = to_proto(resource->get_resource_name()); const std::string rn_ = rn.SerializeAsString(); - if (extra.find(rn_) != extra.end()) { - try { - Stoppable::stop_if_stoppable(resource, extra.at(rn_)); - } catch (const std::runtime_error& err) { + + auto stop_without_extra = [&status_message, + &status](const std::shared_ptr& resource) { + if (auto stoppable = std::dynamic_pointer_cast(resource)) { try { + stoppable->stop(); + return; + } catch (const std::runtime_error& err) { status_message = err.what(); - Stoppable::stop_if_stoppable(resource); - } catch (std::runtime_error& err) { - status_message = err.what(); - status = grpc::UNKNOWN; } catch (...) { status_message = "unknown error"; - status = grpc::UNKNOWN; } + status = grpc::UNKNOWN; } - } else { + }; + + if (extra.find(rn_) != extra.end()) { try { - Stoppable::stop_if_stoppable(resource); - } catch (std::runtime_error& err) { + if (auto stoppable = std::dynamic_pointer_cast(resource)) { + stoppable->stop(extra.at(rn_)); + } + } catch (const std::runtime_error& err) { status_message = err.what(); - status = grpc::UNKNOWN; - } catch (...) { - status_message = "unknown error"; - status = grpc::UNKNOWN; + stop_without_extra(resource); } + } else { + stop_without_extra(resource); } }