Easier initialization for Actions?

[madacker]

With Init() having been removed, it looks like it's now the responsibility of the Action writer to maintain state so as to properly reset for subsequent invocations? E.g., I'm about to return 'true' so let me set an 'isUninitialized' member field that I check on every call to determine if I have to initialize again. That seems like it will be busy work common to a lot of Actions that will be used more than once - and easy to forget so multiple invocations might always be dicey.

Instead of bringing back Init(), would it be worthwhile to consider passing a parameter on run() that indicates when initialization needs to be done? Maybe it could be time-since-action-started where 0.0 indicates that it's the first invocation so initialization should be done. Time-since-action-started is often useful - and its presence would clean up FollowTrajectoryAction::run() and TurnAction::run() a bit.

[rbrott]

I deliberately made Action to be simple so that the semantics are clearer (at the cost of more heap allocations 🙂 ). One could definitely make a subclass that handles initialization. For example,

abstract class MyInitAction : Action {
    private var initialized = false

    abstract fun init()
    abstract fun loop(): Boolean

    override fun run() = if (!initialized) {
        init()
        initialized = true
        true
    } else {
        loop()
    }
}

Or maybe you prefer different semantics

abstract class MyInitAction : Action {
    private var initialized = false

    abstract fun init()
    abstract fun loop(): Boolean

    override fun run() = if (!initialized) {
        init()
        initialized = true
        loop()
    } else {
        loop()
    }
}

You could also implement an Action subclass that keeps track of the elapsed time.

[rbrott]

But to respond to your suggestion about cleaning up the built-in quickstart actions, I'm not very concerned at the moment with the duplication in logic. And I like keeping the base Action nice and compact.

[madacker]

Well, every Action that can be repeatedly invoked has to do "initialized" tracking which is a burden to the programmer, but I can understand the desire to keep the base Action nice and compact!

Speaking of keeping the base Action nice and compact, maybe it's worth considering the removal of the telemetry packet as a Run() parameter and instead making it accessible by an accessor function?

[madacker]

With the new sample code (thanks for that!), spinUp() doesn't work if it is called a second time:

            private boolean initialized = false;
            public boolean run(@NonNull TelemetryPacket packet) {
                if (!initialized) {
                    motor.setPower(0.8);
                    initialized = true;
                }

                double vel = motor.getVelocity();
                packet.put("shooterVelocity", vel);
                return vel < 10_000.0;
            }

Are you sure you don't want to provide a model that makes it easier to avoid such errors when an Action is invoked multiple times? It could look like this, for example:

            public boolean run(double elapsedTime) {
                if (elapsedTime == 0.0) {
                    motor.setPower(0.8);
                }

                double vel = motor.getVelocity();
                this.packet.put("shooterVelocity", vel);
                return vel < 10_000.0;
            }

This looks cleaner (at least to me) and will automatically work correctly when called repeatedly. The elapsed time is also super useful for many scenarios.

(It's not necessary for this particular issue, but I modeled changing Action to an abstract class where the packet member is updated appropriately before run() is invoked. Folks are used to referencing FTC's telemetry as a member of OpMode and it makes instantiation cleaner. But you probably have some good reasons for keeping it as a lean Interface rather than an abstract class.)

[rbrott]

Now that actions are mutable, I prefer them to also be consumable/disposable/single-use. As you point out, this isn't the only reasonable design in this space.