Workspace with Husky for Simulation and Real Robot Control #10
Conversation
There was a problem hiding this comment.
Thank you so much for this patch!
Aside from the individual comments, please:
- Separate copying/moving/re-formatting/fixes into different commits.
- When copying (all or a subset of) files from another repo, be explicit about the commit ids by adding permalinks (i.e., including the commit id in the links). These commits should only include a direct copy, and should not be mixed with any re-formatting or modifications. Please amend the following commits to make the link explicit and separate modifications:
To amend these commits, you may want to use interactive rebase (e.g.,
git rebase -i HEAD~15). See this post if you are not familiar with the commands: https://stackoverflow.com/q/1186535- 6a31993: https://github.com/husky/husky/tree/1e0b1d14d657f04ec3a86e73d6676a2cf7af6f79
- 2e4060b: https://github.com/clearpathrobotics/LMS1xx/tree/90001ac2849c9f6ab9ca9bbfeb816f9b66f966de (I'm not sure about this permalink, since the file contents are different...)
- 5dbbfe5: https://github.com/ros-planning/navigation2/blob/2de3f92c0f476de4bda21d1fc5268657b499b258/nav2_bringup/bringup/params/nav2_params.yaml (I'm not sure about this permalink, since the file contents are different...)
- When moving and modifying/re-formatting many files, it is preferred to isolate the moves/re-format to a dedicated commit, and store the modifications in a separate commit. Otherwise, moving and modifying the files simultaneously may break file history. (See: 7a69eaf#diff-61ed84a60fe1f3a3ca688aa7c509713f5a5addca51d4ab1245af4fae904ec893 ) It may be OK to move a few files and make small changes simultaneously in a single commit, as long as the changes are small and obvious. If there's even the slightest chance of causing confusion, simply separate the changes into different commits.
- As for fixes, it is strongly recommended to keep each commit as simple as possible, even if the fix only requires a few line of changes. It would be even better to include detailed description regarding the corresponding error message, related references for the fix, and the rationale of the fix. See 9da8ce9 and 12f6889 for some examples. (Feel free to use more commits, as adding more commits will not result in any additional charges :) )
- The fundamental principle here is to carefully inspect all changes (potentially with VSCode diff viewer) before adding the file, and include as much relevant information as possible before committing (See: https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=6e96d8a9702d1fef3eb6de47bb84bdea578eb1bb : 20+ lines of description for a one-line change is totally acceptable, and even preferred if all provided information are relevant). The included information should be sufficient for an outsider to understand your modifications just by viewing the commit description and changes. It is also recommended to clean up the commit history before opening a PR (potentially by performing an interactive rebase).
- When copying (all or a subset of) files from another repo, be explicit about the commit ids by adding permalinks (i.e., including the commit id in the links). These commits should only include a direct copy, and should not be mixed with any re-formatting or modifications. Please amend the following commits to make the link explicit and separate modifications:
- Minor: Change the directory name from
husky-wstohusky_ws(or maybehusky-sim_ws) by following the style oftemplate_ws.
There are issues in several commits, and should be amended accordingly:
- Commit 2e4060b: Please isolate the re-formatting / modifications into another commit.
husky-ws/src/husky/LMS1xx/urdf/sick_lms1xx.urdf.xacrois said to be directly copied from https://github.com/clearpathrobotics/LMS1xx/blob/humble-devel/urdf/sick_lms1xx.urdf.xacro . However the contents differ.husky-ws/src/husky/LMS1xx/package.xmlis said to be directly copied from https://github.com/clearpathrobotics/LMS1xx/blob/humble-devel/package.xml . However there are differences in the indentation/whitespaces.husky-ws/src/husky/LMS1xx/CMakeLists.txtis said to be directly copied from https://github.com/clearpathrobotics/LMS1xx/blob/humble-devel/CMakeLists.txt . However the contents differ.
- Commit dbcf1a5: Please separate the simple re-formatting (quotes/double-quotes, the use of
M_PI) into another commit, and leave the actual (minimal) fixes in the current commit.- There are many changes made in this commit as seen in dbcf1a5 . However, there is almost no description on why these changes are necessary, and what issue do they fix. These changes certainly cannot be understood by an outsider, and is definitely not debuggable.
- If possible, separate the fixes into individual commits based on the error message / issue they fix, and provide description on the rationale of these changes. It is totally acceptable if these fixes need to be separated into 20+ commits.
If you have any additional descriptions or references, please also include them in the commit history or file comments. Please also remove any redundant changes to prevent potential bugs. I would greatly appreciate it if this patch could include as much detail as possible.
The comments of this review may seem a bit too obsessive about maintaining a clean commit history. However, please note that this patch is based on many third-party code. It is always beneficial to take extra caution when copying/modifying third-party code. We need to maintain the ability to patch/debug these third-party code in the future. Consider the case where there seems to be a bug in the third-party code sometime in the future:
- If we want to check whether the third-party has provided corresponding patches, we will need the latest commit id of the third-party code included in this repo to quickly determine which patches haven't been included.
- If we want to apply the latest patches, we do not want to deal with broken file history or large changes. We would prefer minimal changes on third-party code.
- If we are unsure about the cause of the bug, detailed commit description for each modifications can help us identify whether the bug is caused by our changes, or is due to the third-party code.
- Etc.
In short, investing extra time in commit time can result in substantial time savings in the future. If there are any undocumented changes in this patch, there is a high chance that we will lose the ability to debug these third-party code and end up reverting this patch and re-implement it again in the future.
The comments in this review corresponds to the 7 commits within commit 352c714 and commit dbcf1a5 . I haven't reviewed the rest of the commits yet, but I believe they may contain related issues and should be fixed accordingly before re-requesting review.
Please tidy up the commit history, apply a force-push, and re-request review. Thank you!
husky-ws/src/husky/husky_description/urdf/decorations.urdf.xacro
Outdated
Show resolved
Hide resolved
YuZhong-Chen
force-pushed
the
husky_sim
branch
2 times, most recently
from
26037d1
to
0097270
Compare
Note that there is a file named "install" in husky/husky_bringup/scripts directory, which has the same name as the ros2 building folder. The rule in gitignore will ignore the file, so it needs to be added alone. Reference : https://github.com/husky/husky/tree/1e0b1d14d657f04ec3a86e73d6676a2cf7af6f79
The command finds the packages: `rosdep install --from-paths src --ignore-src --rosdistro humble -y` Using the command inside the container after creation will take much time. You will need to install those packages every time you create a container from the image. It is better to install these packages in the image by modifying the Dockerfile.
…sky_viz. The change in this commit is not necessary, but it will improve code readability.
The default LiDAR of husky is LMS1xx. For testing the correction of the code, I follow the default now. It will be changed to VLP-16 later when I finish the basics. Reference: https://github.com/clearpathrobotics/LMS1xx/tree/90001ac2849c9f6ab9ca9bbfeb816f9b66f966de
Since we won't use this LiDAR in reality, there is no need to compile the driver for it. We only need the LiDAR model for testing.
Uncomment the settings in urdf to support laser_enabled and realsense_enabled.
The origin launch file will launch Husky's description alone, this may cause asynchronous to other repo. So I modify the code to reuse the launch file in the husky description.
The original repository only supports ROS1, and migrating it to ROS2 is too challenging. I will create a new repository to maintain the same functionality.
Set rate to 5. Warning Message: [async_slam_toolbox_node-9] [INFO] [1697282044.282229935] [slam_toolbox]: Message Filter dropping message: frame 'base_laser' at time 1305.200 for reason 'discarding message because the queue is full'
Reference : - https://navigation.ros.org/configuration/packages/configuring-amcl.html - https://navigation.ros.org/configuration/packages/configuring-bt-navigator.html - https://navigation.ros.org/configuration/packages/configuring-costmaps.html - https://github.com/ros-planning/navigation2_tutorials/blob/master/sam_bot_description/config/nav2_params.yaml
Remove the setting of QoS.
In order to debug the data, I config a new rviz param. Reference: https://navigation.ros.org/setup_guides/odom/setup_odom.html
In order to simulate in a city. Reference: https://github.com/osrf/citysim/tree/3928b08e2598f5ead2e9b24640fe1bde262a136d
The original repository is not designed for ROS, so the package structure differs from a typical package. I have deleted all the CMakeLists.txt files in the repository to prepare for creating a ROS-like package in the future.
Add the CMakeLists.txt and package.xml files to the folder to indicate that it is a ROS package. Note that I didn't compile the plugins in the CMakeLists.txt. This is because we won't be using those plugins.
Since we will be using `colcon` to build the package, there is no need for the "README.md" that explains how to build or update the package. I have removed all files related to it.
Remove the plugins that we won't use.
I choose to rename `imu/data_raw` to `imu/data`. This is because data published by the gazebo imu sensor plugin includes orientation data, which means it is already post-processed. Although `imu/data_raw` is not required in simulation, it'll be used in real-world deployment. The `imu_filter_madgwick` package is used to convert raw IMU data (without orientation) into a post-processed IMU data (with orientation). Refs: * husky/husky#113 * http:https://wiki.ros.org/imu_filter_madgwick I believe this topic name mismatch issue is due to the following commit: Ref: husky/husky@e379ee8
There is a `/odom` TF but no `/odom` topic. This fixes all typos on subscribing to the `/odom` topic.
Since husky is nonholonomic, we do not need to fuse the Y data provided by the IMU as described in the `robot_localization` document. I simply follow the odom & imu configs provided by it. Ref: http:https://docs.ros.org/en/melodic/api/robot_localization/html/configuring_robot_localization.html#fusing-unmeasured-variables
To address the issue of pose data from slam_toolbox occasionally jumping over time, it's advisable not to directly fuse it into ekf_node. In order to prevent the covariance from growing excessively, we can add configuration for `dynamic_process_noise_covariance` in the `robot_localization` package. This will enable the covariance to be dynamically scaled. Reference: - https://docs.ros.org/en/melodic/api/robot_localization/html/state_estimation_nodes.html#dynamic-process-noise-covariance Co-authored-by: Johnson Sun <[email protected]>
Note that commit 92a8b70 doesn't correcly fix this issue
Note that in order to use `docker compose` on Jetson, may need to install latest docker & NVIDIA container toolkit manually Ref: dusty-nv/jetson-containers#354 Ref: https://github.com/BretFisher/multi-platform-docker-build?tab=readme-ov-file#add-dockerfile-logic-to-detect-the-platform-it-needs-to-use
There was a problem hiding this comment.
Simulation code works as expected; Real robot control code works well on x86 system, but currently have docker building issues on arm system (Jetson AGX Orin).
-
The
docker composecommand requires installing the latest docker & NVIDIA container toolkit on Jetson AGX Orin. -
I recall that the arm system error includes the following when executing
clearpath_computer_installer.sh:E: Unable to locate package ros-humble-clearpath-robot
Will continue fixing it tomorrow.
2024/05/29 Update:
The issue is due to ros-humble-clearpath-robot binaries are only available on amd64 but not on arm64.
The straightforward approach is to install package from source when on arm64: https://github.com/clearpathrobotics/clearpath_robot
Will check if this approach works.
|
Current version can reliably control real husky by either amd64 or arm64 system. @YuZhong-Chen please help review my commits or perform further changes. Please re-request a review when no further changes are required, and I'll go ahead and merge this PR. Thanks! |
… correctly. If we intend to use a built-in global variable in a Dockerfile, such as `TARGETARCH`, we should specify it initially using `ARG TARGETARCH`. This ensures that the Docker recognizes and properly uses the variable throughout its execution. Reference: - https://docs.docker.com/reference/dockerfile/#automatic-platform-args-in-the-global-scope - https://docs.docker.com/compose/compose-file/build/#platforms
|
Sorry for the late response. |
|
Thanks for the fix! Could you please help check if I missed out any code license we've included in this PR? (see the latest commit) In addition, should the LiDAR LMS1xx be replaced with VLP-16 LiDAR in this PR? Or is it ok to defer it to another PR some time in the future? |
|
I think one license is missing: the license for the udev rule for Husky. See commit bf09fe1, which copies the udev rule from here. Since it's a different repository, we may need to add its license as well. |
|
Additionally, regarding the replacement of the LiDAR with the VLP-16, I think we can merge this PR first for the convenience of merging other branches. After I finish testing in a few days, I will open a new PR that includes both the VLP-16 and ZED2 to match the current configuration of the actual machine. |
Closes: #3 #17