-
Notifications
You must be signed in to change notification settings - Fork 52
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(control-messages): add control messages #11
Conversation
How about adding a guide for enums? I considered enums here because I'm often confused by message containing multiple enums. For example:
|
@kenji-miyake is there a simple way to not have dependency_ws in build process? Do we need to edit the workflow? https://github.com/autowarefoundation/autoware_msgs/runs/6098081703?check_suite_focus=true#step:7:29 |
Thanks for reminding, we can add from official ros2 messages for an example, will add it to the guidelines.
|
@xmfcx There are two ways.
Either one is okay for me, but I personally prefer the No.2 because it's easier to maintain. |
4610103
to
5bc1db1
Compare
@xmfcx create a message for velocity, acceleration, jerk, steering, steering_rate limits. |
@xmfcx @mitsudome-r Thanks for the great work. I have a few thoughts after mtg and make some additional comments. First,
The reason for this is that the Secondly. (Sorry for a long comment, but I think it is essential.) In the mtg, it is mentioned that the target acceleration is the derivative of the target velocity. But it may be misleading. For example, let's say a vehicle is driving at 50 km/h and Autoware wants the vehicle to be 60 km/h. The velocity profile will then look like this. In this figure, the derivative of the target velocity array is 0, but obviously, the acceleration we want to achieve is not zero, should be some positive value. In the current implementation of .universe, the Some people may think it is strange to suddenly give a target speed of 60 km/h when the vehicle's velocity is 50 km/h. The velocity profile in that case is probably like this. In this case, the target acceleration matches the derivative of the target velocity. But if the Someone may also think, what if we increase the target speed a little so that the FB control works? Actually, this idea is often used in practice. However, in this case, if the vehicle moves according to the derivative of the target velocity array, the vehicle velocity will never catch up to the target velocity, as shown by the blue dots. Supporting multiple interfaces (target speed, acceleration) causes this kind of confusion. The current implementation of .universe follows the first policy (first picture). The planning module calculates a kinematically feasible velocity profile, but the same situation as in the first figure can occur if the low_level_controller's velocity control performance is very poor. The current implementation is based on the idea that "when the In summary, "the derivative of the target velocity is not the target acceleration", or I can say, "the target acceleration cannot be calculated by simply differentiating the target velocity". It may cause a confusion for users. In addition, the target jerk can not be calculated by just differentiating the target acceleration as well. To calculate the target jerk, it is needed to add a new feedback control logic. In that case, is the So, the discussion point would be:
My opinion is:
On the other hand, there is no concern with having control_command_arrays. It will make a room for a |
f3357a1
to
541eff4
Compare
541eff4
to
f57d6da
Compare
b43b9af
to
e2fa0eb
Compare
@TakaHoribe wrote:
I agree, I will add this to |
@TakaHoribe I think we need to clarify the output of control and planning nodes and the input of actual vehicle controller. I want to share the ideal case first: Here, the control stack outputted the blue velocity/time curve. The facts we and the all the nodes know:
For consistency and conveying the right information, following should be done:For planning stack:
For control stack:
For low level controller / vehicle controller:
Here is a bad control example. If with this kind of curve being output from the controller (along with acceleration values I didn't draw to achieve these) the vehicle achieves that kind of real velocity curve in the end, there is something wrong going on. Also we shouldn't forget that these curves are being generated very frequently, about 50hz generally. If the vehicle ends up with that velocity error at time 100ms, then it has already failed. This shouldn't be possible if the control stack and vehicle controller functioned as expected. Also of course if we feed the first element of these to a PID controller naively, the vehicle will never move at all too. Picking the right input of a PID is the job of the controller that is utilizing the algorithm. In the end, the controller outputs a velocity/time curve which vehicle is expected to follow (and can follow) and also an acceleration/time curve which vehicle is expected to follow (and can follow) and if vehicle achieves those acceleration curve, it will end up with the same velocity curve. The arrays we are sending are just discretized versions of these curves. |
@TakaHoribe wrote:
This gray curve is an unrealistic expected velocity curve. At Even if target was 51 km/h, at For the acceleration, assuming vehicle was going at constant velocity 50 km/h, initial acceleration of the vehicle is 0 m/s². As the time progresses, the vehicle might press the gas pedal more and more and achieve a positive acceleration and reduce it at some point to 0 m/s² again. So expected acceleration array would be like: [0, 1, 2, 3, 2, 1, 0] m/s² (I am making up numbers). |
@TakaHoribe wrote:
This gray expected velocity curve is more realistic. But if you put the first value in this curve as the input of the PID controller (assuming error input is delta velocity, output is gas pedal level / throttle), the vehicle won't move. You should at least enter the v_1 - v_0 or something latter as error input. |
@TakaHoribe wrote:
Here the gray expected velocity curve is not realistic again. The vehicle cannot jump to 50 to 53 km/h at It might be what you directly feeding into the PID but that's an implementation detail. You are basically shifting the expected velocity curve from right to left to feed into PID. It is ok but we should pass the expected velocity curve and let the node use it as it likes to. If you take the derivative of the actual expected velocity curve, you will end up with expected acceleration curve.
Unifying the outputs of the planning and control stacks as being expected velocity/acceleration arrays from The unrealistic expectations from the nodes with hardcoded assumptions causes more confusion in my opinion. |
@xmfcx thank you for your reply. I think we should first clarify the role of planning/control/low_leverl_controller. I think it is useful to discuss:
Let's think about the velocity deviation caused by disturbances. Generally, the control module should be responsible for the suppression and correction of the error. For this problem, there are two popular approaches:
The current implementation of .universe uses the latter method from the standpoint of modularity, and the planning module does not perform calculations from self-position or velocity. Thus, this case arises frequently (10km/h deviation is too much though). My opinion is that the planning should not calculate from the ego-position/velocity as implemented in the current .universe. @xmfcx what do you think? |
@TakaHoribe wrote:
I agree, having multiple facilities responding to an error makes it hard to tune and might cause weird oscillations due to multiple feedback mechanisms responding to an error. My thoughts on the plannerPlannerIt should plan trajectories that can be followed by the vehicle. A trajectory is made of poses from where the vehicle is and where it will be in future, discretized with a fixed time step. A trajectory is very deeply attached to the planned velocity profile/curve for the vehicle. Because trajectory includes time information and there is only a single velocity profile possible to a matched trajectory. The trajectory should be planned with the current steering angle, steering rate, velocity, acceleration in mind. Otherwise the vehicle might not be able to follow the plan. Also the planner should be consistent, pessimistic and strict.
Roles of controller and vehicle controllerI’ve stated my opinions on these on previous post. What approach should we use?Reference to architecture design discussion: https://github.com/orgs/autowarefoundation/discussions/4 graph TD
subgraph motion planning & control
subgraph motion planning
oa[obstacle avoidance] --- pp(path planning)
pp --- tp(trajectory planning + velocity optimization)
end
tp --- c(control)
c --- vc(vehicle control)
end
@TakaHoribe wrote:
TEB (Timed Elastic Bands) approach combines all of these in a single optimization problem. Here is a comparison of TEB and MPC approaches. Although it would require a lot of changes to be used in our stack. Is this similar to the paper you sent? I’ve skimmed through the CMU paper but it seemed like it was similar to our modular approach. Maybe I misunderstood it. Could you explain? flowchart TD
subgraph current autoware motion planning & control
oa[obstacle avoidance] --- pp(path planning)
pp --> tp(trajectory planning + velocity optimization)
tp --> c(control)
c --> vc(vehicle control)
end
My concerns with our current approach is similar to yours:
What should be done then? Ideally, I'd expect some feedback pathways on the stack: flowchart TD
subgraph maybe like this?
subgraph pp[path planner]
bp[behavior planner] --- oa[obstacle avoidance]
oa --- pg(path generator)
end
pp --> tp(trajectory planning + velocity optimization)
tp --> c(control)
c --> vc(vehicle control)
vc -. can't follow, replan .-> c
c -. can't follow, replan .-> tp
tp -. can't follow, replan .-> pp
end
But even then I see so many problems arising. |
@xmfcx Just a quick reply:
Usually, the path of the teb_planner is followed by pure_pursuit type controller, right? In that case, it is not a similar approach to the paper I sent. The role of ego-position feedback exists in two modules, teb and pure_pursuit. If the output of the teb_planner is directly used to drive the robot, then it is the same approach as I mentioned as the CMU paper. The planning and control is in the one big module. |
Codecov Report
@@ Coverage Diff @@
## main #11 +/- ##
====================================
Coverage ? 0
====================================
Files ? 0
Lines ? 0
Branches ? 0
====================================
Hits ? 0
Misses ? 0
Partials ? 0 Continue to review full report at Codecov.
|
277c32e
to
b833ea2
Compare
@TakaHoribe @maxime-clem @kenji-miyake could you review it please? |
1deb0cb
to
8999307
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The messages look good to me and I only have some minor comments.
|
||
<license>Apache License 2.0</license> | ||
|
||
<buildtool_depend>ament_cmake_auto</buildtool_depend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In universe this is changed to autoware_cmake
.
- https://github.com/autowarefoundation/autoware_common/tree/main/autoware_cmake
- refactor: use autoware cmake autoware.universe#849
I am not sure if we want to change it here as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kenji-miyake do you have opinions on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I'm not so familiar with the dependency type, but I think we don't have to remove ament_cmake_auto
. We can just add another dependency of autoware_cmake
(if the package uses autoware_cmake
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will keep this one as ament_cmake_auto
. If it becomes necessary, we can do it later on.
autoware_control_msgs/package.xml
Outdated
<description>Autoware control messages package.</description> | ||
|
||
<maintainer email="mfc@leodrive.ai">M. Fatih Cırıt</maintainer> | ||
<maintainer email="opensource@apex.ai">Apex.AI, Inc.</maintainer> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is apex.ai
still involved ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maxime-clem they are not, as fas as I know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm maybe it is a better idea to add them with a NOTICE file since they've contributed to the _auto messages, which I am partially making use of.
Like in https://github.com/autowarefoundation/autoware/blob/main/NOTICE
What do you think about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a good idea to write in the NOTICE
file like These messages are based on _auto messages that are mainly developed by Apex.AI
. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 8c5e4f5
Regarding the message formats, I believe TIER IV is okay if it's okay for Taka and Maxime. |
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
8999307
to
eee2a73
Compare
We've discussed in the AWF architecture mtg. I totally agree with this message definition. |
4833b74
to
20cd819
Compare
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
Signed-off-by: M. Fatih Cırıt <mfc@leodrive.ai>
20cd819
to
35dbbe8
Compare
@TakaHoribe could you review it again please? I've separated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xmfcx Great description. LGTM!
Signed-off-by: M. Fatih Cırıt mfc@leodrive.ai
Description
This PR adds the message guidelines and first set of messages:
autoware_control_msgs
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.