WIP: Scheduler command rewrite #1637
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
update fork
update fork
…rruption, and ending
auscompgeek
reviewed
Mar 31, 2019
| @@ -335,7 +335,7 @@ private CameraServer() { | |||
| table.getEntry("mode").setDefaultString(videoModeToString(mode)); | |||
| table.getEntry("modes").setStringArray(getSourceModeValues(event.sourceHandle)); | |||
| } catch (VideoException ignored) { | |||
| // Do nothing. Let the other event handlers update this if there is an error. | |||
| // Do nothing. Let the other event handlers calculate this if there is an error. | |||
There was a problem hiding this comment.
I don't think you wanted to touch this.
There was a problem hiding this comment.
Oops. Intellij's refactoring tool gets overeager sometimes.
auscompgeek
reviewed
Apr 1, 2019
| @@ -1044,7 +1044,7 @@ protected void getData() { | |||
|
|
|||
| HAL.getMatchInfo(m_matchInfoCache); | |||
|
|
|||
| // Force a control word update, to make sure the data is the newest. | |||
| // Force a control word calculate, to make sure the data is the newest. | |||
There was a problem hiding this comment.
This doesn't make sense either.
| @@ -107,7 +107,7 @@ public Notifier(Runnable run) { | |||
| m_expirationTime += m_period; | |||
| updateAlarm(); | |||
| } else { | |||
| // need to update the alarm to cause it to wait again | |||
| // need to calculate the alarm to cause it to wait again | |||
| @@ -18,7 +18,7 @@ | |||
| * | |||
| * <p>The values supplied as arguments for PWM outputs range from -1.0 to 1.0. They are mapped to | |||
| * the hardware dependent values, in this case 0-2000 for the FPGA. Changes are immediately sent to | |||
| * the FPGA, and the update occurs at the next FPGA cycle (5.005ms). There is no delay. | |||
| * the FPGA, and the calculate occurs at the next FPGA cycle (5.005ms). There is no delay. | |||
| @@ -35,7 +35,7 @@ public static double getMatchTime() { | |||
| /** | |||
| * Pause the thread for a specified time. Pause the execution of the thread for a specified period | |||
| * of time given in seconds. Motors will continue to run at their last assigned values, and | |||
| * sensors will continue to update. Only the task containing the wait will pause until the wait | |||
| * sensors will continue to calculate. Only the task containing the wait will pause until the wait | |||
|
Oh dear. I'll do a thorough check today. |
…supplier, fix unit tests
|
This pull request is giant, making it difficult to review. Why are so many files being touched, and why are copies of non-related classes being made into experimental? If there are changes that reach across non-command classes, please put that into a separate PR, or restructure/squash the commits on this PR so that it can be reviewed on a per-commit basis. |
|
The unrelated class modifications and copies in experimental are from rebasing onto #1300. |
pjbuterbaugh
pushed a commit
to pjbuterbaugh/allwpilib
that referenced
this pull request
Jun 15, 2023
Co-authored-by: Vasista Vovveti <vasistavovveti@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Major rewrite of the command-based libraries.
Refactors
Commandto be an interface, migrating all necessary scheduling state intoScheduleror into temporaryCommandStateobjects used by the scheduler. This greatly improves both encapsulation and overall comprehensibility of the code's control flow.Rewrites
Schedulerto use modern Java features, increasing readability and removing a lot of unneeded cruft.Replaces
CommandGroupwithSequentialCommandGroupandParallelCommandGroup, resulting in both cleaner code and less confusion. The available functionality is exactly the same, as both objects implement the newCommandinterface and thus can be composed.Introduces new
ParallelCommandRace, which runs a set of commands in parallel and exits after the first one finishes, interrupting all the others. Uses this to implement awithTimeoutmethod in theCommandinterface, which cleanly adds a timeout to any command.Refactors
Subsystemto be an interface.Planned additions:
A
BasicSubsystemclass can also be included that calls the necessary defaulted methods (e.g.registerSubsystem) in the constructor to eliminate the minor amount of increased overhead to teams.Provide simple implementations of the new
Commandinterface to cover the features provided by the old assortment of commands (e.g. timeouts). Note that, as the new Command interface does not contain its own scheduling state, it cannot provide certain methods liketimeSinceInitialized- however, it is not difficult to include implementations that do this.