Skip to content
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.

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

Feature/dynamic config alt #27

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jessemorris
Copy link

No description provided.

Copy link
Collaborator

@Schmluk Schmluk left a comment

Choose a reason for hiding this comment

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

Thanks for this draft, looks great! I left a few comments to answer your questions and clarify some of our initial design ideas. Let me know what you think and what use-case you had in mind!

template<typename ConfigT>
ParameterEvenHandler(ConfigT* config, const std::string& key)
: key_(key),
meta_data_(config::internal::Visitor::getValues(*config)), //get meta data from config - we want this for field information etc... TODO: need to do checks
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: The info is generally not static, i.e. for virtual and array/map configs the config layout can change. This is what the getInfo function is meant to return in our demo.


std::cout << "old yaml " << old_yaml << std::endl;

if (config::internal::isEqual(old_yaml, new_yaml)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: It is well possible that the new_yaml is different but the resulting config is the same (e.g. if params are invalid or unrelated), so probabyl should check after parsing.


//TODO: alert which field has changed!?

// std::lock_guard<std::mutex> lock(mutex_);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Note: Since Dynamic configs will always be set from outside these interfaces must be thread-safe.


public:
template<typename ConfigT>
ParameterEvenHandler(ConfigT* config, const std::string& key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

In our demo all dynamic configs are registered with a unique string key (or name/id), is this the same idea?

}

template<typename ConfigT>
bool attemptUpdate(const ConfigT& config) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's probably a philosophical question how much the clients should have to know about types. In general for a C++ client it could be nice to provide a typed interface. Would it make most sense to directly template the client/interface?

On the other, can also be nice to just accept any YAML (so conversions between types and non-C++ clients work well).

auto interface = std::make_shared<ManualInterface>();
DynamicReconfigureServer::registerInterface(interface);

//I am unclear about this key. Does it need to be namespaced according to anything else in the system?
Copy link
Collaborator

Choose a reason for hiding this comment

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

the key would be a an identifier that is unique to each server (in the case of a ROS node e.g. resolved to the node). The a global identifier is required so that outside clients can send their updates to the right address. If you run our ROS example you could e.g. have several ROS nodes with dynamic config servers, and in the UI you can select the server + the config for each server to target. For other applications the end user would be responsible to namespace or otherwise disambiguate configs correctly (e.g. by having an ID counter in objects using dynamic configs).


//I am unclear about this key. Does it need to be namespaced according to anything else in the system?
//Ideally not as its just some key that refers to an INSTANCE of a config and not the Config type (like config::name())
//some bookeeping could be done here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly, this is the idea!

interface->set(new_config, "my_config");

new_config.f = 10.0;
interface->set(new_config, "my_config");
Copy link
Collaborator

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 interface directly binds to a config in the same process? Wouldn't it be easier for this use case to design the system such that the actual config structs are available e.g. in a singleton and modify them directly? That would make a lot of the parsing easier?

* The interface gets a new function that allows sub-updating e.g. Interface::setField(T, namespace)
* e.g interface->setField(10.1, "my_config/f")
*
* since we have access to my_config and we know how to access the YAML field 'f' associated with that config we can just update the field in the yaml.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think that will quite work since constructing the SimpleConfig will set the other fields to default, but calling update on the existing config (or a copy) will do what you want.

* Of course you can update the whole config by using the "base"/(root?) namespace, which in this example would be "my_config". This is also type checked as we store
* the type of SimpleConfig (as is currently implemented)/
*
* This would get around the client side needing to know how to build the WHOLE Config type, you just need to know the namespace and the type of the field (which I guess you
Copy link
Collaborator

Choose a reason for hiding this comment

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

This was the idea behind the YAML-based interface, i.e. the client won't have to know anything about the config, it will send the updates and the local config will know what to do with it (and ideally send a response in the final version)
If one knows the type of-course it could be nice to provide a templated client that can do some checks locally before sending to the config if this is what you're referring to?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants