Skip to content

Add Phases#9

Open
mxtmx wants to merge 2 commits intoSkeleton-Army:mainfrom
mxtmx:main
Open

Add Phases#9
mxtmx wants to merge 2 commits intoSkeleton-Army:mainfrom
mxtmx:main

Conversation

@mxtmx
Copy link

@mxtmx mxtmx commented Jan 24, 2026

Adds a flexible phase management system for FTC match time periods. Includes:

  • Phase class for defining named time periods with configurable durations and time units
  • PhaseManager for automatic phase transitions based on elapsed match time with listener callbacks
  • Comprehensive unit tests covering initialization, transitions, and edge cases

Allows teams to easily structure autonomous and teleop logic around match phases with automatic time-based transitions.

@opi3636
Copy link
Member

opi3636 commented Jan 24, 2026

Thank you very much for your PR.
I have some questions about the PhaseManager class.

  • Why is everything static? To me, it seems better for it to be an object rather than static methods.
  • Why is the index of the current and previous Phase stored instead of the objects themself? It introduces some unnecessary logic that can be a bit confusing.

PhaseManager is now instance-based instead of static, improving encapsulation and usability. Equality and hashCode methods were removed from Phase, so phase equality is now by reference. Corresponding tests were updated to reflect these changes.
@mxtmx
Copy link
Author

mxtmx commented Jan 30, 2026

Thanks for the feedback, I agree both of those things should change. I updated those, as well as changed phase equality comparison to be based on reference instead of name. The documentation PR has been updated as well.

@opi3636
Copy link
Member

opi3636 commented Jan 31, 2026

Thanks for the update. But I have some new notes.

  • notifyPhaseChange() should display the error to the global warning (also stdout or stderr could be nice) rather than to the telemetry, as it can easily get lost there.

  • The PhaseListener interface should be in a different file.

  • I haven't tested if this will work, but it will be better to use an OpMode listener rather than check if the timer should start every loop if possible.

@opi3636
Copy link
Member

opi3636 commented Feb 3, 2026

Thanks for the update. But I have some new notes.

* `notifyPhaseChange()` should display the error to the global warning (also stdout or stderr could be nice) rather than to the telemetry, as it can easily get lost there

* The `PhaseListener` interface should be in a different file.

* I haven't tested if this will work, but it will be better to use an [OpMode listener](https://javadoc.io/static/org.firstinspires.ftc/RobotCore/6.2.1/com/qualcomm/robotcore/eventloop/opmode/OpModeManagerNotifier.html) rather than check if the timer should start every loop if possible.

I want to a bit quick up the development time on this PR. So I will send a PR to your repo later today or tomorrow with the requested fixes and a bit more.

@mxtmx
Copy link
Author

mxtmx commented Feb 3, 2026

Sounds good, sorry about the delay, was preparing for a competition. If you would like me to do any additional changes feel free to let me know.

@opi3636
Copy link
Member

opi3636 commented Feb 3, 2026

Sounds good, sorry about the delay, was preparing for a competition. If you would like me to do any additional changes feel free to let me know.

Don't worry, all good. Best of luck at your competition!

@opi3636
Copy link
Member

opi3636 commented Feb 14, 2026

Could you please check/approve the PR on your repository?

@mxtmx
Copy link
Author

mxtmx commented Feb 15, 2026

Yep, sorry just saw the PR. I added some comments. Once we're done making changes, we'll need to update the documentation as well as I haven't done that.

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