RSDK-6484 - add reconfigurable base class #212
Conversation
| /// @brief Reconfigures a resource. | ||
| /// @param deps Dependencies of the resource. | ||
| /// @param cfg The resource's config. | ||
| virtual void reconfigure(const Dependencies& deps, const ResourceConfig& cfg) = 0; |
There was a problem hiding this comment.
I opted to change the signature of reconfigure to take its args by const&. I think this especially makes sense for the cfg; I could theoretically imagine someone wanting non-const access to deps but I think passing deps by value would be a bit strange, and I suspect that const& dependencies will be correct and manageable. In the worst case, we can add an override of reconfigure that isn't const&. Happy to revisit if others have thoughts.
stuqdog
requested review from
a team,
acmorrow,
benjirewis,
njooma and
purplenicole730
and removed request for
a team,
benjirewis and
njooma
acmorrow
left a comment
acmorrow
left a comment
There was a problem hiding this comment.
One nit, and a question. I'm happy for the answer to the question to be "let's do that in a subsequent step/review".
| @@ -0,0 +1,20 @@ | |||
| #include <memory> | |||
| #include <viam/sdk/resource/reconfigurable.hpp> | |||
There was a problem hiding this comment.
The "associated" include should always be first, in a standalone block.
There was a problem hiding this comment.
Oops, missed this! Thanks for the catch :)
| /// @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, |
There was a problem hiding this comment.
Not sure if we discussed this before or not, but I wonder if you could just make this a non-virtual method of Resource:
class Resource {
...
void reconfigure(const Dep..., const Res...);
...
};
void Resource::reconfigure(...) {
auto reconfigurable_res = std::dynamic_pointer_cast<Reconfigurable>(resource);
if (reconfigurable_res) {
reconfigurable_res->reconfigure(...);
}
Same could be done for stop / Stoppable?
There was a problem hiding this comment.
I like this, it seems nice to have stop and reconfigure sit on the Resource class. Happy enough to make the change here, it seems pretty simple.
One clarifying point/question, I believe the methods will have to be static because otherwise the this arg is a raw pointer, but dynamic_pointer_cast requires a shared_ptr. Is there some cleverness I'm missing for how to have a non-static member method allow for a dynamic_pointer_cast of this?
There was a problem hiding this comment.
Oh, sorry - I hadn't really focused on the fact that it took a shared_ptr arg. But I don't think it is an obstacle: you can just use plain dynamic_cast instead of dynamic_pointer_cast.
There was a problem hiding this comment.
The only thing that maybe bugs me about my proposal is that having reconfigure and (even more so) stop methods on Resource that may be no-ops feels a tiny bit disingenuous, in that you could call the method but it does nothing unless the dynamic type of the object happens to have opted in by deriving from the right mixins. Let's get this merged as is and then think a little harder about it. If we end up with lots of mixins for optional behavior, I also don't want to necessarily clutter the Resource class with convenience entry points for all of them.
There was a problem hiding this comment.
Cool sounds good, I'll make a ticket for revisiting.
| /// @brief Reconfigures a resource. | ||
| /// @param deps Dependencies of the resource. | ||
| /// @param cfg The resource's config. | ||
| virtual void reconfigure(const Dependencies& deps, const ResourceConfig& cfg) = 0; |
…o reconfigurable-base-class
3 participants
Adds
Reconfigurablebase class, analogized fromStoppable