-
Notifications
You must be signed in to change notification settings - Fork 1
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
Merge Fix/low velocity error to development #99
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.
Programming wise it looks fine, so in my opinion you can merge it. But it would be a bit cleaner if you could document within the code on why some things are needed (see comments)
feed_forward[wheel] = 0; | ||
} | ||
else if (wheels_commanded_speeds[wheel] > 0) { | ||
feed_forward[wheel] = wheels_commanded_speeds[wheel] + 13; |
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 be nice to document on why 13 is being added (or subtracted on line 183). It would be better to rename it to some constant that can then have its own documentation string 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.
Yes 13 came from implementing the dry friction. The constant was found after testing our robot. But yes you're right I will rename it as a constant, it is probably more clear.
float feed_forward[4]; | ||
float threshold = 0.05; | ||
|
||
if (abs(wheels_commanded_speeds[wheel]) < threshold) { |
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 was just wondering about the reason for this threshold value, since there is also already a PWM treshold that makes sure that the robot is not slightly jittering on the field, if no explicit instructions are send out.
See
static void limitWheelPWMs(uint32_t pwms[4]){ |
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.
Oh no I don't think this is linked to the PWM signal. It is only linked to the wheel velocity and the dry friction. Here the code simply adds (if driving forward) or subtract (if driving backwards) 13 to the feedforward term. However the feedforward term is equal to 0 if wheel velocity is almost 0 (< 0.05), thus 0.05 if your threshold.
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 just copy pasted the branches from Jan Geert and Leonie, they are called wheel_velocity_estimate and wheel_feedforward_friction. The point is to make a new clean branch (with clean commits) and merge it with development.
This branch fixes the low velocity error, by mainly correcting our friction. We implemented the dry friction in the feed forward term (in the past, that was done in a weird way by adding a magic factor). Our friction is now different depending on the velocity of the robot (it is different whether it drives forward or backward).
Later on, we want to delete the branches wheel_velocity_estimate and wheel_feedforward_friction and only keep this current low_velocity_error branch to merge it to the main one.
wheel_velocity_estimate and wheel_feedforward_friction branches come from the branch feature/record_wheel_ref, which is used to record wheel data on the SD card of the robot. Thus, we want to have the low velocity error branch and the record_wheel_ref and 2 distinct branches for more clarity since they deal with two different problems.