ROS2: Remove need to specify vehicle type param #88
Conversation
icolwell-as
requested review from
shan-as,
dvangeyn-as,
jilinzhouas and
kmushty-as
src/pacmod3_node.cpp
Outdated
| void PACMod3Node::initializeBrakeMotorRptApi() | ||
| { | ||
| can_pubs_[BrakeMotorRpt1Msg::CAN_ID] = | ||
| this->create_publisher<pacmod_msgs::msg::MotorRpt1>("parsed_tx/brake_rpt_detail_1", 20); |
There was a problem hiding this comment.
Similar to Jilin on the other pull request, consider adding 'using namespace pacmod_msgs::msg' in this file and then not having to repeat pacmod_msgs::msg here and in all other similar locations.
There was a problem hiding this comment.
See discussion in other PR.
| initializeVehicle4SpecificApi(); | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we need a default to catch any mismatching CAN_ID?
There was a problem hiding this comment.
There's no functional need, as the callback_can_tx function will just skip/drop the CAN_ID that doesn't have a matching publisher. I don't want to add a ROS_WARN or anything either in case the customer is using a newer DBC or something, which could possibly result in constant ROS_WARN spam for CAN messages the driver doesn't support yet.
| @@ -80,6 +80,26 @@ class PACMod3Node final | |||
| LNI::CallbackReturn on_error(const lc::State & state) override; | |||
|
|
|||
| private: | |||
| void initializeBrakeMotorRptApi(); | |||
There was a problem hiding this comment.
~PACMod3Node();
the destructor should be virtual.
There was a problem hiding this comment.
See pacmod3_node.cpp, there is an explicit destructor since a thread needs to be joined.
Resolves #85 by adding pub/sub groups specific to each PACMod module. They are simply initialized if CAN reports are sent from the PACMod module, otherwise they are not initialized. This removes the need to specify which vehicle platform you are using, since the driver will conform automatically.
There is probably some better way to code this using templates, but this works for now. Further simplification can be explored later.
Tested today with Lexus.