diff --git a/src/viam/sdk/module/service.cpp b/src/viam/sdk/module/service.cpp index a815a07bd..658630484 100644 --- a/src/viam/sdk/module/service.cpp +++ b/src/viam/sdk/module/service.cpp @@ -116,23 +116,22 @@ 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); + + if (auto reconfigurable = std::dynamic_pointer_cast(res)) { + reconfigurable->reconfigure(deps, cfg); res->set_log_level(cfg.get_log_level()); 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 - 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 = 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); @@ -186,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/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(); }; 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; } } 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(); }; 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); } }