-
Notifications
You must be signed in to change notification settings - Fork 18
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
Setup TX2 initialization #356
Conversation
robot/rover/RoverSetup.sh
Outdated
. ~/Programming/robotics-prototype/robot/rospackages/devel/setup.bash | ||
. ~/Programming/robotics-prototype/venv/bin/activate | ||
source ~/Programming/robotics-prototype/robot/basestation/config/.bash_aliases |
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.
Shouldn't you use $REPO for this?
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.
Fixed
robot/rover/RoverSetup.sh
Outdated
|
||
# Install Requirements | ||
pip install -U pip | ||
pip install pyserial |
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.
is there a reason why pip is installed here and not ran through pip -r requirements?
Also you don't include a version # which could be potentially bad
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.
Not all of the python libraries are required, so there's no point to create a requirements.txt file just for one library. I included the version # thanks
robot/util/emailer/emailIPAddress.js
Outdated
@@ -9,7 +9,8 @@ const EMAILS_FILE_NAME = 'emails.txt' | |||
// fallback to default emails if there's and issue with the emails file | |||
const DEFAULT_EMAILS = [ | |||
'[email protected]', | |||
'[email protected]' | |||
'[email protected]', | |||
'[email protected]' |
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.
lol pls add me
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 so you know, read this code to see how to place the email config file so that more members can be notified
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
@@ -1,5 +1,5 @@ | |||
#!/usr/bin/env bash | |||
HOME="/home/odroid" | |||
HOME="/home/nvidia" |
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.
why is it sometimes $USER sometimes nvidia
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.
is this related to what you put in the description?
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.
There are apparently issues with using $USER when running it as a systemd service. I'll try to changing it back to see if I can get to work tho
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.
Also it isn't related to what I put in the description. The description refers to some weird issue with connecting to ROS over a vpn server which has a workaround.
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.
The systemd service doesn't work if $USER is used anywhere because the service has to run under root for some reason. I haven't find a way to fix this yet
User=root | ||
Type=simple | ||
ExecStart=/home/odroid/Programming/robotics-prototype/robot/util/rosRoverStart/runRosRoverStart.sh | ||
ExecStart=/home/nvidia/Programming/robotics-prototype/robot/util/rosRoverStart/runRosRoverStart.sh |
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.
Shouldn't you be using $USER
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.
Won't work cause the systemd service has to run under root for some reason
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.
Thing is, you can change the service to run as non-root but then certain processes later in the pipeline will fail and need to be worked around. Some more digging can be done as to where specifically it messes up, but seeing as we're not going to be switching OBC super often I'd say it's fine to leave this as it is for now. Maybe we can open an issue to investigate this for after this PR is merged.
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 can try testing this out again for some investigation 👀 🍷
55d254c
to
ce944a5
Compare
I think Travis failed to download one of the requirements on the first test and failed lmao |
So how can we test this? You provided some steps but is the jetson always on? If not, can we coordinate a testing time? @KoaTheKoala did you ever test this since I know you did some jetson work |
@MartensCedric We would prob have to coordinate a testing time, unless if @KoaTheKoala runs the jetson 24/7. The setup script I made can be tested in VM to ensure that there's no errors. Although that doesn't guarantee that it would setup everything properly for the Jetson. |
I would also suggest we should include some teensys into the equation that are running the code for rover/arm/science modules, to at least verify if the comms between the MCUs/OBCs still function. |
Given that its a pain for anyone remote to test, maybe we could devise a test for Koa to perform, upon success of this test we could merge the PR |
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.
So for increasing the scope of testing, what can be done is connecting some teensies over USB with Rover/Arm/Science code on them, maybe even an arduino uno with PDS over UART comms, and test that they can communicate appropriately. As well, some usb cameras can be connected and tested.
I remember setting up the dependencies for the node emailer script was a pain, and I'm not seeing any of that setup here so that probably still needs to be accounted for as well as tested.
The changes look good, but really the best way forward for this issue I think would be for me to go visit @KoaTheKoala (since he currently has the jetson) and bring some teensies/cameras, do some further testing and confirm that things look good to merge.
robot/rover/RoverSetup.sh
Outdated
|
||
|
||
# Install camera stuff, these are not ros package dependecies and not installed with rosdep | ||
sudo apt-get install ros-kinetic-cv-camera ros-kinetic-web-video-server -y |
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.
There may be a better way, one that will allow us to remove this line specifically. Not a change request, but just a reminder that we may remove this line based off the outcome of #312
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.
Alrighty I will note this, encase there's is a pr that removes this line
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.
A few coments upon re-reviewing the code, before running a physical test.
robot/rover/RoverSetup.sh
Outdated
|
||
# Install Requirements | ||
pip install -U pip | ||
pip install pyserial==3.4 rospkg==1.1.7 |
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.
since these are included in requirements.txt
perhaps we should just use that? i.e. pip install -r requirements.txt -r requirements-dev.txt
for completeness
robot/rover/RoverSetup.sh
Outdated
|
||
source ~/.bashrc | ||
|
||
sudo apt install ros-kinetic-rosbridge-suite -y |
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 script needs to be run as sudo? If so maybe we can add a previous check to see if whoami
returns root
. If not, exit the script with a message saying it needs to be run with sudo.
robot/rover/RoverSetup.sh
Outdated
sudo cp configEthernet/runConfigEthernet.sh /usr/bin/runConfigEthernet.sh | ||
sudo cp emailer/runEmailer.sh /usr/bin/runEmailer.sh | ||
cd $REPO/robot/util/configEthernet && bash synchConfigEthernet.sh | ||
cd $REPO/robot/util/emailer/ && bash syncEmailer.sh |
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'd be surprised if the node emailer script worked out of the box. Gonna make sure to test this.
User=root | ||
Type=simple | ||
ExecStart=/home/odroid/Programming/robotics-prototype/robot/util/rosRoverStart/runRosRoverStart.sh | ||
ExecStart=/home/nvidia/Programming/robotics-prototype/robot/util/rosRoverStart/runRosRoverStart.sh |
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 can try testing this out again for some investigation 👀 🍷
robot/rover/RoverSetup.sh
Outdated
# Setup venv | ||
cd $REPO | ||
|
||
# Setup systemd services |
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 is useless
@@ -21,4 +21,4 @@ source "$REPO_HOME/venv/bin/activate" | |||
python3 "$REPO_HOME/setup.py" develop | |||
|
|||
# source primary catkin_ws setup bash script and execute one launch script to rule them all | |||
source $OPT_KINETIC_SETUP && source $ROS_PACKAGES_SETUP && roslaunch $ROSLAUNCH_FILE | |||
source OPT_MELODIC_SETUP && source $ROS_PACKAGES_SETUP && roslaunch $ROSLAUNCH_FILE |
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.
dollar sign missing
We had trouble compiling web_video_server until this was done : RobotWebTools/web_video_server#113 Then two problems with the services
|
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.
Before merging this, the exact steps of the non-automated procedure should be written on a README or a wiki page.
Something that are not quite well documented (they are documented somewhere but not in a central place)
- install npm
- services
- udev
- webvideoserver edit
Assignee Section
Description
Made some code changes and additions to ensure that ROS works properly on the TX2.
Steps for Testing
ping nividia
to ensure that you are able to communicate with the Jetson.ssh nvidia@nvidia
and the passwordnvidia
( this is also the sudo password ). Add your computer's ip and username to its /etc/hosts file.rosgui
andstartgui
.ping_acknowledgement_client
node to test your computer's connection to ros master on the Jetson.rostopic list
androsnode list
on the Jetson to view currently subscribed rostopics and running rosnodes.Note:
For some odd reason, the ros nodes use the Jetson's computer name of 'jetson-1422719019290' as its hostname instead of its username, when using the VPN server and ran from the systemd service. Roscore still uses the hostname 'nvidia', which is why you have to assign its ip to both of these hostnames. This behaviour didn't seem to occur when connecting over ethernet.
closes #275
The approval from all software team leads is necessary before merging.
Reviewer Section
Aside from local testing and the General Integration Test it is implied that static analysis should be included in the verification process.
For Pull Requests that do not include code changes, it is not required to perform the tests above.