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

feat(tier4_control_msgs): add accept start interface #53

Merged
merged 3 commits into from
Sep 20, 2022

Conversation

isamu-takagi
Copy link
Contributor

@isamu-takagi isamu-takagi commented Sep 8, 2022

Signed-off-by: Takagi, Isamu isamu.takagi@tier4.jp

Related Links

autowarefoundation/autoware.universe#1808

Description

Add the accept start message to hook vehicle starting.

Remarks

Pre-Review Checklist for the PR Author

PR Author should check the checkboxes below when creating the PR.

  • Code is properly formatted
  • Assign PR to reviewer

Checklist for the PR Reviewer

Reviewers should check the checkboxes below before approval.

  • Commits are properly organized and messages are according to the guideline
  • Code is properly formatted
  • PR title describes the changes

Post-Review Checklist for the PR Author

PR Author should check the checkboxes below before merging.

  • All open points are addressed and tracked via issues or tickets
  • Write release notes

CI Checks

  • Build and test for PR: Required to pass before the merge.
  • Check spelling: NOT required to pass before the merge. It is up to the reviewer(s). See here if you want to add some words to the spell check dictionary.

Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp>
@@ -0,0 +1,2 @@
builtin_interfaces/Time stamp
bool data
Copy link
Contributor

Choose a reason for hiding this comment

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

When will this data be true/false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. How about asking @maxime-clem about the naming?
From the word WillMove, I personally feel it's one time only.

Copy link
Contributor

@maxime-clem maxime-clem Sep 9, 2022

Choose a reason for hiding this comment

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

I am sorry I am not familiar with the PauseInterface.
What I understand:

  • The command gate can be "paused" or "unpaused".
  • If "unpaused", the control command is used.
  • If "paused", the control command is no longer used.
  • If "paused", a stopping command is used.
  • In both cases, a message is published to indicate if the control command has a velocity of 0 or not.

What do you think about changing WillMove to something like MovingControlCmd or StoppedControlCmd (true if velocity = 0) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kenji-miyake @maxime-clem
Thank you. That's right. This is a function that pauses the vehicle to insert processing when the vehicle starts. IsPaused is a pause state and WillMove is used to check if the vehicle is about to depart.

I feel MovingControlCmd is a command rather than a bool. So how do you feel about IsRequestingDeparture or IsStartRequesting?

Copy link
Contributor

Choose a reason for hiding this comment

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

IsRequestingDeparture sounds good to me, but then should we also rename the state name? 🤔
Or IsStartRequesting might be acceptable.

Copy link
Contributor

@maxime-clem maxime-clem Sep 12, 2022

Choose a reason for hiding this comment

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

Sorry my suggestion was not good and I understand how it could be confused with a control command 😞
My issue with IsRequestingDeparture (and IsStartRequesting) is that it sounds like the command gate is the one requesting departure. However, the message informs about the state (requesting to move or not) of the input of the command gate.

I would thus suggest to change to isDepartureRequested.

Signed-off-by: Takagi, Isamu <isamu.takagi@tier4.jp>
@isamu-takagi isamu-takagi mentioned this pull request Sep 20, 2022
7 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.

3 participants