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

Subsystems #16

Merged
merged 106 commits into from
Feb 18, 2022
Merged

Subsystems #16

merged 106 commits into from
Feb 18, 2022

Conversation

ky28059
Copy link
Member

@ky28059 ky28059 commented Jan 22, 2022

No description provided.

@ky28059 ky28059 changed the title Shooter! Shooter, vision, internals Jan 25, 2022
@ky28059 ky28059 mentioned this pull request Jan 25, 2022
@ky28059 ky28059 changed the title Shooter, vision, internals Subsystems Jan 26, 2022
ky28059 and others added 3 commits February 10, 2022 22:54
The drivetrain and intake rollers work as expected, but the intake deploy was not moving the motor upwards (despite the percent output being set correctly). I'd advise whoever's testing this tomorrow to see if running the current code and pressing the A button causes the talon to begin blinking green (positive percent output); if it does, then it's a motor issue, otherwise its a code issue (which is weird, because `deploy.get()` seems to say that the deploy controller is indeed being set to the proper percent output).
This should work because staging initially stops the ball the moment it detects it (when the top of the ball is at the sensor). While staging still detects the ball (for the entire diameter of the ball as it moves up) we keep spinning the top, and as long as the bottom of the ball is still seen by staging when the top of the ball reaches the turret this code should work. We can repurpose the `shotRequested = false` code which runs when the ball leaves internals to also handle ball count decrementation.

import com.kauailabs.navx.frc.AHRS;
import static frc.robot.Constants.TankConstants.bLeftMotorPort;
Copy link
Member Author

Choose a reason for hiding this comment

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

Why this instead of the previous

import static frc.robot.Constants.TankConstants.*;

?

Copy link
Contributor

Choose a reason for hiding this comment

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

auto format

Copy link

Choose a reason for hiding this comment

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

Importing with a * is a bad idea. You don't know all that you're bringing in to the namespace. Better to be explicit.

@@ -1,31 +1,31 @@
package frc.robot.subsystems;
package frc.robot.subsystems.tank;
Copy link
Member Author

Choose a reason for hiding this comment

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

Subsystem nesting is back? 😩
Can we keep the pose estimator thread in /subsystems and not in its own folder?

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd rather not have to read 6 file names to get to a subsystem, i guess it depends on the others

shuffleboardXEntry.setDouble(pose.getX());
shuffleboardYEntry.setDouble(pose.getY());
shuffleboardField.setRobotPose(pose);
}

/**
* Gets the estimated current position of the robot.
*
Copy link
Member Author

Choose a reason for hiding this comment

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

VSCode autoformat 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

The extra newlines in javadoc and comment wrapping actually makes it harder to read than properly-formatted human written code imo

Copy link

Choose a reason for hiding this comment

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

Who is writing the formatting style VSCode is using? I think you can specify your own. Also lint.

private final RelativeEncoder leftEncoder;
private final RelativeEncoder rightEncoder;

private DifferentialDriveWheelSpeeds lastWheelSpeeds;
Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this saved as a field? Is constructing the DifferentialDriveWheelSpeeds object really that expensive?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, but calling the encoders again is -- that's what I have been saying about minimizing shuffleboard calls, .set calls, .get calls... I/O takes time

Copy link

Choose a reason for hiding this comment

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

In addition to @e3l 's comments:

Because you're essentially running an embedded system, you should think carefully about when objects are created and destroyed. Contrary to typical programming, it's common in embedded to have global variables just so the memory is all allocated at the beginning and there's little to no dynamic memory allocation.

Secondly, any operation like constructing an object has a processing penalty. In the embedded case, it is often better to do this all at startup rather than in your control loop. You want very consistent timing for your control loops, and thus you should prefer to keep the operations in those loops well known.


@Override
public void run() {
while (true) {
Copy link
Member Author

Choose a reason for hiding this comment

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

There's no Thread.sleep() in this loop.

Copy link
Contributor

Choose a reason for hiding this comment

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

... yes, because we want to be estimating our pose as often as possible

Copy link

Choose a reason for hiding this comment

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

Uhhhh. @e3l you probably should sleep once an a while, and you probably don't need to update that often. 10 Hz should be plenty. Do we know the timing here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Haven't extensively profiled the performance of this thread or how accuracy changes with different update speeds.

We do know that it's more I/O blocked than anything else, which means that having no sleep doesn't mean it'll be expensive or slow down other threads, necessarily, right?

ky28059 and others added 10 commits February 13, 2022 13:56
Also, experimental style changes! Unsure how I like the comma separated declaration, but it is certainly less wordy than repeating `private final JoystickButton` 8 times.
- Commented out PowerController (it was causing power drops and stuttering, will test causes and try fixes later)
- Bind turntable and hood to mech controller joysticks
- Bind left trigger to internals, right to intake
- Preliminary internals restructuring to replace I2C onboard color sensor
It works half of the time! See slack for details about inconsistencies
- Bind mech y to turn on flywheel, mech b to turn off
- Manual override of internals motors on mech triggers
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.

At this point we are using shooter like develop, so even though I haven't reviewed all this code I'm going to merge it. Let's actually use develop hooray

@eggaskin eggaskin marked this pull request as ready for review February 18, 2022 00:41
@eggaskin eggaskin merged commit 07aba7c into develop Feb 18, 2022
@e3l e3l deleted the shooter branch February 18, 2022 01:37
@e3l e3l restored the shooter branch February 18, 2022 01:37
e3l added a commit that referenced this pull request Mar 5, 2022
At this point we are using shooter like develop, so even though I haven't reviewed all this code I'm going to merge it. Let's actually use develop hooray.

* Recreate shooter branch from refactored Ethan code

* Skeleton `shoot()` method, some cleanup

* Add hood SparkMax

* Remember to add a constant 😳

* PID testing

* editing jetson socket so it's using constants instead of bigdata

* add networktables

* Add Jetson NT testing code

* Update NetworkTables data to match vision code

* Clean up

* editing jetson socket so it's using constants instead of bigdata

* Update NetworkTables data to match vision code

* Clean up

* Add httpcamera for testing camera stream

* Update Jetson stuff

* Jetson refactoring

* Add velocity closed loop testing code

* Jetson restructuring, `periodic()` skeleton code

* Rename shooter to turret

Shooting has been delegated to internals!

* Jetson camera wrapper

* added elevator and tank implementation

* Tank drive code, untested

* add elevator config

* reviewed added code, default commands added

* elevator commands and controller setup

* Working tank and elevator subsystems

* overhaul subsystem structure + overhaul config file

* fix end method in dtcommand

* improved handling of elevatorUpIsPositive const

* change elevator command structure

* put in config txt thing again

* Use `WPI_TalonSRX`, `DifferentialDrive`

* Use builtin `squareInput`

* Add third set of Talons

* Remove `DifferentialDrive`

* Revert "Add third set of Talons"

This reverts commit 1b5ea6d.

* add joystick code

* Add joystick capability

* Add controller switching

* code for testing gyro, ecoder, limit switch, and color+distance sensor. Supposed to print values from sensors. Need to import rev library.

* sensor testing code

* skeleton code for internal subsystem. Includes 2 color sensors, a talon (will replace w/ cim), a method that determines when to activate/deactivate motor based on color.

* Merge commit part 2

* Internals refactoring

* Initiate InternalSubsystem in RobotContainer

* shooter request updates

* added a skeleton method for launching ball into shooter

* Add `turntableAligned()` for internals

* Change initialization to constructor arguments

* Skeleton climb code, cleanup

* Intake skeleton

* Use intake enum, add intake commands

* Reformat to 4 spaces

Finally, my master plan is complete 😈

In all seriousness, all the new files being created in each branch were all using 4 space indents and the constant refactorings to preserve an unliked indent size inherited from 2 (3?) years ago was getting annoying. 4 space is the future!

* Initialize new subsystems in RobotContainer

* Fix tank spacing, better comments

* added motorport2 constant

* added a ballDetected method, updated fields to include another motor, undefined ball count method

* Restructure intake position, PID constant splitting

* Implement new climb design

* updateBallCount

* Intake / internals integration

* merge

* Ball count tracking code

* Jetson constants organization, better reject logic

* Set up shuffleboard for PID testing

* Fix internals motor flow

* Use enum for turret mode

* got rid of TODOs

* Roughly implement the climb plan

It's pretty complex.

* 775 instead of NEO for turret hood

* Update internals with IR sensors

* Use command groups for climb

* Implement module states for turret

This is so that a) drivers know what the status of the shooter is and b) internals can reject quicker (on orange) instead of waiting for a perfectly lined up shot.

* Final-ish button bindings

* Final rebase commit

* `GRTSubsystem` changes, subsystem implementation

* Implement driver intake running

* Non-conflicting motor ports

* New constants, third tank NEO

* Turntable blind spot logic, soft limits

* Degree conversion constant stubs

* Remove mjpg prefix in jetson

* Working vision :D

* Temporary intake + DT testing code

The drivetrain and intake rollers work as expected, but the intake deploy was not moving the motor upwards (despite the percent output being set correctly). I'd advise whoever's testing this tomorrow to see if running the current code and pressing the A button causes the talon to begin blinking green (positive percent output); if it does, then it's a motor issue, otherwise its a code issue (which is weird, because `deploy.get()` seems to say that the deploy controller is indeed being set to the proper percent output).

* Replace exit with staging sensor

This should work because staging initially stops the ball the moment it detects it (when the top of the ball is at the sensor). While staging still detects the ball (for the entire diameter of the ball as it moves up) we keep spinning the top, and as long as the bottom of the ball is still seen by staging when the top of the ball reaches the turret this code should work. We can repurpose the `shotRequested = false` code which runs when the ball leaves internals to also handle ball count decrementation.

* Testing of subsystems, lowered periodic loop times, moved localization to thread

* Enforce style

* Better `ModuleState` names

* Jetson reformatting

* Fix internals current drawn method

* More subsystem testing and some refactoring

* Enforce style again.

* Attempt turret `r` and `theta` state system

* Bare bones intake testing

* Internals color sensor thread

* Measure color constants, reduce IO waste

* More testing setup

* Upgrade to WPILib 2022.3.1

* Update encoder value; comment out internals

* Button binding fix, use integrated encoders for DT

Also, experimental style changes! Unsure how I like the comma separated declaration, but it is certainly less wordy than repeating `private final JoystickButton` 8 times.

* Intake, internals, turret testing

- Commented out PowerController (it was causing power drops and stuttering, will test causes and try fixes later)
- Bind turntable and hood to mech controller joysticks
- Bind left trigger to internals, right to intake
- Preliminary internals restructuring to replace I2C onboard color sensor

* Boolean state for storage rejection checking

* More internals testing

* Internals testing part 2 -- dummy shoot method

It works half of the time! See slack for details about inconsistencies

* Shooter testing

- Bind mech y to turn on flywheel, mech b to turn off
- Manual override of internals motors on mech triggers

Co-authored-by: pinkbluesky <[email protected]>
Co-authored-by: unknown <[email protected]>
Co-authored-by: eggaskin <[email protected]>
Co-authored-by: Ayeeti <[email protected]>
Co-authored-by: Ippyb <[email protected]>
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.

7 participants