PDDL Trees as ROS2 Messages

[jjzapf]

This pull request proposes a new method for passing PDDL trees between the various nodes of PlanSys2. Currently, the system uses C++ classes to define the PDDL trees. However, these objects cannot be passed directly between the nodes. We must first convert the trees to string representations. The receiving nodes must then parse the strings to get back to the PDDL trees.

This extra string conversion/parsing step adds a lot of processing time to the system. It also requires extra code to support the string conversion/parsing. By implementing the trees as ROS2 messages, we can directly pass the trees between the nodes without the extra string conversion/parsing steps. This saves quite a bit of processing time and simplifies the code.

The main changes are in plansys2_msgs and plansys2_pddl_parser->Utils.cpp. Utils.cpp replaces Tree.cpp and is a re-implementation of the tree processing algorithms to use the new ROS2 tree/node messages. The changes in the rest of code are largely a result of this replacement.

A README file was added to the plansys2_msgs package. The beginning of this document discusses the problem of representing PDDL expressions as trees and how these trees can be implemented as ROS2 messages. This is probably the best place to start when reviewing the proposed changes.

[codecov]

Codecov Report

Merging #118 (0d7998e) into master (a15af53) will increase coverage by 0.10%.
The diff coverage is 29.52%.

@@            Coverage Diff             @@
##           master     #118      +/-   ##
==========================================
+ Coverage   24.58%   24.68%   +0.10%     
==========================================
  Files         126      125       -1     
  Lines       10720    10732      +12     
  Branches     6945     6883      -62     
==========================================
+ Hits         2635     2649      +14     
- Misses       2504     2537      +33     
+ Partials     5581     5546      -35     

Flag	Coverage Δ
unittests	24.68% <29.52%> (+0.10%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...e/plansys2_domain_expert/DomainExpertInterface.hpp	100.00% <ø> (ø)
...cutor/include/plansys2_executor/ActionExecutor.hpp	100.00% <ø> (ø)
...2_executor/include/plansys2_executor/BTBuilder.hpp	100.00% <ø> (ø)
...cutor/include/plansys2_executor/ExecutorClient.hpp	100.00% <ø> (ø)
..._executor/src/plansys2_executor/ActionExecutor.cpp	49.65% <0.00%> (ø)
...executor/behavior_tree/apply_atend_effect_node.cpp	53.33% <0.00%> (ø)
...ecutor/behavior_tree/apply_atstart_effect_node.cpp	53.33% <0.00%> (ø)
...s2_executor/behavior_tree/check_atend_req_node.cpp	60.00% <0.00%> (ø)
..._executor/behavior_tree/check_overall_req_node.cpp	60.00% <0.00%> (ø)
...2_executor/behavior_tree/wait_atstart_req_node.cpp	57.89% <0.00%> (ø)
... and 69 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a15af53...0d7998e. Read the comment docs.

[fmrico]

Hi @jjzapf

I have been analyzing this PR, and I think it is a great efficiency improvement, but I would like that you make some changes in the user interface. I think it not intuitive that, for example, in plansys2_problem_expert/ProblemExpert.hpp, there is a declaration like:

bool addInstance(const plansys2_msgs::msg::Param & instance);

because the user doesn't need (and shouldn't) to know that the Instances are coded as a plansys2_msgs::msg::Parammessage. He should think "I am adding a PlanSys2 instance". I have been working with your branch, and I think that the solution is straightforward. I propose to convert this declaration to:

bool addInstance(const plansys2::Instance & instance);

that is more natural. To make it works, I have only created this type in plansys2_core/Types.hpp as follows:

class Instance : public plansys2_msgs::msg::Param
{
public:
  Instance(const std::string & name, const std::string & type)
  : plansys2_msgs::msg::Param(parser::pddl::fromStringParam(name, type)) {}
  Instance(const plansys2_msgs::msg::Param & instance)
  : plansys2_msgs::msg::Param(instance) {}
};

This lets us create instances like this:

problem_expert.addInstance(plansys2::Instance("Paco", "person"));

or even

problem_expert.addInstance({"Paco", "person"});

I would do this for every pddl element (predicate, goal, function...)

[jjzapf]

Hi @fmrico

Thanks for analyzing this PR! I understand your concern about the message type naming schemes not matching the client function calls. I was actually a bit concerned about this myself. I agree that we should make the system as intuitive as possible. As I'm sure you've noticed, the reason the names do not match is that I have less message types than PDDL constructs. That is, I'm using a single message type to represent multiple constructs. For example, a plansys2_msgs/Param could be a PDDL instance or it could be a function/predicate param. Likewise a plansys2_msgs/Node could be many different constructs. In general I think this is a good thing, since it reduces the number of message types that we need to maintain. However, you're right that it does make things a bit confusing from the user perspective.

What do you think about just changing the client functions to accept strings? For example, we could have problem_expert.addInstance("Paco", "person"), where addInstance would then call plansys2_msgs::msg::Param(parser::pddl::fromStringParam(name, type)). Similarly, we could have a problem_expert.addPredicate("(robot_at r2d2 millenium_falcon)"), where addPredicate would then call plansys2_msgs::msg::Node(parser::pddl::fromStringPredicate(pred_string)). Looking over the code, I think we pretty much always make a "fromString" call prior to calling the client functions. So functionally, the code would pretty much remain the same. (Note that the internal communication would still be handled by the new messages.) However, it would still achieve the reduction in user complexity that you are looking for. The other advantage to this approach is that we would not need to maintain the additional object hierarchies, i.e., we would not need to make the additional class objects.

