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

Internals (again) #25

Merged
merged 6 commits into from
Feb 26, 2022
Merged

Internals (again) #25

merged 6 commits into from
Feb 26, 2022

Conversation

ky28059
Copy link
Member

@ky28059 ky28059 commented Feb 20, 2022

Reopening this PR to address some final concerns with internals (and stress testing to see if the code can be broken) before marking it as fully finished.

Initial commit addresses #20 (comment).

@ky28059 ky28059 changed the base branch from master to develop February 20, 2022 23:12
@@ -103,15 +99,14 @@ public InternalSubsystem(TurretSubsystem turretSubsystem) {

// if we are in auton we start with 1 ball
if (DriverStation.isAutonomous()) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is called in the subsystem constructor (and the constructor invoked during robot initialization) I don't think this will work to set ballCount to 1 when autonomous initializes. We might need to put it in Robot.autonomousInit() and pass InternalSubsystem from RobotContainer to Robot via a getter.

Copy link
Contributor

Choose a reason for hiding this comment

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

We can test out both ways. Can you test in shop to see if the ball count prints as 1 when we start autonomously?

Copy link
Member Author

@ky28059 ky28059 Feb 21, 2022

Choose a reason for hiding this comment

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

auton

Ball count is 0 when code deploy (robot initialization) occurs when driver station is on the teleop tab, and stays at 0 when switching to (and enabling) auton.

auton2

Ball count is 1 only when the robot initializes while the driver station is on the auton tab.

The idea involving Robot.autonomousInit() would set ballCount to 1 every time autonomous begins which is something to watch for in testing if we're running autonomous repeatedly.

@@ -143,14 +138,12 @@ public void periodic() {
motorBottom.set(0);

prevEntranceDetected = false;
Copy link
Member Author

Choose a reason for hiding this comment

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

Does the explicit set of prevEntranceDetected to false accomplish anything different when it's set to entranceDetected on line 110? I feel like this set would just be immediately overridden the next periodic loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it allows us to detect if a new ball enters or is entering internals, though it probably is already false from the ball moving. We can delete it, especially if it might be true (and another ball has entered).

eggaskin
eggaskin previously approved these changes Feb 22, 2022
Copy link
Contributor

@eggaskin eggaskin left a comment

Choose a reason for hiding this comment

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

autonomous init works, we can implement rejecting balls from intake when we're over 2 balls through intake after we get shooter working.

@eggaskin eggaskin dismissed their stale review February 25, 2022 00:42

more bugs

@ky28059 ky28059 mentioned this pull request Feb 25, 2022
Copy link
Contributor

@eggaskin eggaskin left a comment

Choose a reason for hiding this comment

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

tested and approved

@eggaskin eggaskin merged commit c575f43 into develop Feb 26, 2022
@eggaskin eggaskin deleted the internals-testing branch February 26, 2022 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants