Skip to content

RSDK-6484 - add reconfigurable base class #212

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 3 commits into from
Feb 22, 2024
Merged

RSDK-6484 - add reconfigurable base class #212

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f235eb3
reconfigurable base class
stuqdog Feb 14, 2024
f443995
header cleanup
stuqdog Feb 22, 2024
ab89d90
Merge branch 'main' of ssh://github.com/viamrobotics/viam-cpp-sdk int…
stuqdog Feb 22, 2024
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
3 changes: 2 additions & 1 deletion src/viam/examples/modules/complex/base/impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,15 @@
#include <viam/sdk/components/component.hpp>
#include <viam/sdk/components/motor/motor.hpp>
#include <viam/sdk/config/resource.hpp>
#include <viam/sdk/resource/reconfigurable.hpp>
#include <viam/sdk/resource/resource.hpp>

using namespace viam::sdk;

// `MyBase` inherits from the `Base` class defined in the viam C++ SDK and
// implements some of the relevant methods along with `reconfigure`. It also
// specifies a static `validate` method that checks config validity.
class MyBase : public Base {
class MyBase : public Base, public Reconfigurable {
public:
MyBase(const Dependencies& deps, const ResourceConfig& cfg) : Base(cfg.name()) {
this->reconfigure(deps, cfg);
Expand Down
3 changes: 2 additions & 1 deletion src/viam/examples/modules/complex/gizmo/impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <vector>

#include <viam/sdk/common/proto_type.hpp>
#include <viam/sdk/resource/reconfigurable.hpp>

#include "api.hpp"

Expand All @@ -11,7 +12,7 @@ using namespace viam::sdk;
// MyGizmo inherits from the `Gizmo` class defined in `api.hpp` and implements
// all relevant methods along with `reconfigure`. It also specifies a static
// `validate` method that checks config validity.
class MyGizmo : public Gizmo {
class MyGizmo : public Gizmo, public Reconfigurable {
public:
MyGizmo(std::string name, std::string arg1) : Gizmo(std::move(name)), arg1_(std::move(arg1)){};
MyGizmo(const Dependencies& deps, const ResourceConfig& cfg) : Gizmo(cfg.name()) {
Expand Down
4 changes: 3 additions & 1 deletion src/viam/examples/modules/complex/summation/impl.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,15 @@

#include <vector>

#include <viam/sdk/resource/reconfigurable.hpp>

#include "api.hpp"

using namespace viam::sdk;

// MySummation inherits from the `Summation` class defined in `api.hpp` and
// implements all relevant methods along with `reconfigure`.
class MySummation : public Summation {
class MySummation : public Summation, public Reconfigurable {
public:
MySummation(std::string name, bool subtract)
: Summation(std::move(name)), subtract_(subtract){};
Expand Down
6 changes: 3 additions & 3 deletions src/viam/examples/modules/simple/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
#include <iostream>
#include <memory>
#include <signal.h>
#include <sstream>

#include <boost/log/trivial.hpp>
Expand All @@ -15,6 +14,7 @@
#include <viam/sdk/module/module.hpp>
#include <viam/sdk/module/service.hpp>
#include <viam/sdk/registry/registry.hpp>
#include <viam/sdk/resource/reconfigurable.hpp>
#include <viam/sdk/resource/resource.hpp>
#include <viam/sdk/rpc/dial.hpp>
#include <viam/sdk/rpc/server.hpp>
Expand All @@ -26,9 +26,9 @@ using namespace viam::sdk;
// Printer is a modular resource that can print a to_print value to STDOUT when
// a DoCommand request is received or when reconfiguring. The to_print value
// must be provided as an attribute in the config.
class Printer : public GenericService {
class Printer : public GenericService, public Reconfigurable {
public:
void reconfigure(Dependencies deps, ResourceConfig cfg) {
void reconfigure(const Dependencies& deps, const ResourceConfig& cfg) {
std::cout << "Printer " << Resource::name() << " is reconfiguring" << std::endl;
for (auto& dep : deps) {
std::cout << "dependency: " << dep.first.to_string() << std::endl;
Expand Down
5 changes: 4 additions & 1 deletion src/viam/examples/modules/tflite/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
#include <viam/sdk/config/resource.hpp>
#include <viam/sdk/module/service.hpp>
#include <viam/sdk/registry/registry.hpp>
#include <viam/sdk/resource/reconfigurable.hpp>
#include <viam/sdk/resource/stoppable.hpp>
#include <viam/sdk/rpc/server.hpp>
#include <viam/sdk/services/mlmodel/mlmodel.hpp>
Expand Down Expand Up @@ -58,7 +59,9 @@ constexpr char service_name[] = "example_mlmodelservice_tflite";
// with the model.
//
// Any additional configuration fields are ignored.
class MLModelServiceTFLite : public vsdk::MLModelService, public vsdk::Stoppable {
class MLModelServiceTFLite : public vsdk::MLModelService,
public vsdk::Stoppable,
public vsdk::Reconfigurable {
class write_to_tflite_tensor_visitor_;

public:
Expand Down
2 changes: 2 additions & 0 deletions src/viam/sdk/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ target_sources(viamsdk
module/signal_manager.cpp
referenceframe/frame.cpp
registry/registry.cpp
resource/reconfigurable.cpp
resource/resource.cpp
resource/resource_api.cpp
resource/resource_manager.cpp
Expand Down Expand Up @@ -155,6 +156,7 @@ target_sources(viamsdk
../../viam/sdk/module/signal_manager.hpp
../../viam/sdk/referenceframe/frame.hpp
../../viam/sdk/registry/registry.hpp
../../viam/sdk/resource/reconfigurable.hpp
../../viam/sdk/resource/resource.hpp
../../viam/sdk/resource/resource_api.hpp
../../viam/sdk/resource/resource_manager.hpp
Expand Down
6 changes: 2 additions & 4 deletions src/viam/sdk/module/service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
#include <viam/sdk/module/service.hpp>

#include <exception>
#include <fstream>
#include <iostream>
#include <memory>
#include <sstream>
#include <stdexcept>
#include <string>
#include <sys/socket.h>
#include <sys/stat.h>
Expand Down Expand Up @@ -33,6 +30,7 @@
#include <viam/sdk/config/resource.hpp>
#include <viam/sdk/module/handler_map.hpp>
#include <viam/sdk/registry/registry.hpp>
#include <viam/sdk/resource/reconfigurable.hpp>
#include <viam/sdk/resource/resource.hpp>
#include <viam/sdk/resource/resource_api.hpp>
#include <viam/sdk/resource/resource_manager.hpp>
Expand Down Expand Up @@ -121,7 +119,7 @@ ::grpc::Status ModuleService::ReconfigureResource(
" as it doesn't exist.");
}
try {
res->reconfigure(deps, cfg);
Reconfigurable::reconfigure_if_reconfigurable(res, deps, cfg);
return grpc::Status();
} catch (const std::exception& exc) {
return grpc::Status(::grpc::INTERNAL, exc.what());
Expand Down
21 changes: 21 additions & 0 deletions src/viam/sdk/resource/reconfigurable.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
#include <viam/sdk/resource/reconfigurable.hpp>
Copy link
Member

@acmorrow acmorrow Feb 22, 2024

Choose a reason for hiding this comment

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

The "associated" include should always be first, in a standalone block.

Copy link
Member Author

@stuqdog stuqdog Feb 22, 2024

Choose a reason for hiding this comment

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

Oops, missed this! Thanks for the catch :)


#include <memory>

namespace viam {
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
31 changes: 31 additions & 0 deletions src/viam/sdk/resource/reconfigurable.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
#pragma once

#include <viam/sdk/config/resource.hpp>
#include <viam/sdk/resource/resource.hpp>

namespace viam {
namespace sdk {

class Reconfigurable {
public:
virtual ~Reconfigurable();

/// @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;
Copy link
Member Author

@stuqdog stuqdog Feb 15, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@acmorrow acmorrow Feb 22, 2024

Choose a reason for hiding this comment

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

I think it is fine.


/// @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,
Copy link
Member

@acmorrow acmorrow Feb 22, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@stuqdog stuqdog Feb 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Member

@acmorrow acmorrow Feb 22, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member

@acmorrow acmorrow Feb 22, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@stuqdog stuqdog Feb 22, 2024

Choose a reason for hiding this comment

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

Cool sounds good, I'll make a ticket for revisiting.

const Dependencies& deps,
const ResourceConfig& cfg);

protected:
explicit Reconfigurable();
};

} // namespace sdk
} // namespace viam
5 changes: 0 additions & 5 deletions src/viam/sdk/resource/resource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
#include <viam/sdk/resource/resource.hpp>

#include <stdexcept>
#include <unordered_map>

#include <grpcpp/support/status.h>

#include <viam/sdk/common/proto_type.hpp>
Expand All @@ -21,8 +18,6 @@ std::string Resource::name() const {
return name_;
}

void Resource::reconfigure(const Dependencies& deps, const ResourceConfig& cfg){};

ResourceName Resource::get_resource_name(std::string name) const {
ResourceName r;
*r.mutable_namespace_() = kRDK;
Expand Down
5 changes: 0 additions & 5 deletions src/viam/sdk/resource/resource.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,6 @@ class Resource {
/// @brief Returns a `ResourceName` for a particular resource name.
virtual viam::common::v1::ResourceName get_resource_name(std::string name) const;

/// @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);

/// @brief Return the resource's name.
virtual std::string name() const;

Expand Down