Skip to content

RSDK-11050: Reconfigure fix #457

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Jun 26, 2025
Merged

RSDK-11050: Reconfigure fix #457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
55a1411
do reconfigure manually
lia-viam Jun 24, 2025
bebbbf5
remove redundant at calls
lia-viam Jun 24, 2025
d04368a
restore deleted log level call
lia-viam Jun 26, 2025
4358c29
remove stop if stoppable
lia-viam Jun 26, 2025
538e6e0
todo comment
lia-viam Jun 26, 2025
9f6b8b5
replace remaining instances of stop if stoppable
lia-viam Jun 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 9 additions & 12 deletions src/viam/sdk/module/service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Reconfigurable>(res)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If reconfigure_if_reconfigurable is going away, I think stop_if_stoppable should too. I don't think they are clearer than just using dynamic_pointer_cast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

reconfigurable->reconfigure(deps, cfg);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⬇️

  • If we aren't going to address the issue where we construct a new resource before we have destroyed the old in this PR, let's ticket that work and add a TODO down around line 132 now. I think it is something of a priority.
  • Do we have any idea why the compiler wasn't able to flag everything after the block dealing with reconfigure as dead code? Actually, nevermind. I see it. If the code managed to throw something other than a std::exception, then it wasn't dead. Oops.

res->set_log_level(cfg.get_log_level());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we no longer need the call to set_log_level?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

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<Stoppable>(res)) {
stoppable->stop();
}

const std::shared_ptr<const ModelRegistration> 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> resource = reg->construct_resource(deps, cfg);
Expand Down Expand Up @@ -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<Stoppable>(res)) {
stoppable->stop();
}

manager->remove(name);
Expand Down
9 changes: 0 additions & 9 deletions src/viam/sdk/resource/reconfigurable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,5 @@ namespace sdk {
Reconfigurable::~Reconfigurable() = default;
Reconfigurable::Reconfigurable() = default;

void Reconfigurable::reconfigure_if_reconfigurable(const std::shared_ptr<Resource>& resource,
const Dependencies& deps,
const ResourceConfig& cfg) {
auto reconfigurable_res = std::dynamic_pointer_cast<Reconfigurable>(resource);
if (reconfigurable_res) {
reconfigurable_res->reconfigure(deps, cfg);
}
}

} // namespace sdk
} // namespace viam
8 changes: 0 additions & 8 deletions src/viam/sdk/resource/reconfigurable.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>& resource,
const Dependencies& deps,
const ResourceConfig& cfg);

protected:
explicit Reconfigurable();
};
Expand Down
14 changes: 8 additions & 6 deletions src/viam/sdk/resource/resource_manager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,16 @@ namespace sdk {
std::shared_ptr<Resource> ResourceManager::resource(const std::string& name) {
const std::lock_guard<std::mutex> 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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, this was a latent bug, in that the old code tried to look up the resource by short name a second time, rather than by using the "long name" it dug out of the short names map. Is that basically it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think the original logic was correct, the issue was just that it was discarding the result of find and then calling at instead of keeping the iterator and dereferencing it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see I missed the at. OK, well, I agree this is better than the double lookups.

if (res_it != resources_.end()) {
return res_it->second;
}
}

Expand Down
8 changes: 0 additions & 8 deletions src/viam/sdk/resource/stoppable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,5 @@ namespace sdk {
Stoppable::~Stoppable() = default;
Stoppable::Stoppable() = default;

void Stoppable::stop_if_stoppable(const std::shared_ptr<Resource>& resource,
const ProtoStruct& extra) {
auto stoppable_res = std::dynamic_pointer_cast<Stoppable>(resource);
if (stoppable_res) {
stoppable_res->stop(extra);
}
}

} // namespace sdk
} // namespace viam
12 changes: 0 additions & 12 deletions src/viam/sdk/resource/stoppable.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>& 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>& resource) {
return stop_if_stoppable(resource, {});
}

protected:
explicit Stoppable();
};
Expand Down
34 changes: 18 additions & 16 deletions src/viam/sdk/tests/mocks/mock_robot.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,31 +252,33 @@ ::grpc::Status MockRobotService::StopAll(::grpc::ServerContext*,
const std::shared_ptr<Resource> 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>& resource) {
if (auto stoppable = std::dynamic_pointer_cast<Stoppable>(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<Stoppable>(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);
}
}

Expand Down