Add elevator_action_plugin

[jamendezib]

This pull request adds a new elevator action pad plugin to the robotnik_pad_plugins package, enabling joystick control of elevator actions via ROS actionlib. The main changes include the implementation of the elevator action plugin, integration with build and package configuration files, and plugin registration.

Elevator action plugin implementation:

• Added new source file src/elevator_action_plugin.cpp and header include/robotnik_pad_plugins/elevator_action_plugin.h implementing PadPluginElevatorAction, which sends elevator commands (raise, lower, stop) using actionlib based on joystick input. [1] [2]

Build and package configuration:

• Updated CMakeLists.txt to include actionlib as a dependency and add the new plugin source file to the build. [1] [2]
• Updated package.xml to declare actionlib as a build and exec dependency.

Plugin registration:

• Registered the new plugin in robotnik_pad_pluginlib.xml and exported it using PLUGINLIB_EXPORT_CLASS in generic_pad_plugins.cpp. [1] [2] [3]

[Copilot]

Pull request overview

This PR adds a new action-based elevator control plugin (PadPluginElevatorAction) to complement the existing service-based elevator plugin. The new plugin enables joystick control of elevator actions (raise, lower, stop) through ROS actionlib, providing asynchronous action execution with callbacks.

Key changes:

• Implements new elevator action plugin with actionlib integration for asynchronous elevator control
• Adds actionlib dependency to build and package configuration
• Registers the new plugin through pluginlib mechanism for runtime loading

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
robotnik_pad_plugins/src/elevator_action_plugin.cpp	Core implementation of the action-based elevator plugin with goal sending and callback handling
robotnik_pad_plugins/include/robotnik_pad_plugins/elevator_action_plugin.h	Header file defining the PadPluginElevatorAction class interface and member variables
robotnik_pad_plugins/CMakeLists.txt	Adds actionlib dependency and includes new plugin source in build
robotnik_pad_plugins/package.xml	Declares actionlib as build dependency (missing exec_depend)
robotnik_pad_plugins/robotnik_pad_pluginlib.xml	Registers the new plugin for pluginlib discovery
robotnik_pad_plugins/src/generic_pad_plugins.cpp	Exports the new plugin class for pluginlib

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Incorrect comment: This is an action client, not a service client. The comment should be updated to reflect that this creates an actionlib action client.

Suggested change

	// Service client
	// Actionlib action client

[Copilot]

The action client is created using nh_ from the base class, but unlike other plugins (e.g., poi_plugin.cpp, blkarc_plugin.cpp), nh_ is not initialized in this plugin's initialize() method. This will cause the action client to be created with an uninitialized node handle. Add nh_ = ros::NodeHandle(); before line 26, similar to poi_plugin.cpp line 20.

[Copilot]

Inconsistent state management: When stopping the elevator (line 55), elevator_is_running_ is set to the negation of sendGoal() result. If sendGoal() returns true (success), this sets elevator_is_running_ = false, which is correct. However, if sendGoal() returns false (failure), this sets elevator_is_running_ = true, incorrectly indicating the elevator is still running even though the STOP command failed. The same issue exists on line 64. Consider setting elevator_is_running_ = false unconditionally after attempting to stop, or handle the failure case explicitly.

[Copilot]

Same inconsistent state management issue as line 55: If sendGoal() fails and returns false, this will set elevator_is_running_ = true, incorrectly indicating the elevator is still running. Consider setting elevator_is_running_ = false unconditionally or handling the failure case explicitly.

[Copilot]

Error message is unclear: The log message "PadPluginElevatorAction::execute: %d" doesn't clearly indicate what action is being performed. Consider using more descriptive messages like "Sending elevator action: %d" or adding action name strings (RAISE/LOWER/STOP) to make debugging easier.

Suggested change

	bool PadPluginElevatorAction::sendGoal(const int action)
	{
	robotnik_msgs::SetElevatorGoal elevator_goal;
	elevator_goal.action.action = action;
	ROS_INFO_NAMED("PadPluginElevatorAction", "PadPluginElevatorAction::execute: %d", action);
	// Helper function to convert action int to string
	static const char* elevatorActionToString(int action) {
	switch (action) {
	case robotnik_msgs::ElevatorAction::RAISE:
	return "RAISE";
	case robotnik_msgs::ElevatorAction::LOWER:
	return "LOWER";
	case robotnik_msgs::ElevatorAction::STOP:
	return "STOP";
	default:
	return "UNKNOWN";
	}
	}
	bool PadPluginElevatorAction::sendGoal(const int action)
	{
	robotnik_msgs::SetElevatorGoal elevator_goal;
	elevator_goal.action.action = action;
	ROS_INFO_NAMED("PadPluginElevatorAction", "Sending elevator action: %s (%d)", elevatorActionToString(action), action);

[Copilot]

[nitpick] Redundant comparison: The expression elevator_is_running_ == true can be simplified to elevator_is_running_ for better readability and consistency with line 38 and line 45 which use !elevator_is_running_.

Suggested change

	if (axes[axis_elevator_] > -0.1 && axes[axis_elevator_] < 0.1 && elevator_is_running_ == true)
	if (axes[axis_elevator_] > -0.1 && axes[axis_elevator_] < 0.1 && elevator_is_running_)

[Copilot]

Missing exec_depend for actionlib. Since actionlib is required at runtime for the action client, it should be added as an execution dependency similar to other dependencies like roscpp, pluginlib, etc.