What do you think?

[fmrico]

Hi @jjzapf

We can add a constructor that accepts strings to the interface (in the plansys2::*Interface class), but maintaining the constructor that accepts instances like plansys2::Predicate or plansys2::Instance. My proposal emphasizes maintaining these constructors and types because there are many PlanSys2 users, a lot of repos (here and here), and tutorials that we should change if we break the backward compatibility.

I would add the proposed classes, because it is transparent for the users, and you could still be working with the internal types. I agree that your optimization lets to simplify and generalize types, but the users should maintaining an intuitive interface. Developers can select both options.

[jjzapf]

Ok. Maintaining backward compatibility does make a lot of sense. I'm fine going with this approach. I will make the changes you suggest. Thanks!

[jjzapf]

Hi @fmrico

I wasn't able to finish the changes today, but I did make some progress. See recent commits. I believe I've taken care of the instance interface calls (i.e. addInstance, removeInstance, getInstance, and getInstances). The getInstances case was a bit tricky, because we need to convert back and forth from a vector of the base class (plansys2_msgs::msg::Param) and a vector of the derived class (plansys2::Instance). I wasn't sure of the best way to do this. In the end, I added two static conversion functions to the plansys2::Instance class. So the derived class looks like this.

class Instance : public plansys2_msgs::msg::Param
{
public:
  Instance(const std::string & name, const std::string & type = {})
  : plansys2_msgs::msg::Param(parser::pddl::fromStringParam(name, type)) {}
  Instance(const plansys2_msgs::msg::Param & instance)
  : plansys2_msgs::msg::Param(instance) {}
  static std::vector<plansys2_msgs::msg::Param> toParam(const std::vector<Instance> & input)
  {
    std::vector<plansys2_msgs::msg::Param> ret;
    ret.reserve(input.size());
    std::transform(
      input.begin(), input.end(), std::back_inserter(ret),
      [](Instance item)
      {
        return parser::pddl::fromStringParam(item.name, item.type);
      });
    return ret;
  }
  static std::vector<Instance> toInstance(const std::vector<plansys2_msgs::msg::Param> & input)
  {
    std::vector<Instance> ret;
    ret.reserve(input.size());
    std::transform(
      input.begin(), input.end(), std::back_inserter(ret),
      [](plansys2_msgs::msg::Param item)
      {
        return Instance(item);
      });
    return ret;
  }
};

Let me know if you can think of a better way to do this. @xydesa, feel free to chime in if you have some ideas here as well. I'm hoping to finish up the rest of the changes tomorrow.

Thanks!

[jjzapf]

Just a quick update ...

I was able to remove the conversion functions from the plansys2::Instance class. Previously, I has modified the getInstances, AddInstance, removeInstance, and getInstance declarations in ProblemExpertInterface.hpp to use the new derived plansys2::Instance class. However, because both the ProblemExpert and ProblemExpertClient must implement these virtual functions, I ended up having to modify a lot of downstream processes, especially when I started to modify the predicate functions.

To avoid this, I made some new user access functions in the problem expert client, which call the virtual function implementations. The access functions make use of the derived message class, while the virtual function implementations continue to use the base ROS2 message. This helps reduce the number of times that we need to convert between the derived message class and the base ROS2 message, since we only need to perform the conversion at the point of entry in the problem expert client.

[jjzapf]

Hi @fmrico

I think this is mostly done. I will give it one more look tomorrow to see if there is anything left to cleanup. When I'm happy with it, I'll give you the go ahead for your final review. Thanks.

[jjzapf]

Hi @fmrico

I've finished my final checks and I believe this should be ready to go. One thing to note is that I added the plansys2_core package to the CI workflow. We noticed that this package was absent from the workflows/master.yaml file (credit goes to @xydesa for finding this). We figured it was a good idea to add it, especially since we are adding more stuff to it.

I look forward to your review. Thanks again!

[fmrico]

Hi @jjzapf

Sorry for the delay. I have been very busy last week. I hope to have time to review this weekend

[jjzapf]

No worries.

[jjzapf]

@xydesa and @fmrico, I removed the explicit specifiers from some of the constructor declarations in plansys2_core/Types.hpp. (See recent commit.) These specifiers seem to be causing build errors such as this

error: could not convert ‘ret’ from ‘plansys2_msgs::msg::Node’ {aka ‘plansys2_msgs::msg::Node_<std::allocator<void> >’} to ‘std::optional<plansys2::Predicate>’

@xydesa is correct, however, that cpplint believes the specifiers should be used. This is likely the reason I had originally created the additional user access functions. These functions were removed based on the suggestion by @fmrico. I agree with @fmrico that it is nicer not to have the extra access functions. ... So it seems that we may not be able to have the best of both worlds, i.e., make cpplint happy and not require the use of extra access functions. I'm not exactly sure why cpplint is suggesting the use of the explicit specifier here. So unless anyone has concerns with this, I'm fine leaving it out.

[fmrico]

Hi @jjzapf

Thanks for working on this PR. I think that we got a good result (even if we make cpplint a little sad).

Merging!!!!!!