-
Notifications
You must be signed in to change notification settings - Fork 3
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
Update Gazebo with new masses and sensor suite #193
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.
im approving but there's a to-fix please fix <3
@@ -198,7 +213,7 @@ | |||
<headingDriftFrequency>0.0 0.0 0.0</headingDriftFrequency> | |||
<headingGaussianNoise>0.0 0.0 0.0</headingGaussianNoise> | |||
</plugin> | |||
</gazebo> | |||
</gazebo>--> |
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.
can you remove the commented out things?
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.
oops just saw this will do
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.
cool. merge when you do, unless you add things then i can rereview
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.
unapproving since you asked me to. LMK when i can review
…published by gazebo" This reverts commit 6094942. Conflicts: buzzmobile/simulation/sim_car_interface/src/sim_car_interface.cpp
…zmobile into navsatfix Conflicts: buzzmobile/launch/includes/sim_spawn_car.launch buzzmobile/launch/simulation.launch buzzmobile/package.xml
…navsatfix Conflicts: buzzmobile/package.xml
@irapha ok updated this PR because I was doing everything on this branch |
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 ur build is broken
buzzmobile/constants.yaml
Outdated
@@ -44,7 +44,7 @@ wheel_separation: 1.3716 # distance between left and right wheels in meters | |||
max_steering_angle: 0.262 # radians (=15 degrees) | |||
|
|||
# Max speed of buzzmobile in m/s | |||
max_speed: 0.2 | |||
max_speed: 2 #TODO this is a murder machine |
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 this change then? just for checking that simulation works?
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.
our max speed is likely less than 2 m/s but also more than 0.2. I can update the TODO to be more professional (I'll do that for all of them in this PR), but max speed is something we just need to measure. If we left the max speed at 0.2 the car won't move.
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.
IIRC we put 0.2 down because the arduino software we got from RJ had a few bugs where in it would flush the speed update messages all at once, causing toyota mode to happen. We fixed it for when it was going forward, but didn't have time to do anything when it was turning.
You can experiment with this speed when you have access to the car, but I'd start with slow speed at first so you don't die while trying to figure out what the ideal speed is.
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'm bumping it down to 1 m/s now as a compromise. Still a bit faster than the car but if we set it to 0.2 then the simulation (running at 0.03 real time factor) doesn't let you see the car move.
@@ -1,5 +1,5 @@ | |||
#! /bin/bash | |||
|
|||
sleep 3 | |||
sleep 15 |
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 neeeddeeeeddddddddd??? such overhead, and we're not even using the launch script! why
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.
Its a good kludge, there should at least be a todo to investigate what that actually relies on, and to instrument something that manages process deps better.
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's an issue #158
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'll change it back to 3 since we're not using the launch script.
buzzmobile/launch/simulation.launch
Outdated
@@ -9,7 +9,7 @@ | |||
<include file="$(find buzzmobile)/launch/includes/mapping.launch"/> | |||
<include file="$(find buzzmobile)/launch/includes/steering.launch"/> | |||
<include file="$(find buzzmobile)/launch/includes/lidar.launch"/> | |||
<include file="$(find buzzmobile)/launch/includes/controller.launch"/> | |||
<!--<include file="$(find buzzmobile)/launch/includes/controller.launch"/>--> |
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.
huh i guess this change makes sense, right? we don't want controller in simulation
wait controller.launch also has car_pose_mux, which we need to make this work. how did you test this at all without car_pose_mux?
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.
you just have rqt publish the state of the car to be 0 (AUTO).
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.
but you still need car_pose_mux. That's the thing that takes in auto_car_pose and outputs car_pose.
I agree you don't need the controller node, but you need the car_pose_mux, which is in controller*.launch*
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'm not sure why this worked either because one would think that the PID controller would need to poll the pose of the car to calculate output, but it worked. Nonetheless, I added the car_pose_mux import back.
buzzmobile/plan/steering/steering.py
Outdated
@@ -41,7 +41,7 @@ | |||
BRAKING_DISTANCE = rospy.get_param('braking_distance') | |||
THRESHHOLD = rospy.get_param('braking_score_threshhold') | |||
BUZZMOBILE_WIDTH = rospy.get_param('buzzmobile_width') | |||
MAX_SPEED = rospy.get_param('max_speed', 1.0) | |||
MAX_SPEED = rospy.get_param('max_speed', 2.0) #TODO don't be a murder machine |
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 fix todo maybe?
@@ -134,6 +134,8 @@ def median_filter(fix): | |||
elif len(g['fixes']) % 2 and len(g['fixes']) > 1: | |||
return sorted_points[index] | |||
elif len(g['fixes']) > 1: | |||
rospy.loginfo("sorted point: ") | |||
rospy.loginfo(sorted_points) |
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 shouldnt be in PR when its 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.
That said, logging might be good.
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.
go away qa
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.
:3
<mass value="5.0"/> | ||
<inertia ixx="0.10595165" ixy="-0.002487144" ixz="0.011981253" iyy="0.126271962" iyz="0.011832006" izz="0.033185337"/> | ||
<mass value="136.0"/> | ||
<inertia ixx="39179.495" ixy="-257.203201" ixz="3771.586" iyy="95397.6332" iyz="-20.60398" izz="94045.9387"/> |
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.
YAY CORRECT INERTIALS?
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.
yup! thanks to Matt and Wolfram mathworld!
@@ -342,15 +356,21 @@ | |||
<joint name="lidar_joint" type="fixed"> | |||
<parent link="chassis"/> | |||
<child link="laser"/> | |||
<origin xyz="0.255 0 0.110" rpy="0 0 0"/> | |||
<origin xyz="0.255 0 5" rpy="0 0 0"/> |
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 all you did here was put the lidar way above the car. can you actually place it where our lidar is IRL?
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, its actually the same height as the base axle and about 12 inches from the front of the chassis.
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.
Wait so this is.. correct? Or are you gonna update it?
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.
no its incorrect, that comment is more a note to myself.
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.
Okay. Let me know when you have something I can review again.
constexpr double chassis_length = 0.33246; | ||
constexpr double chassis_width = 0.28732; | ||
constexpr double chassis_length = 1.8; | ||
constexpr double chassis_width = 1.57; |
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.
are these real-world?
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.
yes but i think they have to be converted to imperial, good catch.
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.
uh what why omg are you sure?
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.
Those look reasonable. 1.8 meters long and 1.57 meters wide.
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.
Wait hold on this doesn't at all match https://github.com/gtagency/buzzmobile/blob/master/buzzmobile/constants.yaml#L39 (which also says our wheels are 2.2 meters in circumference, which is wrong)
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.
nope its all metric, had it flipped in my head. it was autoCAD thats all imperial
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 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.
pls no hart atak anymor
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.
@joshuamorton yeah the dims don't match at all and these were the numbers I got from RJ...
In the CAD those are the dimensions of the car, when (if) I get access to the car, I'll check myself which measurements are correct. For now we should merge it in but with a TODO.
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 made an issue for this #208
constexpr double inv_chassis_length = 1.0 / chassis_length; | ||
constexpr double chassis_width_2 = chassis_width / 2.0; | ||
constexpr double max_torque = 0.1; | ||
constexpr double max_torque = 2.5; //lololol |
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? no. rip
aka pls fix?
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.
max torque is also just an estimate, but yeah I'll cleanup the comment.
Not sure why builds are failing. According to the circle ci tests |
The SHA files thing is just a missing config file, it dosen't cause a failure, it just means your builds are slower than they could be (in theory) #156 I think this is the error you want to look at:
From the simulation test. Seems like a missing dependency, so not too big of a deal |
As a note, @chsahit when you're looking for what caused a circleci build failure, you can either find the failure through the gh badges (see image) or through the |
(and a gh issue)
…On Wed, May 10, 2017 at 6:53 PM Sahit Chintalapudi ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In buzzmobile/simulation/sim_car_interface/src/sim_car_interface.cpp
<#193 (comment)>:
> @@ -35,13 +35,13 @@ double speed_measured_right = 0.0;
double steer_set_point = 0.0;
// TODO(sahit): change to rosparams
-constexpr double chassis_length = 0.33246;
-constexpr double chassis_width = 0.28732;
+constexpr double chassis_length = 1.8;
+constexpr double chassis_width = 1.57;
@joshuamorton <https://github.com/joshuamorton> yeah the dims don't match
at all and these were the numbers I got from RJ...
In the CAD those are the dimensions of the car, when (if) I get access to
the car, I'll check myself which measurements are correct. For now we
should merge it in but with a TODO.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#193 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHC0h3BvIwmW7_2yDfMYRYJrJXG6vHXrks5r4modgaJpZM4MPGM6>
.
|
buzzmobile/launch/simulation.launch
Outdated
@@ -9,6 +9,7 @@ | |||
<include file="$(find buzzmobile)/launch/includes/mapping.launch"/> | |||
<include file="$(find buzzmobile)/launch/includes/steering.launch"/> | |||
<include file="$(find buzzmobile)/launch/includes/lidar.launch"/> | |||
<node pkg="buzzmobile" name="car_pose_mux" type="car_pose_mux.py"/> |
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 don't know how much I like this solution.. I'd rather have a redundant controller node started (and not doing anything) than launch nodes directly. Others can pitch in but in either case this change doesn't belong in this PR
buzzmobile/constants.yaml
Outdated
@@ -44,7 +44,7 @@ wheel_separation: 1.3716 # distance between left and right wheels in meters | |||
max_steering_angle: 0.262 # radians (=15 degrees) | |||
|
|||
# Max speed of buzzmobile in m/s | |||
max_speed: 2 #TODO this is a murder machine | |||
max_speed: 1 #TODO: measure the max speed. |
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.
if TODO -> make gh issue
pls
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 sort the issues, i just want us to track TODOs as issues. It's fine for the "TODO" to exist in code too.
buzzmobile/plan/steering/steering.py
Outdated
@@ -41,7 +41,7 @@ | |||
BRAKING_DISTANCE = rospy.get_param('braking_distance') | |||
THRESHHOLD = rospy.get_param('braking_score_threshhold') | |||
BUZZMOBILE_WIDTH = rospy.get_param('buzzmobile_width') | |||
MAX_SPEED = rospy.get_param('max_speed', 2.0) #TODO don't be a murder machine | |||
MAX_SPEED = rospy.get_param('max_speed', 1.0) #TODO measure car max speed |
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 does this get_param
call have a second argument? Can you just remove it and have it rely on the real param? This way we don't have to modify max_speed
in multiple places when we want to modify it
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 think the second parameter was there to prevent an exception being thrown if the rosparam server wasn't up or that rosparam wasn't there. But there exists a get_param
call that only takes in the first parameter.
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.
Yes. And I think that if rosparam server isn't up, this should throw an exception and error out.
@@ -356,7 +356,7 @@ | |||
<joint name="lidar_joint" type="fixed"> | |||
<parent link="chassis"/> | |||
<child link="laser"/> | |||
<origin xyz="0.255 0 5" rpy="0 0 0"/> | |||
<origin xyz="2 0 0" rpy="0 0 0"/> |
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 the correct location of the lidar?
if not can you make a TODO and an issue to mesure it in the car exactly?
@@ -39,7 +39,8 @@ constexpr double chassis_length = 1.8; | |||
constexpr double chassis_width = 1.57; | |||
constexpr double inv_chassis_length = 1.0 / chassis_length; | |||
constexpr double chassis_width_2 = chassis_width / 2.0; | |||
constexpr double max_torque = 2.5; //lololol | |||
//TODO(sahit): this can also be calculated/measured when we have the car | |||
constexpr double max_torque = 2.5; |
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.
make issue too
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.
All the measurement ones can be the same issue. Just be sure to link to each line where these TODOs exist
buzzmobile/launch/simulation.launch
Outdated
@@ -9,7 +9,8 @@ | |||
<include file="$(find buzzmobile)/launch/includes/mapping.launch"/> | |||
<include file="$(find buzzmobile)/launch/includes/steering.launch"/> | |||
<include file="$(find buzzmobile)/launch/includes/lidar.launch"/> | |||
<include file="$(find buzzmobile)/launch/includes/controller.launch"/> | |||
<node pkg="buzzmobile" name="car_pose_mux" type="car_pose_mux.py"/> | |||
<!--<include file="$(find buzzmobile)/launch/includes/controller.launch"/>--> |
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 made a comment but idk why its not showing up. Id rather have a redundant controller node started than be launching nodes in two different ways in this file.
Others can argue otherwise, but in either case, this change doesnt belong in this PR (unless this is required to make sim work). If you feel strongly abt it then make a new PR pls
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.
jk it was showing up
|
||
constexpr double wheel_circumference = 2.0 * M_PI * 0.036; | ||
constexpr double wheel_circumference = 2.0 * M_PI * 0.3302; |
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.
dont forget that all of these should eventually be rosparams (and not constants in this 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.
There's an issue out for this #190 so this PR can be merged
I was going to add that that seems reasonable, yeah.
…On Thu, May 11, 2017 at 1:22 PM Raphael Gontijo Lopes < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In buzzmobile/plan/steering/steering.py
<#193 (comment)>:
> @@ -41,7 +41,7 @@
BRAKING_DISTANCE = rospy.get_param('braking_distance')
THRESHHOLD = rospy.get_param('braking_score_threshhold')
BUZZMOBILE_WIDTH = rospy.get_param('buzzmobile_width')
-MAX_SPEED = rospy.get_param('max_speed', 2.0) #TODO don't be a murder machine
+MAX_SPEED = rospy.get_param('max_speed', 1.0) #TODO measure car max speed
Yes. And I think that if rosparam server isn't up, this *should* throw an
exception and error out.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#193 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEz4APrSwt4CQ3R4gaGfUPRlG7zA1iW2ks5r424RgaJpZM4MPGM6>
.
|
Hopefully fix build.
Pls fix.
|
…published by gazebo" This reverts commit 6094942. Conflicts: buzzmobile/simulation/sim_car_interface/src/sim_car_interface.cpp
…to navsatfix Conflicts: buzzmobile/launch/simulation.launch
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.
Go ahead and pull from master to clean up this PR some.
@@ -106,8 +106,13 @@ int main(int argc, char **argv) { | |||
ros::Publisher leftSteeringPublisher = handle.advertise<std_msgs::Float64>("/left_steer_position_controller/command", 1); | |||
ros::Publisher rightSteeringPublisher = handle.advertise<std_msgs::Float64>("/right_steer_position_controller/command", 1); | |||
|
|||
<<<<<<< HEAD |
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.
Please fix.
assert(ct.message) | ||
|
||
print(ct.message) | ||
time.sleep(30) |
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.
Try moving this outside of the with block and see what happens/if you can shorten it.
@joshuamorton I pulled from master and git reports that my branch is already up to date |
Weird, its giving some really unhelpful diffs that include already merged changes. |
So I didn't change anything but I went to the circle CI console and hit build and now we're passing... @joshuamorton @irapha |
It may be because I pushed to pyros.
…On Thu, Jun 15, 2017, 3:13 PM Sahit Chintalapudi ***@***.***> wrote:
So I didn't change anything but I went to the circle CI console and hit
build and now we're passing... @joshuamorton
<https://github.com/joshuamorton> @irapha <https://github.com/irapha>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#193 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AEz4ADritUGO_cHL5wZU4rkViJCOOKOOks5sEaydgaJpZM4MPGM6>
.
|
holy guacamoli |
Feel free to merge. But before you do, please double check this PR's description to make sure it actually closes the issues listed |
Closes #161
Closes #196