Skip to content
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

Clean up IntakeAssembly (#111 / #112 / #114) #117

Merged
merged 8 commits into from
Apr 3, 2018

Conversation

cjlawson02
Copy link
Contributor

@cjlawson02 cjlawson02 commented Apr 3, 2018

Fixes #111. Still needs javadocs

@cjlawson02 cjlawson02 added enhancement New feature or request subsystem Changes to a specific subsystem labels Apr 3, 2018
@cjlawson02 cjlawson02 added this to the post-svr-sweep milestone Apr 3, 2018
@cjlawson02 cjlawson02 self-assigned this Apr 3, 2018
@cjlawson02 cjlawson02 changed the title Less confusion when dealing with IntakeAssembly and Wrist Clean up IntakeAssembly (#111 & #114) Apr 3, 2018
Copy link
Contributor

@yabberyabber yabberyabber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I like the separation between Claw and Wrist

Are you able to test this Tuesday? I haven't negotiated Thursday robot availability yet

@cjlawson02
Copy link
Contributor Author

I’ll try to come in Tuesday but I might not be able to make it. If I do make it, I’ll test.

@yabberyabber
Copy link
Contributor

I can probably work with ChrisM to test this Tuesday afternoon

@yabberyabber
Copy link
Contributor

Can you also do #112 in here? Literally all you gotta do is remove all references to m_wristState

@cjlawson02
Copy link
Contributor Author

Ok

@cjlawson02 cjlawson02 changed the title Clean up IntakeAssembly (#111 & #114) Clean up IntakeAssembly (#111 / #112 / #114) Apr 3, 2018
@cjlawson02
Copy link
Contributor Author

cjlawson02 commented Apr 3, 2018

Fixes #112
Fixes #114

Copy link
Contributor

@yabberyabber yabberyabber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (m_intakeAssembly->GetClaw()->IsCubeIn()) {
m_cubeIntakeState = CubeIntakeState::Idle;
m_intakeAssembly->HoldCube();
m_intakeAssembly->Flash();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

excellent

}
else {
m_intakeAssembly->StopIntake();
s_intaking = false;
m_cubeIntakeState = CubeIntakeState::Idle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you will need to call m_intakeAssembly->HoldCube() here otherwise it'll continue intaking forever

}
else {
m_intakeAssembly->StopIntake();
m_cubeIntakeState = CubeIntakeState::Idle;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@@ -65,7 +65,8 @@ void CenterSwitchAuto::Execute(AutoRoutineBase::AutoDirection direction) {
break;
case 3:
if (m_drive->GetSplinePercentComplete() > 0.9) {
m_intakeAssembly->WideIntake();
m_intakeAssembly->RunIntake(-1.0);
m_intakeAssembly->OpenClaw();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

much better

}
if (m_intakeAssembly->GetWrist()->IsCubeIn() ||
if (m_intakeAssembly->GetClaw()->IsCubeIn() ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ohh this probably looks confusing but let me explain

GetClaw() should return something of type const Claw*. In C++ when you have a const object, you're only allowed to call const methods (methods that explicitly have the const keyword on them). C++ does this so you don't accidentally change things if you only have a const reference to it.

Anything that changes Elevator, Claw, and Wrist needs to go through IntakeAssembly because IntakeAssembly needs to make sure the states stay in sync. Anything that reads from Elevator, Claw, and Wrist is fair game because you can't screw it up through a read. That's why for reads we can say e.g. m_intakeAssembly->GetElevator()->GetHeight();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do I need to change?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nothing here

just change the definitions of GetWrist, GetClaw, and GetElevator to return const Wrist *, const Claw*, and const Elevator* respectively

@yabberyabber
Copy link
Contributor

Tested 3. April. 2018

@yabberyabber yabberyabber merged commit 7d36164 into dev Apr 3, 2018
@yabberyabber yabberyabber deleted the intakeassembly-cleanup branch April 3, 2018 22:13
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request subsystem Changes to a specific subsystem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants