Is it bad practice to declare the node as a global variable to call a service from an external callback?

Question

I want to call a service SendError from a registered callback from an external library. The only way I see I can do this is by having the node declared as a global variable, but I haven't seen any examples of this and I don't know if it's bad practice.

I have tried two different ways that work, with the callback inside the class and outside the class, but it works by declaring the node as a global variable, because the callback function to register has to be static and cannot be able to access the node object.

Outside the class:

#include <iostream>
#include <rclcpp/rclcpp.hpp>

#include <libExt.h>

class Foo : public rclcpp::Node
{
public:
    explicit Foo() : Node("foo")
    {
        clientSendError = this->create_client<foo_interfaces::srv::SendError>("send_error");
    }

    void sendError(int code)
    {
        while (!clientSendError->wait_for_service(1s))
        {
            if (!rclcpp::ok())
            {
                RCLCPP_ERROR(rclcpp::get_logger("rclcpp"), "Interrupted while waiting for service. Exiting.");
                return;
            }
            RCLCPP_INFO(rclcpp::get_logger("rclcpp"), "Service not available, waiting again...");
        }

        auto requestSendError = std::make_shared<foo_interfaces::srv::SendError::Request>();
        requestSendError->code = code;

        auto resultSendError = clientSendError->async_send_request(requestSendError);
    }
    
private:
    rclcpp::Client<foo_interfaces::srv::SendError>::SharedPtr clientSendError;
}

std::shared_ptr<Foo> node;

static void errorNotification(const int32_t *error)
{
    RCLCPP_INFO(rclcpp::get_logger("rclcpp"), "Error notification received");
    int errort = *error;

    node->sendError(errort);
}

int main(int argc, char **argv)
{
    rclcpp::init(argc, argv);

    registerErrorCallback(errorNotification);
    node = std::make_shared<Foo>();

    rclcpp::spin(node);
    rclcpp::shutdown();
    return 0;
}

Inside the class (class has to be declared before node declaration too, which doesn't look too good):

#include <iostream>
#include <rclcpp/rclcpp.hpp>

#include <libExt.h>

class Foo;

std::shared_ptr<Foo> node;

class Foo : public rclcpp::Node
{
public:
    explicit Foo() : Node("foo")
    {
        registerErrorCallback(errorNotification);

        clientSendError = this->create_client<foo_interfaces::srv::SendError>("send_error");
    }

private:
    void sendError(int code)
    {
        while (!clientSendError->wait_for_service(1s))
        {
            if (!rclcpp::ok())
            {
                RCLCPP_ERROR(rclcpp::get_logger("rclcpp"), "Interrupted while waiting for service. Exiting.");
                return;
            }
            RCLCPP_INFO(rclcpp::get_logger("rclcpp"), "Service not available, waiting again...");
        }

        auto requestSendError = std::make_shared<foo_interfaces::srv::SendError::Request>();
        requestSendError->code = code;

        auto resultSendError = clientSendError->async_send_request(requestSendError);
    }
    
    static void errorNotification(const int32_t *error)
    {
        RCLCPP_INFO(rclcpp::get_logger("rclcpp"), "Error notification received");
        int errort = *error;

        node->sendError(errort);
    }

    rclcpp::Client<foo_interfaces::srv::SendError>::SharedPtr clientSendError;
}

int main(int argc, char **argv)
{
    rclcpp::init(argc, argv);

    node = std::make_shared<Foo>();

    rclcpp::spin(node);
    rclcpp::shutdown();
    return 0;
}

It works, but is this bad practice? If it is, how can I make this work using good practice?

If it's good practice, is it better to have the callback inside or outside the class?

Edit: the library is in pure C.

Answer 1

I don't 100% understand the full situation but here are a couple tips:

• Split your file into a header and source file, then include header whenever necessary. which will fix your issue with class declaration before definition.

• If possible swap from using a raw function pointer to an std::function object, via either the constructor or std::bind. as it allows you to use a non static method and is overall safer.

• Its better to attach the callback to the node, to make it easier to understand when you return to this after a break.

• there are use cases for global variables, but I personally have not use many in my projects, and try to avoid them.