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

[FSM/Executor] Make resetting posture tasks optional #389

Merged
merged 2 commits into from
Sep 13, 2023

Conversation

arntanguy
Copy link
Collaborator

This PR allows to skip the reset of the posture tasks before transitioning to the next state. This has been requested multiple times, and proves useful in some cases where the user does not want to keep the current posture. Example:

base: Meta
ResetPostures: false
transitions:
- [ChooseState, A, StateA, Auto]
- [ChooseState, B, StateB, Auto]
- [StateA, OK, HalfSitting, Auto]
- [StateB, OK, HalfSitting, Auto]
- [HalfSitting, OK, ChooseState, Auto]

This will set the desired posture to HalfSitting instead of keeping it to the current posture. Not that with ResetPostures: true in this example, the behaviour would be counterintuitive: the effet of the HalfSitting state is immediately canceled by the resetting of the posture tasks done by the Exectutor (as we are not awaiting for completion of the HalfSitting state).

Copy link
Member

@gergondet gergondet left a comment

Choose a reason for hiding this comment

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

Thanks @arntanguy

However, I think the default should be reconsidered:

  1. True for the main executor
  2. False for Meta executors

Note: this closes #353

@@ -8,7 +8,8 @@
"properties":
{
"Managed": { "type": "boolean", "default": false, "description": "If true, this state does not handle transitions by itself"},
"StepByStep": { "type": "boolean", "description": "Affects the StepByStep transition behaviour<br/>If not set, inherits the setting from its parent FSM" },
"StepByStep": { "type": "boolean", "default": true, "description": "Affects the StepByStep transition behaviour<br/>If not set, inherits the setting from its parent FSM" },
"ResetPostures": { "type": "boolean", "default": true, "description": "When true rest the posture tasks to the current posture before transitioning to the next state" },
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"ResetPostures": { "type": "boolean", "default": true, "description": "When true rest the posture tasks to the current posture before transitioning to the next state" },
"ResetPostures": { "type": "boolean", "default": true, "description": "When true reset the posture tasks to the current posture before transitioning to the next state" },

@@ -8,7 +8,8 @@
"properties":
{
"Managed": { "type": "boolean", "default": false, "description": "If true, this state does not handle transitions by itself"},
"StepByStep": { "type": "boolean", "description": "Affects the StepByStep transition behaviour<br/>If not set, inherits the setting from its parent FSM" },
"StepByStep": { "type": "boolean", "default": true, "description": "Affects the StepByStep transition behaviour<br/>If not set, inherits the setting from its parent FSM" },
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 unrelated to the PR but this documentation is actually wrong:

  • StepByStep defaults to true for the main executor and false otherwise
  • StepByStep does not inherits the setting from its parent FSM

Warning this may cause subtle behaviour changes if the controller relied
on having the FSM automatically reset the posture between each states in
a Meta state
@arntanguy
Copy link
Collaborator Author

However, I think the default should be reconsidered:

  1. True for the main executor
  2. False for Meta executors

Done, I agree this is a saner default. This might cause subtle issues in existing controllers as this changes the default reset behavior of all Meta states. I don't think this is a serious concern though, and we're better off with a good default for the future.

@gergondet gergondet merged commit 6f92db8 into jrl-umi3218:master Sep 13, 2023
19 checks passed
@mmurooka
Copy link
Member

This might cause subtle issues in existing controllers as this changes the default reset behavior of all Meta states.

Thanks for the update. Could you elaborate a bit more on how the behavior could change?

@gergondet
Copy link
Member

Thanks for the update. Could you elaborate a bit more on how the behavior could change?

Before this PR the posture task would be reset at every transition in a Meta state with no way to disable the behavior (other than removing the FSM posture tasks)

After this PR, this only happens (by default) for the main FSM and not for nested FSM. The old behavior can be re-enabled by setting ResetPostures: true in the Meta FSM

@mmurooka
Copy link
Member

Thank you, I understood that the behavior does not change unless nested FSM is used.

gergondet added a commit that referenced this pull request Sep 19, 2023
Added
---

- [mc_control] Added motor status to joint sensor (#395)
- [mc_control/FSM] Posture tasks' reset can be disabled (#389)
- [mc_rtc/Configuration] Added support for `std::variant` (#393)
- [mc_rtc/Configuration] Added `Configuration::find` (#393)
- [mc_rtc/GUI] Added form elements to provide more complex forms (#394)

Changes
---

- [mc_control/FSM] FSM embedded in a Meta state no longer reset the posture at transition by default (#389)

Fixes
---

- [mc_observers] KinematicInertial uses the correct function from state-observation (#391)
- [mc_solver] Fix a crash in monitor activation
- [mc_tvm] Correctly include refAccel in Orientation|PositionFunction
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.

3 participants