-
Notifications
You must be signed in to change notification settings - Fork 0
Add a build script for wx_armor #25
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
Conversation
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.
Thanks for the nice PR. A couple suggestions if they make sense.
README.md
Outdated
|
||
Users must also set `LD_LIBRARY_PATH` to the shared library locations. In your `~/.bashrc`, add the following line: | ||
```bash | ||
export LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH |
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.
Maybe add a check in build.sh to add this to .bashrc if not already there?
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 tried doing this, but since this build script is being run using sudo
, it uses the root's .bashrc instead. Since there's no easy fix for this, I simply omitted it. Instead, I have the part about setting LD_LIBRARY_PATH
be the first step in README.
- `sudo apt install -y yaml-cpp-dev` | ||
|
||
#### Building From Source | ||
|
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.
Add some explanation about the system requirements, e.g. ubuntu version tested etc..
Also add a link to our other repo for running on Jetson devices.
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.
Good point. 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.
Hey Le, thanks for the review. Addressed your comments. PTAL.
I can also confirm that the build script works flawlessly on Ubuntu 24.04, in addition to 22.04.
- `sudo apt install -y yaml-cpp-dev` | ||
|
||
#### Building From Source | ||
|
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.
Good point. Done.
README.md
Outdated
|
||
Users must also set `LD_LIBRARY_PATH` to the shared library locations. In your `~/.bashrc`, add the following line: | ||
```bash | ||
export LD_LIBRARY_PATH=/usr/local/lib:$LD_LIBRARY_PATH |
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 tried doing this, but since this build script is being run using sudo
, it uses the root's .bashrc instead. Since there's no easy fix for this, I simply omitted it. Instead, I have the part about setting LD_LIBRARY_PATH
be the first step in README.
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.
LG. Btw, is there any instruction about udev rules? wx_armor fails to talk to the arm w/o the udev rules to define /dev/ttyDXL
The merge-base changed after approval.
7e5238d
to
89a076f
Compare
Forced pushed a rebase to main @le-horizon |
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.
Thanks for the automation. Some suggestions to make the process even more automatic.
build.sh
Outdated
# Install json | ||
cd /tmp | ||
rm -rf json | ||
git clone https://github.com/nlohmann/json | ||
cd json | ||
mkdir build | ||
cd build | ||
cmake .. | ||
make -j"$(nproc)" | ||
make install | ||
cd /tmp | ||
rm -rf json | ||
cd "$HOME_DIR" |
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.
This will do:
sudo apt-get install -y nlohmann-json3-dev
Also need these:
sudo apt-get install -y udev libyaml-cpp-dev libspdlog-dev gcc g++ cmake libjsoncpp-dev uuid-dev zlib1g-dev
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.
Done.
@QuantuMope could you work on this PR so we can close #33? Also need to avoid using |
f1cfc45
to
5cddb5e
Compare
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.
Hey Le, addressed all comments. Also, added a cmake fix to avoid the .bashrc LD_LIBRARY_PATH setting.
Tested that everything works on gail7.
build.sh
Outdated
# Install json | ||
cd /tmp | ||
rm -rf json | ||
git clone https://github.com/nlohmann/json | ||
cd json | ||
mkdir build | ||
cd build | ||
cmake .. | ||
make -j"$(nproc)" | ||
make install | ||
cd /tmp | ||
rm -rf json | ||
cd "$HOME_DIR" |
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.
Done.
b59ae20
to
22fcc66
Compare
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.
Just one question. Thanks for the automation!
spdlog::spdlog | ||
nlohmann_json::nlohmann_json | ||
yaml-cpp) | ||
yaml-cpp::yaml-cpp) |
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.
dynamixel_sdk dependency seems to be missing, have you tested CMake w/o LD_LIBRARY_PATH?
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.
Yeah I have. I unset it. I don't think that dependency existed before in the cmake. Could be that toolbox refers to sdk in its own cmake.
This PR adds an automated build script for
wx_armor
. It's quite annoying / tedious to have to install it on fresh machines. This script should now handle all necessary dependencies seamlessly.Tested on two separate machines.
An old tutorial can be found here for reference.
https://horizonrobotics.feishu.cn/wiki/NYL6wgBc8i05O0kLkk2cqFNxn2g