-
Notifications
You must be signed in to change notification settings - Fork 43
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
Program for calculating the linear drag coefficient in all 6 directions #394
base: master
Are you sure you want to change the base?
Conversation
Program to calculate the linear drag in all 6 directions.
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.
Great work on this program! After some small changes I hope to see it running on the Sub to model this and help to improve our controls!
In addition to my comments, the program should go in gnc/sub8_system_id/nodes/estimate_drag_coefficients
and info should be added to the README of that package explaining what the program does and how to use it.
Drag.py
Outdated
rospy.set_param('LINEAR_FORCE', 10) | ||
rospy.set_param("TORQUE", 5) | ||
rospy.set_param('ForceDown', -20) | ||
rospy.set_param('TimeOfForceDown', 10) |
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.
Instead of setting them here, just get them as you do below, with defaults. This code would prevent them from ever being changed, as they're just getting reset when the program runs
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.
It may also be wise to consider moving this to a config down the line, and ensuring sensible values are set before starting anything.
Drag.py
Outdated
|
||
|
||
def Find_Bouyancy(choice): # Initial function used to upward force of buoyancy (used in determining drag in Z axis) | ||
global Bouyancy |
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 great practice to make all your variables global, this would definitely be much more readable using a class
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.
Ditto
Drag.py
Outdated
Apply_Force('yw') | ||
Apply_Force('rl') | ||
Apply_Force('p') | ||
# Drag coefficients are written to file, do with them what you wish. |
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 noticed there is no user feedback for this program. It would be kinda scary/confusing to watch the robot do this with no idea why it's moving or what it's thinking. Encourage you to use rosypy.loginfo
upon making these transitions of state. Also recommend printing out to the user the solution in addition to writing it to a file
Drag.py
Outdated
file_object.write("\nRoll: " + str(Roll_Drag)) | ||
file_object.write("\nPitch: " + str(Pitch_Drag)) | ||
file_object.write("\nYaw: " + str(Yaw_Drag)) | ||
file_object.close() |
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.
Would be more helpful if this was written to a file in a way that a computer could read, such as yaml. Luckily, this is super easy in python!
import yaml
data = {'x': 500, 'y': 500, ...}
f = open('drag_coefficients.yaml', 'w')
yaml.dump(data, f)
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.
Ditto'ing Kevin's comment, definitely consider dumping the data into a format that's computer readable, a yaml like what Kevin mentions would be great. It would make it much easier to dump things into Python to extract data, graph, etc.
Drag.py
Outdated
file_object.write("\nYaw: " + str(Yaw_Drag)) | ||
file_object.close() | ||
except rospy.ROSInterruptException: | ||
pass |
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.
Passing on exceptions is a major red flag. The user would not be notified of why the program closes or potentially unpredictable behavior of the robot that an exception could cause. You should always atleast
except rospy.ROSInterruptException as e:
rospy.logerr(e)
Drag.py
Outdated
Calculates drag based on the initial applied force and the approximate max velocity | ||
the sub achieves in that axis. See for formula: | ||
Axis determined by char argument. | ||
''' |
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.
Appreciate your use of docstrings. It also really nice to talk about what the functions parameters are and what, if anything the function returns.
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 seems like the formula is missing
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 exciting! Jason and I talked about doing something like this for a long time.
Some things to consider:
- Observe many, repeated motions of the sub and try to fit a model. Read up on optimization, your keywords are "maximum likelihood estimation" and "system identification"
- The measurements you get from the real sub will be noisy, exact
< 0.0
checks that work in no-sensor sim won't work in the pool
Drag.py
Outdated
rospy.set_param("TORQUE", 5) | ||
rospy.set_param('ForceDown', -20) | ||
rospy.set_param('TimeOfForceDown', 10) | ||
Linear_Drag_X = 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.
Consider naming things in a consistent way
Variables: snake_case
Functions: snake_case
Classes: CamelCase
Drag.py
Outdated
pub.publish(force_msg) | ||
Down_Force = Down_Force - .001 | ||
rospy.sleep(.01) | ||
if Velocity < 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.
Remember that the sub's velocity is estimated from sensors, so it will be noisy. Even if the sub is perfectly still, you'll observe a velocity bouncing around zero.
One might expect this to be compounded by the fact bobbing of the sub in the water
As @jpanikulam mentioned, the right way to do this is to formulate it as a statistical regression problem. Not only would you get better results with less painstaking experiments, you'd also become an omnipotent god of estimation theory in the process. However, MIL is 50% about doing things right, and 50% about doing things right enough. So in the style of your approach here, might I suggest the following: It sounds like the real goal is to get a simplified drag model that can help compute a feedforward term in your controller. I.e. your PD controller will output some wrench based on feedback, plus the drag coefficients you found here times the current twist, in hopes of cancelling the effect of drag on the sub. So, just skip ahead and add that term to your controller right now with a wild guess at the drag coefficients (all zero is a fine guess). Then, instead of commanding wrenches for experimenting (which wont stop the sub from moving along axes it shouldn't for the current test), you should command twists through your controller. So if you're testing the sub's forward motion, command a twist that is only nonzero for forward linear velocity. As usual, you'll see a nice controlled straight line motion that rejects most bobbing and other disturbances... but also as usual, the velocity that the sub achieves won't be the exact velocity you commanded! It will be off by some amount, because your feedforward term has the wrong drag coefficient. Tweak the drag coefficient until the sub's velocity matches what you commanded. Repeat for each axis. If tweaking by hand seems tedious, code up your tweaking process so it happens automatically. Congratulations, you made an adaptive controller. Anyway, nice work so far! |
Drag.py
Outdated
Max_Velocity = .1 | ||
Bouyancy = 0 | ||
# Magnitude of applied force for determining drag coeffcients in linear axes | ||
appliedLinearForce = rospy.get_param("LINEAR_FORCE", default=10) |
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 caution against setting defaults that would cause the sub to do something when no config/values are set (in this case I'm assuming there would be some force/torque/etc. applied for some amount of time?)
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.
Great work! Happy you want to join MIL! Send your resume our way if you are looking for an internship!
Drag.py
Outdated
Calculates drag based on the initial applied force and the approximate max velocity | ||
the sub achieves in that axis. See for formula: | ||
Axis determined by char argument. | ||
''' |
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 seems like the formula is missing
Drag.py
Outdated
if (choice == 'x'): | ||
Linear_Drag_X = (appliedLinearForce / abs(Max_Velocity)) | ||
elif (choice == 'y'): | ||
Linear_Drag_Y = (appliedLinearForce / abs(Max_Velocity)) |
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 see a from __future__ import division
at the top of the file. This can lead to some annoying bugs in the future if an int is passed into this function. If you still want to keep the int dividing behavior in other parts of the file, cast one of the values to a float
before dividing.
Drag.py
Outdated
|
||
|
||
def Find_Bouyancy(choice): # Initial function used to upward force of buoyancy (used in determining drag in Z axis) | ||
global Bouyancy |
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.
Ditto
…ms/id/estimate_drag_coefficients.py
…py to gnc/sub8_systems_id/sub8_systems_id/estimate_drag_coefficients.py
….py to gnc/sub8_system_id/sub8_system_id/estimate_drag_coefficients.py
I added the new program along with a readme file to my forked repo. Let me know if I need to change anything before it gets 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.
Nice improvements to readability! It does not seem to run for me, gets past the buoyancy stage then gets stuck on applying a Z force (nothing is published to /wrench).
I'd recommend running this on your machine and see if you can reproduce this problem. It may be related to you recreating Pub/Subs within a loop or init_node'ing twice
terminal (linear/rotationl) velocity in that direction. Once that | ||
max velocity is found, it is used in the calculate_drag() function. | ||
''' | ||
global max_velocity, bouyancy, linear_drag_z |
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.
Still using the globals here but they should be a part of self
def find_bouyancy(self, choice): # Initial function used upward force of buoyancy(used determining drag in Z axis) | ||
down_force = -.5 # Applies initial downward force in z axis | ||
pub = rospy.Publisher('/wrench', WrenchStamped, queue_size=30) | ||
rospy.init_node('move_sub', anonymous=False) |
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.
init_node
must only be called once at the start of the program before and sub/pubs are created
force_msg.wrench.force.z = 0 # Applies 0 force which stops downward force | ||
pub.publish(force_msg) | ||
while not (rospy.is_shutdown()): | ||
rospy.Subscriber("/odom", Odometry, self.get_velocity, choice) |
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 you have these in multiple parts of your program, perhaps they should be a class variable
I made the ReadMe file correction. I also addressed the problem about nothing being published to wrench. Apparently a newly declared publisher has to wait a small amount of time so the topic can connect before it sends a message |
Most of the problems in my latest review still seem present.
|
No description provided.