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

refactor: use control_cmd_auto instead #1020

Closed
wants to merge 1 commit into from

Conversation

xmfcx
Copy link

@xmfcx xmfcx commented Jun 20, 2023

Types of PR

  • New Features
  • Upgrade of existing features
  • Bugfix

Link to the issue

Part of: autowarefoundation/autoware.universe#3674

Description

Using this,

Autoware will publish:

Topic Message Type
/control/command/control_cmd autoware_msgs
/control/command/control_cmd_auto autoware_auto_msgs

To enable compatibility, this PR renames the incoming topic to suffix _auto.

How to review this PR.

Others

@HansRobo
Copy link
Member

HansRobo commented Jul 10, 2023

@xmfcx Thank you for your contribution!

But, in this repository, we need backwards compatibility to merge changes into master.
It is a problem if supporting the latest Autoware makes it impossible to use with old Autoware.

If this interface change was made after estimating that the change on the simulator side was only the change in this PR, it may be a miscalculation of the cost.
In fact, the cost of changing the topic name is not much different for scenario_simulator_v2 than changing the message type.

@xmfcx
Copy link
Author

xmfcx commented Jul 11, 2023

In fact, the cost of changing the topic name is not much different for scenario_simulator_v2 than changing the message type.

I don't have the access to the instances/systems that could be affected by this. So I am disappointed by the fact that we have miscalculated the costs.

  • I can add a try to make a modification to this repo to have it accept both types, trying to make use of https://ros.org/reps/rep-2007.html feature.
  • I can experiment with trying to have multiple subscribers to the same topic (different types) at the same time. (not sure if it'll work)

@HansRobo what do you think?

@HansRobo
Copy link
Member

I don't have the access to the instances/systems that could be affected by this. So I am disappointed by the fact that we have miscalculated the costs.

I apologize for having told about the problem of TIER IV internal.
But, the problem seems that it was unexpected for Mr. Mitsudome.
It seems necessary to wait for his judgment or the outcome of the discussion led by him as to what to do with this pull-request.

I can add a try to make a modification to this repo to have it accept both types, trying to make use of https://ros.org/reps/rep-2007.html feature.
I can experiment with trying to have multiple subscribers to the same topic (different types) at the same time. (not sure if it'll work)

thank you for your nice suggestions
I think both are worth trying!

@xmfcx
Copy link
Author

xmfcx commented Jul 11, 2023

@HansRobo I will test it out on a small test repository then.

@xmfcx
Copy link
Author

xmfcx commented Jul 13, 2023

If I do following for ros2 run demo_nodes_cpp talker:

$ ros2 topic info /chatter
Type: std_msgs/msg/String
  sub_str_ = create_subscription<std_msgs::msg::String>(
    "/chatter", 10, [](const std_msgs::msg::String::SharedPtr msg) {
      RCLCPP_INFO_STREAM(rclcpp::get_logger("test_node"), "Received: " << msg->data);
    });

  try {
    sub_f32_ = create_subscription<std_msgs::msg::Float32>(
      "/chatter", 10, [](const std_msgs::msg::Float32::SharedPtr msg) {
        RCLCPP_INFO_STREAM(rclcpp::get_logger("test_node"), "Received: " << msg->data);
      });
  } catch (const std::exception & e) {
    RCLCPP_ERROR_STREAM(rclcpp::get_logger("test_node"), e.what());
  }

It catches the exception and subscription keeps working:

[ERROR 1689248406.432381374] [test_node]: could not create subscription: invalid allocator, at ./src/rcl/subscription.c:218 ...
[INFO 1689248407.387876890] [test_node]: Received: Hello World: 234
[INFO 1689248407.387876890] [test_node]: Received: Hello World: 235
...

So it is possible to keep trying to subscribe with both, alternating between two, attempt to subscribe until it works. But this would be a very ugly solution.

Pick the fitting subscriber based on the Autoware's version

@mitsudome-r @HansRobo what about we let the scenario simulator query the autoware version and pick the appropriate subscriber type according to the version?

Right now, this service returns the Autoware External API のバージョン but we can add another service for the Autoware's own version.

@mitsudome-r @HansRobo @isamu-takagi what do you think?

@mitsudome-r
Copy link
Contributor

@xmfcx
I think having listening to Autoware version is a good idea. However, do you think it is possible to support the old Autoware version that does not publish Autoware version topic in the first place? Should we set a timeout in the scenario simulator to look for Autoware version?

@xmfcx
Copy link
Author

xmfcx commented Jul 18, 2023

However, do you think it is possible to support the old Autoware version that does not publish Autoware version topic in the first place? Should we set a timeout in the scenario simulator to look for Autoware version?

@mitsudome-r exactly, this is what I had in mind. If Autoware version is not published, it should assume it's old.

And if it is published, it can act depending on the version. I think it's a very clean solution.

@HansRobo
Copy link
Member

As I mentioned here,I think one of the good ways to get the version is getting it via a macro or constant values, in dedicated header file, for software that compiles with Autoware like scenario_simulator_v2.

With this method, it is possible to detect old Autoware.Core by judging the existence of header files as follows.

#if __has_include(<autoware_version_manager/version_autoware.hpp>)
#include <autoware_version_manager/version_autoware.hpp>  // to include VERSION_AUTOWARE macro
std::cout << "autoware version: " << VERSION_AUTOWARE << std::endl; 
#else
std::cout << "Autoware is too old to get version via header file" << std::endl;
#endif

Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
@xmfcx xmfcx force-pushed the refactor/control-msgs-auto branch from f162e9f to b35a24f Compare July 20, 2023 13:02
@hakuturu583 hakuturu583 marked this pull request as draft September 5, 2023 04:59
@hakuturu583 hakuturu583 marked this pull request as ready for review September 5, 2023 07:37
@xmfcx
Copy link
Author

xmfcx commented Mar 7, 2024

I'm closing all of my message migration PRs.

@xmfcx xmfcx closed this Mar 7, 2024
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.

4 participants