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

Shuffleboard abstraction #31

Closed
wants to merge 24 commits into from
Closed

Shuffleboard abstraction #31

wants to merge 24 commits into from

Conversation

ky28059
Copy link
Member

@ky28059 ky28059 commented Mar 29, 2022

This PR introduces convenience API wrappers around shuffleboard for ease of use. To add a simple entry,

ShuffleboardTab tab = Shuffleboard.getTab(tab);
GRTNetworkTableEntry entry = new GRTNetworkTableEntry(shuffleboardTab.add(name, value).getEntry());

becomes

GRTShuffleboardTab tab = new GRTShuffleboardTab(tab);
GRTNetworkTableEntry entry = shuffleboardTab.addEntry(name, value);

To add a listener,

ShuffleboardTab tab = Shuffleboard.getTab(tab);
shuffleboardTab.add(name, value).getEntry().addListener(this::method, EntryListenerFlags.kUpdate);

becomes

GRTShuffleboardTab tab = new GRTShuffleboardTab(tab);
shuffleboardTab.addListener(name, value, this::method);

or

shuffleboardTab.addListener(name, value, this::method, EntryListenerFlags.kNew | EntryListenerFlags.kUpdate | EntryListenerFlags.kImmediate);

if custom flags need to be set. .addListener() can also be chained to add multiple listener entries at once. There is also .addToggle(), which functions the same as .addListener() except exists specifically for boolean listeners to wrap the displaying of a fancy toggle switch:
image

shuffleboardTab.add(name, value).withWidget(BuiltInWidgets.kToggleSwitch).getEntry().addListener(this::method, EntryListenerFlags.kUpdate);
// becomes
shuffleboardTab.addToggle(name, value, this::method);

To set the position of entries from code,

ShuffleboardTab tab = Shuffleboard.getTab(tab);
GRTNetworkTableEntry entry = new GRTNetworkTableEntry(shuffleboardTab.add(name, value).withPosition(col, row).getEntry());

becomes

GRTShuffleboardTab tab = new GRTShuffleboardTab(tab);
GRTNetworkTableEntry entry = shuffleboardTab.addEntry(name, value, col, row);
shuffleboardTab.addListener(name, value, this::method, col, row);

While addEntry(), addListener, and addToggle() support the extra position arguments, it is not required and omitting position arguments will simply cause the entry to placed in the first available location when shuffleboard is populated.

@ky28059
Copy link
Member Author

ky28059 commented Mar 29, 2022

Todos:

  • Make .list() support position and size setting
  • Consider whether it's worth it for entries to support size setting (probably not, since we don't really need to resize anything past 1x1 and it makes the method signatures even more convoluted)
  • Add .grid()?
  • Make .list() take in an optional parameter to control the LABEL_POSITION layout property?

@ky28059 ky28059 marked this pull request as ready for review March 30, 2022 07:28
@ky28059
Copy link
Member Author

ky28059 commented Mar 30, 2022

Positioning GRTNetworkTableEntrys has changed a bit: it is now (similar to lists)

shuffleboardTab.addEntry(name, val).at(col, row);

for better composability and less constructor clutter (especially paired with .withSize()).

// Without these methods, GRTNetworkTableEntry would have to have a 2, 4, and 6 value constructor, as well
// as a static method most likely to handle the case where size is provided but position is not.
shuffleboardTab.addEntry(name, val).at(col, row).withSize(2, 2);

Speaking of lists, I should document those at some point. Also note that listeners, due to their structure, have not changed in how positions must be supplied in the method signature; only .addEntry().

}
forceTimer.reset();
internalSubsystem.requestShot();
shotsCompleted++;
Copy link
Contributor

Choose a reason for hiding this comment

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

how about 'shotsRequested'?

