Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

Port planning_sim_launcher to ROS2 #2

Draft
wants to merge 2 commits into
base: ros2
Choose a base branch
from

Conversation

nnmm
Copy link

@nnmm nnmm commented Nov 23, 2020

Changes

Launch architecture

There was an RPC server for communication between scenario_runner and planning_simulator_launcher based on a package that is not available in ROS2 anymore. Luckily, with the ROS2 Python launch API, a simpler design is possible that doesn't require the RPC server anymore.

Previously, there was a script, scenario_roslaunch_server.py, that launched the scenario_runner.launch launch file. That script listened on a server for calls made from within scenario_runner, most importantly a "terminate" call which would stop the launch file execution. This "terminate" command was used by the scenario_runner to stop all the currently running nodes (instead of just exiting, which would only stop itself), and by scenario_roslaunch_server.py when a timeout on the simulation duration was reached.

Another function of that server was to receive updates for some variables of interest (duration and mileage of simulation), which it would then write to a log file.

We modified the launch script to exit when the scenario_runner node exits, eliminating the use case of "terminate" by scenario_runner. Terminating from outside (for timeouts) can be done using standard launch APIs. And logging of these variables of interest is now done directly by the scenario_runner node itself.

Launch files

  • Deleted npc_simulator from launch files

Python3

I made a few easy changes to all Python files:

  • Removed #!/usr/bin/env python, it is not needed for libraries at all and even for the executable, ros2 run takes care of selecting the interpreter
  • Fixed lint issues pointed out by flake8 and ran isort
  • Changed double underscores to single underscores since double underscores have special meaning in Python.

Since I had to basically rewrite it anyway, I tried to make the code in launch.py idiomatic Python:

  • Used pathlib instead of os
  • Used print(..., end='') instead of sys.stdout.write.
  • Used proper string interpolation instead of appending with +
  • Used enumerate() instead of creating a manual counting variable
  • No passing arguments via self: self.parser was basically a hidden argument to map_path(), i.e. run_all_scenario() sets that variable before each call to map_path(). If someone else would call map_path(), it would do something unexpected or fail, so I made it an argument to the function to make it less confusing
  • Added type annotations
  • Made a class for the database parsing that validates the database contents in the constructor

The code in scenario_generator.py, scenario_parameter.py, scenario_parser.py and show_result.py have been left untouched apart from the aforementioned global changes. It is somewhat difficult to understand what they do anyway since there are no comments. But a few things to point out, in case someone needs to revisit this

  • scenario_id sometimes has a different meaning in these files than in launcher.py which is confusing
  • Python functions should use snake_case
  • Comments are needed to understand what it does
  • If this code basically substitutes parameters, maybe this could be simplified by using a templating engine
  • Hardcoded values that should be shared between files (like "generate_log.json")

Reviewer comments

  • I changed it so that all paths in scenario_database.json and in command line arguments are required to be absolute. With relative paths, there can be confusion whether they are relative to the current working directory, the scenario_database.json file, the script file source location or script file install location. If we want to change it later, it's relatively easy, we just need to give the absolute_call function the right base_dir.
  • Please check our reasoning for the "terminate" command

Open questions

  • Wouldn't you want to configure e.g. the vehicle model via scenario_database.json or the command line? Should there be a flag to forward args to the launch file?

@nnmm nnmm marked this pull request as draft November 23, 2020 15:49
@nnmm nnmm force-pushed the planning_simulator_launcher branch 10 times, most recently from 53623f9 to 7223f63 Compare November 26, 2020 16:50
@nnmm nnmm force-pushed the planning_simulator_launcher branch from 7223f63 to 928b06f Compare November 26, 2020 17:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant