-
Notifications
You must be signed in to change notification settings - Fork 4
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
New Motors #98
base: develop
Are you sure you want to change the base?
New Motors #98
Conversation
This reverts commit 2543233.
…old PWM motor code)
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.
Changes to this file (and no-sensor-excavate) aren't going to be super important as they will need to be re-written completely. Likely will need to make at least some of these changes again on the new version.
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.
Left some notes. Will probably be more eventually.
@@ -44,7 +44,7 @@ def __init__(self, excavation_publisher: rospy.Publisher = None, lin_act_publish | |||
# TODO: check and test this value | |||
self.LOWERING_TIME = 23 #In seconds, how long it takes to lower the linear actuators 90% of the way | |||
|
|||
self.EXCAVATION_SPEED = 127 | |||
self.EXCAVATION_SPEED = 1000 #RPM | |||
self.LIN_ACT_SPEED = -110 | |||
|
|||
self.is_sim = rospy.get_param("is_sim") |
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.
Github won't let me leave a comment in the right place, but two lines down (line 54) exc_message is not updated I don't think
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.
Updated in fix threshold commit
lunabot_behavior/stuck.py
Outdated
@@ -52,7 +52,7 @@ def stuck(self): | |||
error_publisher.publish(stuck_msg) | |||
|
|||
# check if trying to go somewhere | |||
if self.robot_effort.left_drive >= 15 or self.robot_effort.right_drive >= 15: | |||
if self.robot_effort.left_drive >= 15 or self.robot_effort.right_drive >= 15: #TODO RJN - these might need to be higher |
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, you're almost certainly correct. That being said, I don't think this file ever gets used. There's a strong argument to be made for deleting it entirely.
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.
Updated in fix threshold commit
self.d = rospy.get_param("~d", 0) # D gain for PID controller | ||
self.i_sat = rospy.get_param("~i_saturate", 10) # Max for integral term | ||
|
||
self._max_speed = rospy.get_param("~max_speed", 2.0) * 60 / 2 / np.pi # In rad/s converted to RPM |
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 this isn't in RPM directly? Also, to convert from linear to angular speed, don't you need wheel diameter (12 inches, if it's relevant)
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 RPM speed is 3000 at full torque and 5000 overall. I see very little downside to capping this at 3000 rpm
|
||
left_drive_msg.data = self.constrain(left) | ||
right_drive_msg.data = self.constrain(right) | ||
left_drive_msg.data = self.constrain(left_set * self._meters_per_rad) |
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 goes and uses the self._meters_per_rad. Due to github, I can't leave a comment directly on that definition, but I think that number may be wrong. Even if it is correct, something more than a magic number would be appreciated.
@@ -115,12 +75,12 @@ def _loop(self): | |||
self._right_drive_pub.publish(right_drive_msg) | |||
|
|||
def constrain(self, val): | |||
val = np.clip(-1, val, 1) # Clipping speed to not go over 100% | |||
return np.int8(val * 127 * self.max_speed_percentage) | |||
val = np.clip(-self._max_speed, val, self._max_speed) # Clipping speed to not go over 100% |
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 looking at this context self._max_speed should really just be explicity defined as RPM. If needed the paramater is used elsewhere, it would be better to back-calculate it from the rpm value.
@@ -115,14 +115,19 @@ def publish_effort(self): | |||
effort_msg.lead_screw = self.constrain( | |||
self.lead_screw_voltage, max_percent=self.max_lead_screw_percent | |||
) | |||
effort_msg.excavate = self.constrain( | |||
effort_msg.excavate = self.constrain_RPM( | |||
self.exc_voltage, max_percent=self.max_exc_percent |
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 would probably be a good idea to change exc_voltage to exc_rpm or something similar, unless I'm misunderstanding what the varaible does.
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.
Updated in fix threshold commit
|
||
def constrain_RPM(self, val, max_percent): | ||
val = np.clip(-1, val, 1) # Clipping speed to not go over 100% | ||
return np.int32(val * 1000 * max_percent) |
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 RPM constrained to never go over 1000? It should be either 3000 or 5000. The percentage that we're spinning the motor at should be the thing that changes.
127 was 100% of motor power last year.
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.
Updated in fix threshold commit
@@ -93,6 +98,7 @@ def __init__(self): | |||
|
|||
self.driving_mode = "Forwards" | |||
|
|||
self._max_speed = rospy.get_param("~max_speed", 2.0) * 60 / 2 / np.pi # In rad/s converted to RPM |
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.
Same thing that applies to the other max_speed thing in other file. Should be defined as RPM explicitly.
@@ -190,9 +196,9 @@ def joy_callback(self, joy): | |||
|
|||
# Take priority for right trigger. If it is nearly zero, use the left trigger instead | |||
if (right_trigger_axis_normalized <= 0.01): | |||
effort_msg.excavate = -1 * constrain(left_trigger_axis_normalized) | |||
effort_msg.excavate = -1 * constrain_RPM(left_trigger_axis_normalized, 1000) |
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.
Self._max_speed should either be the same value for all motors, or potentially 3k for drive and 5k for excavation. Deposition doesn't really matter, I don't expect it to be going fast
@@ -93,6 +98,7 @@ def __init__(self): | |||
|
|||
self.driving_mode = "Forwards" | |||
|
|||
self._max_speed = rospy.get_param("~max_speed", 2.0) * 60 / 2 / np.pi # In rad/s converted to RPM | |||
self.drive_speed_modifier = 1 | |||
self.slow_drive_speed = 0.5 | |||
self.fast_drive_speed = 1 |
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.
Github being bad. Deposition speed is wrong two lines down (still at 127)
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.
Updated in fix threshold commit
// USER MANUAL HERE: | ||
// https://www.omc-stepperonline.com/index.php?route=product/product/get_file&file=3354/User%20Manual%20Of%20iSV2-RS.pdf | ||
|
||
// TODO RJN - deal with alarms and error codes |
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.
Being able to reset from alarms will be very useful.
Sabertooth_MotorCtrl act_right_mtr{&MC3, STMotor::M2}; | ||
Sabertooth_MotorCtrl act_left_mtr{&MC2, STMotor::M2}; | ||
Sabertooth_MotorCtrl act_right_mtr{&MC1, STMotor::M1}; | ||
Sabertooth_MotorCtrl act_left_mtr{&MC1, STMotor::M2}; | ||
|
||
constexpr uint8_t ACT_RIGHT_CURR_ADC = 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.
These will almost certainly need to be updated. We probably won't have mutliple ADCs for only two sensors
Making a pull request for ease of commenting.
DO NOT MERGE UNTIL NEW MOTORS ARE ON THE ROBOT.
Adds new brushless motor control to the robot.