feat(controllers): implement lock-free service wrappers for demanding callbacks

[bpapaspyros]

Description

• Implement lock-free service for controllers #218

This PR solves the issue by partially duplicating the existing functions but omitting the mutex locks. We can do nicer work without duplicating much, but that would require changing the original functions a bit, so ultimately I think duplicating this much is not a problem.

Review guidelines

Estimated Time of Review: 5 minutes

Checklist before merging:

• Confirm that the relevant changelog(s) are up-to-date in case of any user-facing changes

Related issues

Blocked by:

• feat(controllers): allow control type change after construction #217

[domire8]

Seems like a reasonable change to me!

[domire8]

Looks good to me!

[SprGrf]

Nice!

The base branch was changed.

[eeberhard]

justification for the lockfree service is fine, just some suggestions for clarity and code duplication

[eeberhard]

this is certainly more compact and smarter than my suggestion, which was only concerned with the callback part (so yes would have required 4x validate_service_name and get_node()->create_service as in the details block below), but at the same time doesn't need any templating.

4x implementation

void BaseControllerInterface::add_service(
    const std::string& service_name, const std::function<ControllerServiceResponse(void)>& callback) {
  auto parsed_service_name = validate_service_name(service_name, "empty");
  if (!parsed_service_name.empty()) {
    try {
      auto service = get_node()->create_service<modulo_interfaces::srv::EmptyTrigger>(
          "~/" + parsed_service_name,
          [this, callback](
              const std::shared_ptr<modulo_interfaces::srv::EmptyTrigger::Request>,
              std::shared_ptr<modulo_interfaces::srv::EmptyTrigger::Response> response) {
            auto r = handle_service_callback(callback, true);
            response->success = r.success;
            response->message = r.message;
          },
          qos_);
      empty_services_.insert_or_assign(parsed_service_name, service);
    } catch (const std::exception& ex) {
      RCLCPP_ERROR(get_node()->get_logger(), "Failed to add service '%s': %s", parsed_service_name.c_str(), ex.what());
    }
  }

void BaseControllerInterface::add_service(
    const std::string& service_name,const std::function<ControllerServiceResponse(const std::string& string)>& callback) {
  auto parsed_service_name = validate_service_name(service_name, "string");
  if (!parsed_service_name.empty()) {
    try {
      auto service = get_node()->create_service<modulo_interfaces::srv::StringTrigger>(
          "~/" + parsed_service_name,
          [this, callback](
              const std::shared_ptr<modulo_interfaces::srv::StringTrigger::Request> request,
              std::shared_ptr<modulo_interfaces::srv::StringTrigger::Response> response) {
            const auto run_callback = [&]() { return callback(request->payload); };
            auto r = handle_service_callback(run_callback, true);
            response->success = r.success;
            response->message = r.message;
          },
          qos_);
      empty_services_.insert_or_assign(parsed_service_name, service);
    } catch (const std::exception& ex) {
      RCLCPP_ERROR(get_node()->get_logger(), "Failed to add service '%s': %s", parsed_service_name.c_str(), ex.what());
    }
  }

// and again x2 for lockfree, acquire_lock false

Overall if your latest implementation is non-breaking and functions as before then seems an improvement to me.

[domire8]

This looks indeed non breaking, tested with the tags from v5.0.1 Core but built with this modulo version instead of v5.2.2. If anything was broken, then JTC should have complained. This is the best test that I can think of.