finalTimer.start();
complete = true;
// Force a shot if we haven't shot in 4 seconds
if (forceTimer.hasElapsed(4)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

8 seconds driving + 8 seconds of forcing for two balls is just too much time in a worst case scenario

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps the 8 seconds of driving time is the issue? I kinda don't want to decrease the force timer out of fear that the first shot may be forced out too early.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think shooting two balls at once fixes this

* @return The distance from the camera to the hub.
*/
public double getHubDistance() {
public double getHubDistance() {
return hubDistance;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

it'd probably be clean to return a pair of (distance, theta) and automatically include setting consumed to true as part of that method

@@ -204,9 +204,21 @@ public void periodic() {
// Spin the top motor on a timer
exitTimer.start();
motorTop.set(0.5);
stagingExitBall = 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.

So the way the two ball shot delay was fixed was to remove the final ball state entirely? Perhaps just commenting out the !stagingExitBall check on line 166 would be a better solution which still represents the ball count semi-accurately?

@@ -215,18 +227,18 @@ public void periodic() {

// Reset states
// If the only ball in the system is the one we just shot, mark the shot as completed
if (ballCount <= 1) shotRequested = false;
if (ballCount < 1) shotRequested = false;
rejectingChecked = 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.

This is a little more complicated. If we skip the stagingExitBall timer and don't wait the 0.5 seconds, rejection logic won't check the color of the second ball. If the two balls are of different colors, it will shoot both either rejecting or shooting (which is bad in both cases).

Copy link
Member Author

Choose a reason for hiding this comment

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

Perhaps a solution is to only skip the stagingExitBall check if the balls are of the same color? Might be a bit messy to implement

// If setting initial position manually, start it at a position assuming we are facing the hub
// at a distance `hubDist` inches and 0 on the y axis.
// TODO: is this worth having be a flag at all?
if (MANUAL_START_POS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i feel like we need a starting position anyways, we might as well make it something like 70, 0 instead of 0, 0 and keep this flag on. Cuz auto will overwrite it anyways

}
}

@Override
public void periodic() {
sixPosEntry.setValue(sixEncoder.getPosition());
Copy link
Contributor

Choose a reason for hiding this comment

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

i feel like setting motor powers should go in periodic. this means we have a state with the desired power, and in periodic we decide what to do with it

private static final double DELAY_LIMIT_RESET = 0.2;
private Double switchPressed = 0.0;
private double driverPower = 0;
private boolean driverOverride = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we assumed that if driverpower isn't 0 then we want to override

Copy link
Contributor

Choose a reason for hiding this comment

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

i really don't like having all these powers and overrides

Copy link
Member Author

Choose a reason for hiding this comment

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

That works, but it means that we can't override with 0 power (something that RunIntakeCommand does for 2 seconds after driver input is 0). I guess we can remove driverOverride entirely since we're not using the intake camera to detect balls at all.


// System.out.println("JETSON r: " + r + " theta: " + theta);
// Reset offsets when we refresh rtheta from vision.
resetOffsets();
Copy link
Contributor

Choose a reason for hiding this comment

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

idt we should reset offsets, we should try not to and see how driver practice goes. Because ideally offset accounts for bad interpolation tuning, which will persist even if new vision data comes in

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, I thought offsets were for rtheta drifting which would be reset when vision sends data?

shuffleboardTurntablePosEntry.setValue(Math.toDegrees(turntableEncoder.getPosition()));
shuffleboardFlywheelVeloEntry.setValue(flywheelEncoder.getVelocity());
shuffleboardHoodPosEntry.setValue(Math.toDegrees(hood.getSelectedSensorPosition() / HOOD_RADIANS_TO_TICKS));
turntablePosEntry.setValue(Math.toDegrees(turntableEncoder.getPosition()));
Copy link
Contributor

Choose a reason for hiding this comment

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

can we have all the setvalues together at the end

@ky28059
Copy link
Member Author

ky28059 commented Apr 9, 2022

Merged into #32 .

@ky28059 ky28059 closed this Apr 9, 2022
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