Skip to content
This repository has been archived by the owner on Jan 13, 2025. It is now read-only.

rename OICondition inner class to Condition. update example OI to use OIFragment and Conditions. #77

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 12 additions & 10 deletions src/main/java/com/team766/framework/OIFragment.java
Original file line number Diff line number Diff line change
@@ -1,28 +1,29 @@
package com.team766.framework;

import com.team766.logging.Category;
import java.util.LinkedList;
import java.util.List;
import java.util.function.BooleanSupplier;

/**
* Fragment of an OI, with facilities to make it easy to set up {@link OICondition}s for usage in the fragment's
* Fragment of an OI, with facilities to make it easy to set up {@link Condition}s for usage in the fragment's
* {@link #handleOI} method.
*
* The overall OI for a robot will contain a set of fragments, typically one per set of controls (eg, driver, boxop, debug),
* and it will call {@link #runOI(Context)} once per its own loop. During each call to {@link #runOI}, the fragment
* will evaluate any {@link OICondition}s that were created for this fragment. This simplifies OI logic that checks if a
* will evaluate any {@link Condition}s that were created for this fragment. This simplifies OI logic that checks if a
* specific condition is currently triggering (eg pressing or holding down a joystick button) or if a condition that had been triggering
* in a previous iteration of the OI loop is no longer triggering in this iteration.
*/
public abstract class OIFragment extends LoggingBase {

protected class OICondition {
protected class Condition {
private final BooleanSupplier condition;
private boolean triggering = false;
private boolean newlyTriggering = false;
private boolean finishedTriggering = false;

public OICondition(BooleanSupplier condition) {
public Condition(BooleanSupplier condition) {
this.condition = condition;
register(this);
}
Expand Down Expand Up @@ -53,21 +54,22 @@ public boolean isFinishedTriggering() {
}

private final String name;
private final List<OICondition> conditions = new LinkedList<OICondition>();
private final List<Condition> conditions = new LinkedList<Condition>();

/**
* Creates a new OIFragment.
* @param name The name of this part of the OI (eg, "BoxOpOI"). Used for logging.
*/
public OIFragment(String name) {
loggerCategory = Category.OPERATOR_INTERFACE;
this.name = name;
}

public final String getName() {
return name;
}

private void register(OICondition condition) {
private void register(Condition condition) {
conditions.add(condition);
}

Expand All @@ -79,10 +81,10 @@ protected void handlePre() {}

/**
* OIFragments must override this method to implement their OI logic. Typically called via the overall
* OI's loop, once per iteration through the loop. Can use any {@link OICondition}s
* they have set up to simplify checking if the {@link OICondition} is {@link OICondition#isTriggering()},
* OI's loop, once per iteration through the loop. Can use any {@link Condition}s
* they have set up to simplify checking if the {@link Condition} is {@link Condition#isTriggering()},
* or, if it had been triggering in a previous iteration of the loop, if it is now
* {@link OICondition#isFinishedTriggering()}.
* {@link Condition#isFinishedTriggering()}.
*
* @param context The {@link Context} running the OI.
*/
Expand All @@ -100,7 +102,7 @@ protected void handlePost() {}
*/
public final void runOI(Context context) {
handlePre();
for (OICondition condition : conditions) {
for (Condition condition : conditions) {
condition.evaluate();
}
handleOI(context);
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/com/team766/robot/common/DriverOI.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public class DriverOI extends OIFragment {
protected double leftJoystickY = 0;
protected boolean isCross = false;

private final OICondition movingJoysticks;
private final Condition movingJoysticks;

public DriverOI(Drive drive, JoystickReader leftJoystick, JoystickReader rightJoystick) {
super("DriverOI");
Expand All @@ -28,7 +28,7 @@ public DriverOI(Drive drive, JoystickReader leftJoystick, JoystickReader rightJo
this.rightJoystick = rightJoystick;

movingJoysticks =
new OICondition(
new Condition(
() ->
!isCross
&& Math.abs(leftJoystickX)
Expand Down
45 changes: 45 additions & 0 deletions src/main/java/com/team766/robot/example/DriverOI.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package com.team766.robot.example;

import com.team766.framework.Context;
import com.team766.framework.OIFragment;
import com.team766.hal.JoystickReader;
import com.team766.robot.example.constants.InputConstants;
import com.team766.robot.example.procedures.*;

public class DriverOI extends OIFragment {
private final JoystickReader joystick;
private double joystickX;
private double joystickY;

// add any conditions (joystick inputs, etc)
private Condition button1;
private Condition moveJoystick;

// add any mechanisms to the constructor arguments as well
public DriverOI(JoystickReader joystick) {
super("DriverOI");
this.joystick = joystick;

button1 = new Condition(() -> joystick.getButton(InputConstants.BUTTON_TRIGGER));
moveJoystick = new Condition(() -> Math.abs(joystickX) > 0 || Math.abs(joystickY) > 0);
}

@Override
protected void handlePre() {
joystickX = joystick.getAxis(InputConstants.AXIS_X);
joystickY = joystick.getAxis(InputConstants.AXIS_Y);
Copy link
Member

Choose a reason for hiding this comment

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

non-blocking, because i'm not sure if i have a better way to do this, but

it seems like the code around conditions is getting pretty spread out, which hurts readability. it requires code in the constructor, in handlePre, and in handleOI

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, let's discuss.

this is less about code for conditions and more about code used in the OI that's also used in conditions.
problem to solve - allow for some code to run before conditions are evaluated (since we moved that to an explicit step run by something like the OIFragment). we could simplify by having everything in handleOI, at the cost of "missing a frame".

I understand the concern - open to a different approach that allows code to run before the evaluation step.

wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thinking about alternatives - one could be to make this part of the Conditions - tho I don't think that would be more readable.

eg, that woud make updating variables a side effect.. and given that lambdas are supposed to be short, devs might end up moving some of the logic into helper methods called via method references.

Copy link
Member

@rcahoon rcahoon Mar 11, 2024

Choose a reason for hiding this comment

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

A couple of ideas. I'm not sure any of them is the best, though I like (2) because it doesn't require class member variables.

  1. Condition has an invalidate() method that OIFragment calls before handleOI. Then conditions are lazily evaluated on the first call to e.g. isTriggering
  2. Instead of passing the lambda to the constructor, Condition has an evaluate method that the user is required to call during handleOI.
public class DriverOI extends OIFragment {
    private final JoystickReader joystick;

    private Condition moveJoystick = new Condition();

    public DriverOI(JoystickReader joystick) {
        super("DriverOI");
        this.joystick = joystick;
    }

    protected void handleOI(Context context) {
        final double joystickX = joystick.getAxis(InputConstants.AXIS_X);
        final double joystickY = joystick.getAxis(InputConstants.AXIS_Y);
        moveJoystick.evaluate(Math.abs(joystickX) > 0 || Math.abs(joystickY) > 0);

        if (moveJoystick.isTriggering()) {
            // handle joystick movement
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I'd be a bit uncomfortable with 2 since that feels a bit error-prone, eg if devs miss calling that, they'll wonder why their conditions aren't triggering as expected.

if we wanted to avoid having a preHandle, we could recommend conditions be more self-contained vs depend on these intermediate member variables that store their own state. (nothing would prevent someone from doing that eg in a helper method the developer set up and called from their conditions but those calls would be more obviously visible).

Copy link
Member

Choose a reason for hiding this comment

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

i thought about requesting that conditions be more self-contained, but then I thought about what if the condition is based off of some complex code, for example processing vision output or something. I guess in that case, probably that complex processing should be moved into a Mechanism or ProcedureWithValue or something. but it still seems like something that someone might want to do, even if it was just temporary.

what if we combined (1) and (2)? the OIFragment invalidates the Condition prior to calling handleOI, and if you try to query the Condition before evaluating it, it raises an exception. we could also raise an exception if someone tries to evaluate a condition multiple times in a single frame (i.e. evaluate is called when the Condition is not invalidated)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks! riffing off of this, I'm trying something simple that allows a fragment sublass to evaluate all of the conditions and checks that that call was made, throwing an exception if not.

wdyt?

}

@Override
protected void handleOI(Context context) {
if (button1.isNewlyTriggering()) {
// handle button press
} else if (button1.isFinishedTriggering()) {
// handle button release
}

if (moveJoystick.isTriggering()) {
// handle joystick movement
}
}
}
20 changes: 11 additions & 9 deletions src/main/java/com/team766/robot/example/OI.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,23 +5,25 @@
import com.team766.hal.JoystickReader;
import com.team766.hal.RobotProvider;
import com.team766.logging.Category;
import com.team766.robot.example.procedures.*;

/**
* This class is the glue that binds the controls on the physical operator
* interface to the code that allow control of the robot.
*/
public class OI extends Procedure {
private JoystickReader joystick0;
private JoystickReader joystick1;
private JoystickReader joystick2;
// input devices
private final JoystickReader leftJoystick;
private final JoystickReader gamepad;

// OIFragments (driver, boxop, etc)
private final DriverOI driverOI;

public OI() {
loggerCategory = Category.OPERATOR_INTERFACE;

joystick0 = RobotProvider.instance.getJoystick(0);
joystick1 = RobotProvider.instance.getJoystick(1);
joystick2 = RobotProvider.instance.getJoystick(2);
leftJoystick = RobotProvider.instance.getJoystick(0);
Copy link
Member

Choose a reason for hiding this comment

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

should there not be a rightJoystick?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for these examples (most frequently used with BurroBots - a competition bot would typically use Swerve drive with the com.team766.robot.common.DriverOI), I was thinking arcade drive. but the example sets the wrong expectation with leftJoystick - renamed to joystick.

I can also include leftJoystick and rightJoystick or joystick and gamepad if you think that's better.

Copy link
Member

Choose a reason for hiding this comment

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

joystick SGTM

gamepad = RobotProvider.instance.getJoystick(1);
driverOI = new DriverOI(leftJoystick);
}

public void run(final Context context) {
Expand All @@ -30,8 +32,8 @@ public void run(final Context context) {
context.waitFor(() -> RobotProvider.instance.hasNewDriverStationData());
RobotProvider.instance.refreshDriverStationData();

// Add driver controls here - make sure to take/release ownership
// of mechanisms when appropriate.
// call each of the OI fragment's runOI methods.
driverOI.runOI(context);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
package com.team766.robot.example.constants;

public class InputConstants {
// this class only contains constants - should never be instantiated
private InputConstants() {}

public static final int AXIS_X = 0;
public static final int AXIS_Y = 1;

public static final int BUTTON_TRIGGER = 1;
}
12 changes: 6 additions & 6 deletions src/main/java/com/team766/robot/reva/BoxOpOI.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ public class BoxOpOI extends OIFragment {
private final Shooter shooter;
private final Climber climber;

private final OICondition shooterShoot;
private final OICondition intakeOut;
private final OICondition intakeIn;
private final Condition shooterShoot;
private final Condition intakeOut;
private final Condition intakeIn;

public BoxOpOI(
JoystickReader gamepad,
Expand All @@ -36,9 +36,9 @@ public BoxOpOI(
this.shooter = shooter;
this.climber = climber;

shooterShoot = new OICondition(() -> gamepad.getAxis(InputConstants.XBOX_RT) > 0);
intakeOut = new OICondition(() -> gamepad.getButton(InputConstants.XBOX_RB));
intakeIn = new OICondition(() -> gamepad.getButton(InputConstants.XBOX_LB));
shooterShoot = new Condition(() -> gamepad.getAxis(InputConstants.XBOX_RT) > 0);
intakeOut = new Condition(() -> gamepad.getButton(InputConstants.XBOX_RB));
intakeIn = new Condition(() -> gamepad.getButton(InputConstants.XBOX_LB));
}

@Override
Expand Down
21 changes: 10 additions & 11 deletions src/main/java/com/team766/robot/reva/DebugOI.java
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,11 @@ public class DebugOI extends OIFragment {
private final Climber climber;
private final Intake intake;
private final Shooter shooter;
private final OICondition controlShoulder;
private final OICondition controlClimber;
private final OICondition controlShooter;
private final OICondition intakeIn;
private final OICondition intakeOut;
private final Condition controlShoulder;
private final Condition controlClimber;
private final Condition controlShooter;
private final Condition intakeIn;
private final Condition intakeOut;

public DebugOI(
JoystickReader macropad,
Expand All @@ -58,12 +58,11 @@ public DebugOI(
this.intake = intake;
this.shooter = shooter;

controlShoulder =
new OICondition(() -> macropad.getButton(InputConstants.CONTROL_SHOULDER));
controlClimber = new OICondition(() -> macropad.getButton(InputConstants.CONTROL_CLIMBER));
controlShooter = new OICondition(() -> macropad.getButton(InputConstants.CONTROL_SHOOTER));
intakeIn = new OICondition(() -> macropad.getButton(InputConstants.INTAKE_IN));
intakeOut = new OICondition(() -> macropad.getButton(InputConstants.INTAKE_OUT));
controlShoulder = new Condition(() -> macropad.getButton(InputConstants.CONTROL_SHOULDER));
controlClimber = new Condition(() -> macropad.getButton(InputConstants.CONTROL_CLIMBER));
controlShooter = new Condition(() -> macropad.getButton(InputConstants.CONTROL_SHOOTER));
intakeIn = new Condition(() -> macropad.getButton(InputConstants.INTAKE_IN));
intakeOut = new Condition(() -> macropad.getButton(InputConstants.INTAKE_OUT));
}

@Override
Expand Down
Loading