Port four bar linkage and differential transmission loaders from ROS1 #656
Conversation
github-actions
bot
requested review from
bijoua29,
bmagyar,
DasRoteSkelett,
destogl,
kellyprankin,
mhubii and
peterdavidfagan
VX792
force-pushed
the
port-four-bar-linkage
branch
from
9a9dcd9 to
ea4a878
Compare
VX792
force-pushed
the
port-four-bar-linkage
branch
from
ea4a878 to
f2a508b
Compare
VX792
changed the title
| std::shared_ptr<transmission_interface::Transmission> transmission; | ||
| const hardware_interface::TransmissionInfo & info = infos[0].transmissions[0]; | ||
| transmission = transmission_loader->load(info); | ||
| ASSERT_TRUE(nullptr != transmission); |
There was a problem hiding this comment.
Please check best practices comment.
There was a problem hiding this comment.
The best practices comment says that either the comparison ASSERT_TRUE(nullptr != transmission) is redundant, or there is possible null pointer dereference. I don't think there's a case where the transmission_loader would be a nullptr, so should I just leave out these comparisons?
|
|
||
| std::shared_ptr<transmission_interface::Transmission> transmission = nullptr; | ||
| const hardware_interface::TransmissionInfo & info = infos[0].transmissions[0]; | ||
| transmission = transmission_loader->load(info); |
There was a problem hiding this comment.
Aren't we throwing an exception here in SimpleTransmission? If so we should do the same.
| {jnt_offset1, jnt_offset2})); | ||
| return transmission; | ||
| } | ||
| catch (const Exception & ex) |
There was a problem hiding this comment.
this doesn't capture the exception from .at(), do the tests exercise anything that'd produce that? can we even end up with a situation like that ATM?
There was a problem hiding this comment.
Yes, for example if we have one actuator and 2 joints in our urdf. I've just tested it and the test case fails with
C++ exception with description "vector::_M_range_check: __n (which is 1) >= this->size() (which is 1)" thrown in the test body.
The initial SizeIs checks passes, and in the load function we try to access the nonexistent
transmission_info.actuators.at(1).mechanical_reduction; variable. Is it okay if I catch an std::exception type here?
| RCLCPP_ERROR( | ||
| rclcpp::get_logger("differential_transmission_loader"), | ||
| "Failed to construct transmission '%s'", ex.what()); |
There was a problem hiding this comment.
do we want RLCPP_ERROR or std::cerr here like the other classes? I believe std::cerr was used to stop a crash, why is this not causing a crash in the test?
bmagyar
force-pushed
the
port-four-bar-linkage
branch
from
844c78f to
9483385
Compare
* Add missing backward_ros dependency and minor cmake cleanup * Fix unused arg warning (cherry picked from commit d96f662) Co-authored-by: Bence Magyar <bence.magyar.robotics@gmail.com>
Plus minor changes to parsing
Should I create test cases with invalid roles, since they were present in the ROS1 version?
Issue: #599