Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override domain_id with ROS_DOMAIN_ID provided by the agent #333

Merged
merged 4 commits into from Jul 10, 2023

Conversation

delihus
Copy link
Contributor

@delihus delihus commented Jun 30, 2023

There was a need to select ROS_DOMAIN_ID in firmware without recompiling. Comparing this changes #322 and this PR #331 I prepared overriding client domain id. This change will override the clients domain (only when it is 255 - safe value due to the smaller domain limit) to the ROS_DOMAIN_ID configured as an environmental variable in e. g. micro-ros-agent docker container.

Tested on Nucleo-L476RG publisher example and ROSbot 2R firmware.

Here are the changes inside firmware husarion/rosbot_ros2_firmware#20.

I prepared prebuilt docker container with these changes husarion/micro-ros-agent:humble-overrice-domain-id.

This is the example compose for ROSbot 2R or any microros serial client:

  microros:
    image: husarion/micro-ros-agent:humble-overrice-domain-id
    network_mode: host
    ipc: host
    devices:
      - ${SERIAL_PORT:?err}
    environment:
      - SERIAL_PORT
      - RMW_IMPLEMENTATION=rmw_fastrtps_cpp
      - ROS_DOMAIN_ID=2
    command: serial -D $SERIAL_PORT serial -b 576000 # -v6

image

Best regards,
JD

@delihus delihus changed the title override domain_id with agent_domain_id from env Override domain_id with ROS_DOMAIN_ID provided by the agent Jun 30, 2023
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

@pablogs9
Copy link
Member

pablogs9 commented Jul 3, 2023

Please take a look at the Windows CI Warning

@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

Copy link
Member

@pablogs9 pablogs9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feature shall be documented in user manual when merged.

@@ -1242,5 +1260,15 @@ bool FastDDSMiddleware::matched_replier_from_bin(
return rv;
}

int16_t FastDDSMiddleware::get_domain_id_from_env(){
int16_t agent_domain_id = 0;
const char * agent_domain_id_env = std::getenv( "ROS_DOMAIN_ID" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO The override of Micro XRCE-DDS Agent shall be ROS 2 agnostic. What about using XRCE_DOMAIN_ID_OVERRIDE?

CC: @Acuadros95

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then the compose would look like this if you have defined ROS_DOMAIN_ID before.

microros:
  image: husarion/micro-ros-agent:humble-overrice-domain-id
  network_mode: host
  ipc: host
  devices:
    - ${SERIAL_PORT:?err}
  environment:
    - SERIAL_PORT
    - RMW_IMPLEMENTATION=rmw_fastrtps_cpp
    - XRCE_DOMAIN_ID_OVERRIDE=${ROS_DOMAIN_ID}
    # - XRCE_DOMAIN_ID_OVERRIDE=2
  command: serial -D $SERIAL_PORT serial -b 576000 # -v6

Not bad. I can do this. ;)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @pablogs9, looks cleaner

@delihus
Copy link
Contributor Author

delihus commented Jul 4, 2023

This feature shall be documented in user manual when merged.

Where can I describe the feature?

Signed-off-by: Jakub Delicat <jakub.delicat@husarion.com>
@richiprosima
Copy link
Contributor

Build status:

  • Linux Build Status
  • Windows Build Status

@pablogs9
Copy link
Member

pablogs9 commented Jul 4, 2023

Where can I describe the feature?

Don't worry @delihus, we can do it here https://github.com/eProsima/Micro-XRCE-DDS-docs after the PR is merged

Copy link
Contributor

@Acuadros95 Acuadros95 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just tested it and can confirm it works!

@Acuadros95 Acuadros95 merged commit bd6ea2c into eProsima:develop Jul 10, 2023
3 checks passed
@pablogs9 pablogs9 mentioned this pull request Aug 10, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants