-
Notifications
You must be signed in to change notification settings - Fork 0
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
49 crp standardize control class #70
base: main
Are you sure you want to change the base?
Conversation
Fetch from Main
Fetch mods from main
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.
Some coding rules changes:
- the class definition should be in a separate .hpp header file
- also all the includes (except the header) should be in the header file
- some naming conventions
- also file names should use cammelCase too
...trollers/ctrl_vehicle_control_lat_compensatory/src/ctrl_vehicle_control_lat_compensatory.cpp
Outdated
Show resolved
Hide resolved
...trollers/ctrl_vehicle_control_lat_compensatory/src/ctrl_vehicle_control_lat_compensatory.cpp
Outdated
Show resolved
Hide resolved
crp_APL/ctrl_vehicle_control/include/ctrl_vehicle_control/ctrl_vehicle_control.hpp
Outdated
Show resolved
Hide resolved
} | ||
|
||
twist_msg.angular.z = msg->steering_tire_angle; | ||
ctrl_msg.lateral.steering_tire_angle = msg->steering_tire_angle; | ||
ctrl_msg.lateral.steering_tire_rotation_rate = 0.0f; |
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 think this should be the value from the message instead of 0. We can always set it to 0 in the lat controller but if we need to use this value than this would block it.
…ctrl_vehicle_control_lat_compensatory.cpp Co-authored-by: Józsa Dávid <44705915+AnonymDavid@users.noreply.github.com>
…ctrl_vehicle_control_lat_compensatory.cpp Co-authored-by: Józsa Dávid <44705915+AnonymDavid@users.noreply.github.com>
…_vehicle_control.hpp Co-authored-by: Józsa Dávid <44705915+AnonymDavid@users.noreply.github.com>
@AnonymDavid, done |
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.
Lat controller still does not have its headers and includes separated to a header file.
i guess it's good for now @AnonymDavid |
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.
minor coding rule related changes needed
...tory/include/ctrl_vehicle_control_lat_compensatory/ctrl_vehicle_control_lat_compensatory.hpp
Outdated
Show resolved
Hide resolved
...tory/include/ctrl_vehicle_control_lat_compensatory/ctrl_vehicle_control_lat_compensatory.hpp
Outdated
Show resolved
Hide resolved
...tory/include/ctrl_vehicle_control_lat_compensatory/ctrl_vehicle_control_lat_compensatory.hpp
Outdated
Show resolved
Hide resolved
m_pub_cmd = this->create_publisher<autoware_control_msgs::msg::Lateral>("/control/command/control_cmdLat", 30); | ||
|
||
traj_sub = this->create_subscription<autoware_planning_msgs::msg::Trajectory>("/plan/trajectory", 10, std::bind(&CtrlVehicleControlLat::trajCallback, this, std::placeholders::_1)); | ||
ego_vehicle_sub = this->create_subscription<crp_msgs::msg::Ego>("/ego", 10, std::bind(&CtrlVehicleControlLat::egoVehicleCallback, this, std::placeholders::_1)); |
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.
accordingly per the other comments
crp_APL/ctrl_vehicle_control/include/ctrl_vehicle_control/ctrl_vehicle_control.hpp
Outdated
Show resolved
Hide resolved
} | ||
|
||
void crp::apl::ControlHandler::run() | ||
{ | ||
m_pub_twist_->publish(twist_msg); | ||
ctrl_msg.stamp = this->now(); |
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.
shall we add the same stamp to the twist message 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.
the twist msg has no time stamp
…ude/ctrl_vehicle_control_lat_compensatory/ctrl_vehicle_control_lat_compensatory.hpp Co-authored-by: Ignéczi Gergő <51396141+gfigneczi1@users.noreply.github.com>
…ude/ctrl_vehicle_control_lat_compensatory/ctrl_vehicle_control_lat_compensatory.hpp Co-authored-by: Ignéczi Gergő <51396141+gfigneczi1@users.noreply.github.com>
…ude/ctrl_vehicle_control_lat_compensatory/ctrl_vehicle_control_lat_compensatory.hpp Co-authored-by: Ignéczi Gergő <51396141+gfigneczi1@users.noreply.github.com>
…_vehicle_control.hpp Co-authored-by: Ignéczi Gergő <51396141+gfigneczi1@users.noreply.github.com>
@gfigneczi1 check again, corrected the required changes |
Added control class cleanup