not necessary, or unrelated to this PR

these are unrelated, please rollback them

these are unrelated, please rollback them

when the frame_id is empty, the system will keep using the latest frame id?

Here we want to call the set depth with -5.0 for example, which will default to whatever controller defaults to. but,

suppose we call depth=-4.0 frame_id="torpedo" first, then we call depth=-5.0 without frame_id (assuming will default to 'odom'), it will not, instead it will go to depth=-5.0 at frame_id="torpedo"

simply, if the frame_id is empty, then empty the self.frame_id

if not req.frame_id
    self.target_frame_id =  req.frame_id
self.target_frame_id = req.frame_id

After this there is no point in the if statement, so why not just remove the if, and set the target_frame_id = req.frame_id at all times

Got it.
self.target_frame_id = req.frame_id # If the target frame_id is not given, simply empty the frame_id

I think we don't need this?

also this should not be needed

this as well

Yes, transform handling is now at controller_ros.h

I think we must leave this empty,

self.target_frame_id = ""

since the controller node will already default to some frame if the given frame is empty. let's not default to some value in 2 places. harder to maintain.

True, controller_ros.h handles it already

these needed?

TF libraries are no longer needed here. Also, time was not being used, thus removed as well.

Hmm, the position controller state vector accepts targets in odom frame by default, not base link. desired_state_.head(6)

maybe this base_frame is meant to be "odom", in any case let's rename it to default_frame

Changing base_frame ---> default_frame

lets make this a bit more readable.

const auto is_transform_required = !source_frame.empty() || source_frame == base_frame;

then

if (is_transform_required) {
try {
transform_stamped = tf_buffer.lookupTransform(base_frame, source_frame, ros::Time(0));
tf2::doTransform(msg->pose, transformed_pose, transform_stamped);
ROS_DEBUG_STREAM_THROTTLE(10.0,"Transformed pose from " << source_frame << " to " << base_frame); }
} catch (tf2::TransformException &ex) {
rest of the catch.
}

Hmm. In this implementation,
const auto is_transform_required = !source_frame.empty() || source_frame == base_frame;
the transformation would be considered required even if source_frame is the same as default frame. But transforming to the same frame seems unnecessary. Do we need to transform the source frame to the default frame if they are the exact same?
This one will not cause unnecessary transformation:
const auto is_transform_required = !source_frame.empty() && source_frame != default_frame; // only transform if not empty and not the same as default frame

Correct,

const auto is_transform_required = !source_frame.empty() && source_frame != default_frame;

Let's drop the message if failing to transform and never write to desired_state_.head(6)

example
suppose the frame "frame1" is 50m above the odom. (not likely but anyways :D), target of -50 at frame1 is actually target of 0 at odom, so it should be surfacing (odom is at water surface). but if you type it wrong or the transform between frames were not valid for some couple of seconds. then -50 at default frame is used with this code. then the robot will hit the bottom.

instead, drop the message and return from the function, ignoring the target pose.

Hmm, never thought about that.

catch (tf2::TransformException &ex) { // if transformation failed, ignore the target pose.
        return;

I guess this is for debugging. After making sure the PR acts as expected, let's remove this publisher to avoid confusion.

lets leave this cmd_pose

Alright. Changing the publishing part in depth_controller_node.py

# Initialize publisher
        self.cmd_pose_pub = rospy.Publisher(
            "/taluy/cmd_pose", PoseStamped, queue_size=10)

maybe get this from rosparam

const auto nh_priv = ros::NodeHandle{"~"};
position_control_default_frame_ = nh_priv.param<std::string>("position_control_default_frame", "odom");


...

private:
  std::string position_control_default_frame_;

Correct,

const auto is_transform_required = !source_frame.empty() && source_frame != default_frame;

New cmd_pose_callback structure with a get_source_frame helper function.

member variable decleration for position_control_default_frame_

publishes to cmd_pose instead of cmd_pose_stamped