Skip to content

Commit f5ce69a

Browse files
lia-viamlia-viam
authored
RSDK-11050: Reconfigure fix (viamrobotics#457)
1 parent d7ac685 commit f5ce69a

File tree

7 files changed

+35
-71
lines changed

7 files changed

+35
-71
lines changed

‎src/viam/sdk/module/service.cpp‎

Lines changed: 9 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -116,23 +116,22 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service {
116116
"unable to reconfigure resource " + cfg.resource_name().name() +
117117
" as it doesn't exist.");
118118
}
119-
try {
120-
Reconfigurable::reconfigure_if_reconfigurable(res, deps, cfg);
119+
120+
if (auto reconfigurable = std::dynamic_pointer_cast<Reconfigurable>(res)) {
121+
reconfigurable->reconfigure(deps, cfg);
121122
res->set_log_level(cfg.get_log_level());
122123
return grpc::Status();
123-
} catch (const std::exception& exc) {
124-
return grpc::Status(::grpc::INTERNAL, exc.what());
125124
}
126125

127126
// if the type isn't reconfigurable by default, replace it
128-
try {
129-
Stoppable::stop_if_stoppable(res);
130-
} catch (const std::exception& err) {
131-
VIAM_SDK_LOG(error) << "unable to stop resource: " << err.what();
127+
if (auto stoppable = std::dynamic_pointer_cast<Stoppable>(res)) {
128+
stoppable->stop();
132129
}
133130

134131
const std::shared_ptr<const ModelRegistration> reg =
135132
Registry::get().lookup_model(cfg.name());
133+
134+
// TODO RSDK-11067 new resource gets constructed while old one is still alive.
136135
if (reg) {
137136
try {
138137
const std::shared_ptr<Resource> resource = reg->construct_resource(deps, cfg);
@@ -186,10 +185,8 @@ struct ModuleService::ServiceImpl : viam::module::v1::ModuleService::Service {
186185
"unable to remove resource " + name.to_string() + " as it doesn't exist.");
187186
}
188187

189-
try {
190-
Stoppable::stop_if_stoppable(res);
191-
} catch (const std::exception& err) {
192-
VIAM_SDK_LOG(error) << "unable to stop resource: " << err.what();
188+
if (auto stoppable = std::dynamic_pointer_cast<Stoppable>(res)) {
189+
stoppable->stop();
193190
}
194191

195192
manager->remove(name);

‎src/viam/sdk/resource/reconfigurable.cpp‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,5 @@ namespace sdk {
88
Reconfigurable::~Reconfigurable() = default;
99
Reconfigurable::Reconfigurable() = default;
1010

11-
void Reconfigurable::reconfigure_if_reconfigurable(const std::shared_ptr<Resource>& resource,
12-
const Dependencies& deps,
13-
const ResourceConfig& cfg) {
14-
auto reconfigurable_res = std::dynamic_pointer_cast<Reconfigurable>(resource);
15-
if (reconfigurable_res) {
16-
reconfigurable_res->reconfigure(deps, cfg);
17-
}
18-
}
19-
2011
} // namespace sdk
2112
} // namespace viam

‎src/viam/sdk/resource/reconfigurable.hpp‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,6 @@ class Reconfigurable {
1515
/// @param cfg The resource's config.
1616
virtual void reconfigure(const Dependencies& deps, const ResourceConfig& cfg) = 0;
1717

18-
/// @brief Reconfigures a resource if it is Reconfigurable.
19-
/// @param resource the Resource to reconfigure.
20-
/// @param deps Dependencies of the resource.
21-
/// @param cfg The resource's config.
22-
static void reconfigure_if_reconfigurable(const std::shared_ptr<Resource>& resource,
23-
const Dependencies& deps,
24-
const ResourceConfig& cfg);
25-
2618
protected:
2719
explicit Reconfigurable();
2820
};

‎src/viam/sdk/resource/resource_manager.cpp‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,14 +24,16 @@ namespace sdk {
2424
std::shared_ptr<Resource> ResourceManager::resource(const std::string& name) {
2525
const std::lock_guard<std::mutex> lock(lock_);
2626

27-
if (resources_.find(name) != resources_.end()) {
28-
return resources_.at(name);
27+
auto res_it = resources_.find(name);
28+
if (res_it != resources_.end()) {
29+
return res_it->second;
2930
}
3031

31-
if (short_names_.find(name) != short_names_.end()) {
32-
const std::string short_name = short_names_.at(name);
33-
if (resources_.find(short_name) != resources_.end()) {
34-
return resources_.at(short_name);
32+
auto name_it = short_names_.find(name);
33+
if (name_it != short_names_.end()) {
34+
res_it = resources_.find(name_it->second);
35+
if (res_it != resources_.end()) {
36+
return res_it->second;
3537
}
3638
}
3739

‎src/viam/sdk/resource/stoppable.cpp‎

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,5 @@ namespace sdk {
66
Stoppable::~Stoppable() = default;
77
Stoppable::Stoppable() = default;
88

9-
void Stoppable::stop_if_stoppable(const std::shared_ptr<Resource>& resource,
10-
const ProtoStruct& extra) {
11-
auto stoppable_res = std::dynamic_pointer_cast<Stoppable>(resource);
12-
if (stoppable_res) {
13-
stoppable_res->stop(extra);
14-
}
15-
}
16-
179
} // namespace sdk
1810
} // namespace viam

‎src/viam/sdk/resource/stoppable.hpp‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -17,18 +17,6 @@ class Stoppable {
1717
return stop({});
1818
}
1919

20-
/// @brief Stops a Resource if it is Stoppable.
21-
/// @param resource The Resource to stop.
22-
/// @param extra Extra arguments to pass to the resource's `stop` method.
23-
static void stop_if_stoppable(const std::shared_ptr<Resource>& resource,
24-
const ProtoStruct& extra);
25-
26-
/// @brief Stops a Resource if it is Stoppable.
27-
/// @param resource The Resource to stop.
28-
inline static void stop_if_stoppable(const std::shared_ptr<Resource>& resource) {
29-
return stop_if_stoppable(resource, {});
30-
}
31-
3220
protected:
3321
explicit Stoppable();
3422
};

‎src/viam/sdk/tests/mocks/mock_robot.cpp‎

Lines changed: 18 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -252,31 +252,33 @@ ::grpc::Status MockRobotService::StopAll(::grpc::ServerContext*,
252252
const std::shared_ptr<Resource> resource = r.second;
253253
const ResourceName rn = to_proto(resource->get_resource_name());
254254
const std::string rn_ = rn.SerializeAsString();
255-
if (extra.find(rn_) != extra.end()) {
256-
try {
257-
Stoppable::stop_if_stoppable(resource, extra.at(rn_));
258-
} catch (const std::runtime_error& err) {
255+
256+
auto stop_without_extra = [&status_message,
257+
&status](const std::shared_ptr<Resource>& resource) {
258+
if (auto stoppable = std::dynamic_pointer_cast<Stoppable>(resource)) {
259259
try {
260+
stoppable->stop();
261+
return;
262+
} catch (const std::runtime_error& err) {
260263
status_message = err.what();
261-
Stoppable::stop_if_stoppable(resource);
262-
} catch (std::runtime_error& err) {
263-
status_message = err.what();
264-
status = grpc::UNKNOWN;
265264
} catch (...) {
266265
status_message = "unknown error";
267-
status = grpc::UNKNOWN;
268266
}
267+
status = grpc::UNKNOWN;
269268
}
270-
} else {
269+
};
270+
271+
if (extra.find(rn_) != extra.end()) {
271272
try {
272-
Stoppable::stop_if_stoppable(resource);
273-
} catch (std::runtime_error& err) {
273+
if (auto stoppable = std::dynamic_pointer_cast<Stoppable>(resource)) {
274+
stoppable->stop(extra.at(rn_));
275+
}
276+
} catch (const std::runtime_error& err) {
274277
status_message = err.what();
275-
status = grpc::UNKNOWN;
276-
} catch (...) {
277-
status_message = "unknown error";
278-
status = grpc::UNKNOWN;
278+
stop_without_extra(resource);
279279
}
280+
} else {
281+
stop_without_extra(resource);
280282
}
281283
}
282284

0 commit comments

Comments
 (0)