-
Notifications
You must be signed in to change notification settings - Fork 16.8k
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
Rover: add wheel encoder support #6588
Conversation
AP_Param::setup_object_defaults(this, var_info); | ||
|
||
// init state and drivers | ||
memset(state,0,sizeof(state)); |
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.
nicpic : miss space after comma
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 in follow up commit.
if (state[instance].total_count == 0) { | ||
return 0.0f; | ||
} | ||
return constrain_float((1.0f - ((float)state[instance].error_count / (float)state[instance].total_count)) * 100.0f, 0.0f, 100.0f); |
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.
we should reset total_count and error_count with the first one that overflow !
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.
thanks for the review. I'm going to leave this unresolved because I think even with my little rover (which thus has a very high number of events coming back from the wheel encoder), it would need to travel about 200km before an overflow occurred. Also more importantly, I'm pretty sure we will need to change this signal quality to shorten the period of time it covers. I suspect that work will resolve the potential overflow. Thanks again for the review.
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.
Yep I didn't make the calculation ... 200km is fine, your motor died before !
About the period, yes we can probably improve the time windows with some better statistic, but it is a futur enhancement
Looking at the ROS model, they have implemented a low level PID for the ifferential_drive, it could be quite interesting to expose a similar structure so ArduPilot could offer a direct compatibility to ROS: Please note that ROS just added a new diff_drive package http://wiki.ros.org/diff_drive_controller that is addind the skid steering. |
@patrickpoirier51, thanks for that. I definitely agree we should add a low level velocity controller using the wheel encoder(s) probably both for the main throttle and skid-steering rovers. That plus getting the wheel encoder stuff into the EKF will be a big jump forward for Rover. |
We can either:
or
|
I should also add that there is theoretically no difference in performance with these two approaches, just where the conversion from cumulative to delta position occurs. |
// return true if the instance is enabled | ||
bool enabled(uint8_t instance) const; | ||
|
||
// get the position of the wheel associated with the wheel encoder |
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 we add some information on the axis system this is defined in and the units.
This PR adds initial support for Rovers to read from up to two "quadrature" wheel encoders (the most common type of wheel encoder apparently).
Details on the signals produced by a quadrature encoder can be found on the Wikipedia Rotary encoder page (look for "Incremental rotary encoder").
This PR adds:
This PR was tested by driving down my block and checking the dataflash logs:
![drivetest](https://user-images.githubusercontent.com/1498098/28152891-b29c9cea-67dd-11e7-8683-4c4892a765bd.png)
Some items to address in future PRs: