RSDK-11050: Reconfigure fix #457
Conversation
lia-viam
requested review from
njooma and
gabegottlob
and removed request for
a team
| @@ -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()); | |||
There was a problem hiding this comment.
Why do we no longer need the call to set_log_level?
| Reconfigurable::reconfigure_if_reconfigurable(res, deps, cfg); | ||
| res->set_log_level(cfg.get_log_level()); | ||
|
|
||
| if (auto reconfigurable = std::dynamic_pointer_cast<Reconfigurable>(res)) { |
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Ah I see I missed the at. OK, well, I agree this is better than the double lookups.
| res->set_log_level(cfg.get_log_level()); | ||
|
|
||
| if (auto reconfigurable = std::dynamic_pointer_cast<Reconfigurable>(res)) { | ||
| reconfigurable->reconfigure(deps, cfg); |
There was a problem hiding this comment.
⬇️
- 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.
Fixes logic in ReconfigureResource which would have prevented any action on non-Reconfigurable resources