Refactor the robotic manipulation package #11
Conversation
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
jhanca-robotecai
force-pushed
the
kd/code_refactor
branch
from
bba007b to
206c68c
Compare
|
Rebased and force-pushed your branch after integrating other changes to the repository. |
| rclcpp::executors::SingleThreadedExecutor m_executor; | ||
|
|
||
| std::vector<double> m_startingPose; | ||
| }; |
There was a problem hiding this comment.
Which configuration of clang-format did you use? It is quite strange the tool did not add a line break here.
Please add the configuration file to the repository to ensure every developer uses the same one. I suggest using the same one as in other projects within this namespace.
There was a problem hiding this comment.
I added a configuration file I found in the RAI repository and reformated the files again.
| target_include_directories(robotic_manipulation PUBLIC | ||
| $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include> | ||
| $<INSTALL_INTERFACE:include>) | ||
| target_compile_features(robotic_manipulation PUBLIC c_std_99 cxx_std_17) # Require C99 and C++17 | ||
| target_compile_features(robotic_manipulation PUBLIC c_std_99 cxx_std_20) # Require C99 and C++20 |
There was a problem hiding this comment.
Is there any particular reason for c++20?
There was a problem hiding this comment.
std::numbers::pi is the reason. Before C++20, no standard constant for π was defined.
| auto logger = m_node->get_logger(); | ||
|
|
||
| const int NumTries = 10; | ||
| int const NumTries = 10; |
There was a problem hiding this comment.
You might have used constexpr for such value, hence the pre-compiler could use it directly in the code.
There was a problem hiding this comment.
Replaced const with constexpr.
|
|
||
| class RaiManipulationInterfaceNode { | ||
| public: | ||
| RaiManipulationInterfaceNode(); |
There was a problem hiding this comment.
What happened to the destructor? Don't you need to destroy the services anymore?
There was a problem hiding this comment.
The services will get destroyed automatically when the shared_ptrs stored in this class get destroyed. Previously the destructor was here to shut down the executor thread, which wasn't even used since we spin the node in the main thread now, so I removed it.
| // The joint values for the gripper to be open and closed. | ||
| double const OpenGripperJointValue = 0.038; | ||
| double const ClosedGripperJointValue = 0.002; | ||
|
|
||
| // The base orientation of the end effector resulting in the gripper pointing | ||
| // straight down. | ||
| double const EndEffectorBaseRoll = 0.0; | ||
| double const EndEffectorBasePitch = std::numbers::pi; | ||
| double const EndEffectorBaseYaw = tf2Radians(45.0); |
There was a problem hiding this comment.
Can we define these within the header of the class in the private section (as static constexpr)? Or in the anonymous namespace? We do not want to populate it everywhere in the code.
There was a problem hiding this comment.
I moved these constants into the class header and made them constexpr.
| @@ -12,16 +26,20 @@ int main(int argc, char *argv[]) { | |||
| auto armController = std::make_shared<ArmController>(); | |||
| armController->Initialize(); | |||
|
|
|||
| armController->MoveThroughWaypoints({armController->CalculatePose(0.3, 0.0, 0.35)}); | |||
| armController->MoveThroughWaypoints( | |||
There was a problem hiding this comment.
Some magick numbers left here.
Signed-off-by: Kacper Dąbrowski <[email protected]>
knicked
force-pushed
the
kd/code_refactor
branch
from
5877993 to
b9152fa
Compare
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
Signed-off-by: Kacper Dąbrowski <[email protected]>
knicked
force-pushed
the
kd/code_refactor
branch
from
1389dec to
131bb20
Compare
This PR refactors the robotic manipulation package to improve the quality of the code. In particular, this PR:
StateControllerclass to better mirror its purposepackage.xml