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.
I read through some of it, it looks really good. I see a lot of great improvements and stuff I'm really happy to see.
I like that setting the constants for the motor's integrated PID is gone, as it was awkward and could cause problems.
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 looked at the odometry, I have a few opinions.
Coordinates
I think that for all the coordinate parameters, which involve an x
and a y
, it should be refactored to a Point
or 2dPoint
. This has a few benefits. First, there is less boilerplate as only one parameter is needed. Second, the API is barely changed - chassis.driveToPoint({x, y});
. Third, it allows people to store points in their code, and then use those points multiple times:
Point usefulPoint{1_ft, 11_ft};
chassis.driveToPoint(usefulPoint);
I think this pattern should be used everywhere, from OdomMath::computeAngleToPoint(Point)
to OdomChassisController::driveToPoint(Point)
.
Additional movement functions
It would be nice if the following odom function could be added: void OdomChassisController::turnToPoint(Point &ipoint);
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 this.
If I were using this API to write an autonomous program, and I wanted to visualize the field in cartesian, I think it would be awkward to have to put the StateMode in every single call:
chassis.driveToPoint({0_ft, 5_ft}, StateMode::CARTESIAN);
chassis.driveToPoint({1_ft, 3_ft}, StateMode::CARTESIAN);
chassis.driveToPoint({5_ft, 7_ft}, StateMode::CARTESIAN);
Instead, it would be nicer to have a chassis-wide flag like setTurnsMirrored
that we can set once.
It could be modified through a setter method, but it could also be through a parameter in the ctor.
People will want to choose one mindset and stick with it, so it is redundant to ask for it in every single command.
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.
Glad to see these changes getting merged in, there's a lot of really good functionality here. My primary concern with the changes is the same as @theol0403, I'd like to see a global switch for Cartesian coordinates rather than passing in a switch to every relevant function.
I noticed that there are a number of somewhat unrelated changes being grouped into this PR such as a switch to using PROS C types internally and removing the ability to modify the motors' PID values. Is there a notice of this in the other PRs that are being merged in with this, or could this be added to the description of this PR? I think those are positive changes, I just would like to be able to see where those changes were made when looking back at revision history without too much digging.
…and turn thresholds to 0 by default.
Tested with a real robot
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.
Looks like my requested changes have been applied, this all looks good to me 👍
Description of the Change
This PR merges the odometry feature set. This is being merged in an RC-ready condition. This PR also removes the integrated PID-related function from AbstractMotor and changes the ADI classes to use PROS' C API.
Note: This PR is not labeled as
needs docs
because the documentation for v4 is being moved to Doxygen, which generates documentation from header files. There is a separate branch which improves the documentation for this feature set.Verification Process
I have done manual integration testing on this branch for a few months, as have a few community members. The latest version has been tested driving around a robot without any issues.
Applicable Issues
Closes #213.