sequenceplanner/r2r

Support for constants in ROS messages

Closed this issue · 10 comments

First of all, thanks for making this binding crate! It makes it super easy for communicating with existing C++/Python nodes.

What I want to ask is whether we plan to support constants in ROS messages shortly. Currently, I cannot find the support and need to manually duplicate the ROS message constants in Rust.

One of the examples is action_msgs::msg::GoalStatus, where constants STATUS_UNKNOWN, STATUS_ACCEPTED, etc. are defined. However, the generated action_msgs.rs looks like this:

pub mod msg {
    // ... some other structs

    #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
    #[serde(default)]
    pub struct GoalStatus {
        pub goal_info: action_msgs::msg::GoalInfo,
        pub status: i8,
    }

    impl WrappedTypesupport for GoalStatus {
        type CStruct = action_msgs__msg__GoalStatus;

        fn get_ts() -> &'static rosidl_message_type_support_t {
            unsafe { &*rosidl_typesupport_c__get_message_type_support_handle__action_msgs__msg__GoalStatus() }
        }

        fn create_msg() -> *mut action_msgs__msg__GoalStatus {
            unsafe { action_msgs__msg__GoalStatus__create() }
        }

        fn destroy_msg(msg: *mut action_msgs__msg__GoalStatus) -> () {
            unsafe { action_msgs__msg__GoalStatus__destroy(msg) };
        }

        fn from_native(msg: &Self::CStruct) -> GoalStatus {
            GoalStatus {
                goal_info: action_msgs::msg::GoalInfo::from_native(&msg.goal_info),
                status: msg.status,
            }
        }

        fn copy_to_native(&self, msg: &mut Self::CStruct) {
            self.goal_info.copy_to_native(&mut msg.goal_info);
            msg.status = self.status;
        }
    }

    impl Default for GoalStatus {
        fn default() -> Self {
            let msg_native = WrappedNativeMsg::<GoalStatus>::new();
            GoalStatus::from_native(&msg_native)
        }
    }

    // ...some other structs
}

What I tried before raising this issue:

  1. Read the documentation to see if this is mentioned elsewhere
  2. Checked the features of the crates to see if there is a feature to open
  3. Went over the files under OUT_DIR to see if it is generated in another file

Please forgive me if the constants are generated somewhere else or if this is a known issue. And thanks again for making this crate!

m-dahl commented

Thanks for your kind words. The constants are unfortunately not implemented in a nice way, but I agree they would be nice to have.

However, I you need them, they are still there in a way, if you depend on r2r_msg_gen.

For example, you can write:

    let x = r2r_msg_gen::action_msgs__msg__GoalStatus__STATUS_ABORTED as i8;
    println!("{x}");

The problem is that the constants are not part of the rosidl introspection api, they are only generated as enums in the c header files. We do give the header files to bindgen, which is why you can use the constant as above, but there is no way to know that a constant belongs to a certain type.

However, now that I think about it, perhaps we could write a macro that expands to the correct identifier. Maybe it could also get the correct type from the field name. 🤔

m-dahl commented

I took a stab at exposing the constants in the respective message type here #46

Thank you for the reply!

I think I can work with r2r_msg_gen::action_msgs__msg__GoalStatus__STATUS_ABORTED for now. I don't have a lot of constants to deal with, so adding a module that reexports the constants with user-friendly names is not a big task.

Please feel free to keep or close the issue.

m-dahl commented

Great. For goal status is the get_status function not enough for you? Goal status is implemented with a rust enum just because it's a bit of a special case.

I will keep the issue open until I finish #46, in case someone else is interested in the constants.

Well, my problem is in some custom ROS messages, but I took GoalStatus as an example because it is a well-known ROS message type and is always built in r2r, hence is easier to reproduce the issue.

Thanks again for making this crate.

m-dahl commented

Ok I see. Would you like to quickly try the branch in pr #46? It seems to work just fine here, if it works for you I can merge it and make a new release.

That branch works. Now I can see a block in the generated file like

#[allow(non_upper_case_globals)]
impl MyCustomMsg {
    pub const MY_CONSTANT_ONE: _bindgen_ty_139 = my_custom_package__msg__MyCustomMsg__MY_CONSTANT_ONE;
}

Thanks!

Thank you for implementing this, it works for me as well.

I think the ergonomics could still be improved. As of now, match statements on constants are a bit difficult due to their _bindgen_ty_* enum type. Would it be possible to add a cast to their actual type as specified in the msg file?

m-dahl commented

Thanks @JiangengDong and @krepa098 for testing the pr.

I agree about the bindgen enum types being inelegant. But as it is we never actually read the msg files, everything is based on the c code generated by the ros msg pipeline. Unfortunately the ros pipeline generates all constants without any type information. It also does it with one enum per constant, like this (which is why we get all these bindgen types):

enum
{
  action_msgs__msg__GoalStatus__STATUS_ACCEPTED = 1
};

I think we would have to actually start reading the msg files to be able to force the correct type. Which I am hesitant to work on at the moment.

For now I will merge this and we can open a new issue for the improvement.

m-dahl commented

FYI I released 0.7.1 which includes this change.