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

Wait until action #39

Open
wants to merge 49 commits into
base: main
Choose a base branch
from
Open

Wait until action #39

wants to merge 49 commits into from

Conversation

xiyuoh
Copy link
Member

@xiyuoh xiyuoh commented Aug 7, 2024

This PR provides a wait_until PerformAction plugin for the MiR fleet adapter. It depends on and follows #36.

This performable action enables robots to travel to a waypoint and wait there until a timeout or a move off signal is received. The expected task behavior would be:

  1. Robot goes to a waypoint and waits for X seconds (configurable)
  2. If the robot receives a move-off signal (configurable), it will end the waiting action and move to the next waypoint
  3. Robot repeats Steps 1. and 2. with subsequent waypoints on a list until it has travelled to all the waypoints in the task.

During the waiting process, the robot may only move off if the default timeout has been reached, or if it receives a move off signal. This move off signal can be configured by the user depending on the user case. The plugin provides 2 out-of-the-box move off signals as well as the option for users to customize their own.

  • mission: The waiting action would be mission-based, i.e. the fleet adapter would trigger a MiR mission specified in the config when the robot starts waiting, and end the waiting action when the mission is completed.
  • plc: The waiting action would begin and the fleet adapter monitors the state of the specified PLC register. Once it turns to a positive integer or True, the waiting action ends.
  • custom: Users can define their own move off signal by writing their own MoveOff module and provide it to the fleet config. A abstract class BaseMoveOff and example of an implementation are provided in rmf_move_off.py and rmf_move_off_on_alert.py respectively.

For cases where users might want the move off signal to vary from task to task within the same fleet, they may provide the move off behavior in their task description instead of the fleet config. Any move off signal config provided in a task description will override the plugin config. If neither the task description or the plugin config provided sufficient information, no move off signals will be configured and the robot will simply wait for the full duration of the timeout.

The README contains greater detail of how customization for this action should work.

TODO:

  • Test in sim

xiyuoh added 30 commits January 15, 2024 17:06
Signed-off-by: Xiyu Oh <[email protected]>
…in pickup/dropoff, and move action-related code into its own package

Signed-off-by: Xiyu Oh <[email protected]>
Signed-off-by: Xiyu Oh <[email protected]>
Signed-off-by: Xiyu Oh <[email protected]>
Signed-off-by: Xiyu Oh <[email protected]>
Base automatically changed from xiyu/efc_fleet_adapter_mir to main November 13, 2024 08:34
@xiyuoh xiyuoh requested review from mxgrey and aaronchongth December 6, 2024 06:23
@xiyuoh xiyuoh marked this pull request as ready for review December 6, 2024 06:24
Copy link
Member

@luca-della-vedova luca-della-vedova left a comment

Choose a reason for hiding this comment

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

With a note that I didn't run this since it requires hardware and I only did the test of running it in my brain, I just have a few small comments here and there and a more meaty point where I'd be happy to hear your input. Generally speaking the quality of the coding and the API looks quite solid to me

configs/mir_config.yaml Outdated Show resolved Hide resolved
docs/mir_actions.md Outdated Show resolved Hide resolved
Comment on lines 121 to 129
mission:
mission_name: "some_mission_name" # If this mission is found on the robot, it will be queued when the action starts, and the robot will move off when the mission is completed.
retry_count: 10 # Optional field for signal type "mission". The fleet adapter will re-attempt queueing the mission for this number of tries.
resubmit_on_abort: False # Optional field for signal type "mission". Set to True to resubmit mission if the mission gets aborted by the robot. Default to False.
plc:
register: 20 # Fill in with PLC register number. Robot moves off when this PLC register returns True
custom:
module: "fleet_adapter_mir_actions.rmf_move_off_on_alert" # Fill in with path to custom module written. Robot moves off when module provides the user-defined move off signal.
default: "custom" # Specifies a default signal type out of those listed above. If the task description does not provide any signal config, the fleet adapter will use this default signal type.
Copy link
Member

Choose a reason for hiding this comment

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

This is more something for discussion rather than a request for change. Thinking again about the limitation that the adapter will only support one mission, one plc and one custom, which is especially important for custom since the module is not exposed to the user API, what do you think about a config file like the following:

signals:
  sample_mission: # Only a unique name
    signal_type: mission # Specifies the actual type
    mission_name: "some_mission_name"
    [,,,]
  sample_mission_2:
    signal_type: mission # Specifies the actual type
    mission_name: "some_other_mission_name"
    [,,,]
  custom_1:
     signal_type: custom
     module: "fleet_adapter_mir_actions.rmf_move_off_on_alert"
  custom_2:
     signal_type: custom
     module: "fleet_adapter_mir_actions.rmf_move_off_on_something_else"
default_signal: "custom_1"

In theory this should allow multiple entries which can be addressed by name (since the list is a map), but to be honest I don't know how common the use case of having such level of configurability is and if it justifies the added complexity, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a really interesting suggestion and can be incorporated very easily. I can see the main pro being having everything configured at launch, such that users who would like 10 different missions to be triggered for various tasks won't have to constantly overwrite default values with their task descriptions. In fact, this helps to reduce the complexity of task descriptions - instead of allowing users to pick and choose the values they want to overwrite, I'd limit their options to either A) pick an existing configured signal type, B) provide a mission name/PLC register (no more submitting "PLC" task without specifying the register value), or C) pick the default signal like before.

From experience I feel like high level of configurability never hurts (as long as they are clear), and more often than not it is requested when we least expect it 🤣 I'll work on adding this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Signed-off-by: Xiyu Oh <[email protected]>
@xiyuoh xiyuoh force-pushed the xiyu/wait_until_action branch from 9bd95df to 9d49b7e Compare December 24, 2024 06:06
@xiyuoh xiyuoh force-pushed the xiyu/wait_until_action branch from 9d49b7e to 6284a28 Compare December 24, 2024 07:17
Signed-off-by: Xiyu Oh <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants