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

[commands] Add TimedCommandRobot class #4871

Closed

Conversation

srimanachanta
Copy link
Contributor

@srimanachanta srimanachanta commented Dec 28, 2022

CommandRobot class extends TimedRobot and is intended to add the CommandScheduler run method into the periodic loop without exposing it to the user.

Depending on when this merges, I can update the templates and examples to use this

@Oblarg's idea via discord

@srimanachanta
Copy link
Contributor Author

/format

@srimanachanta
Copy link
Contributor Author

/format

@sciencewhiz
Copy link
Contributor

Note that it's generally preferred to use present imperative tense for commit messages, (e.g. add instead of added). https://reflectoring.io/meaningful-commit-messages/#active-voice

@srimanachanta srimanachanta marked this pull request as ready for review December 28, 2022 05:48
@srimanachanta srimanachanta requested a review from a team as a code owner December 28, 2022 05:48
@sciencewhiz
Copy link
Contributor

Depending on when this merges, I can update the templates and examples to use this

I don't think that templates and examples should be merged for 2023, given that none of this would be beta tested.

@srimanachanta
Copy link
Contributor Author

Depending on when this merges, I can update the templates and examples to use this

I don't think that templates and examples should be merged for 2023, given that none of this would be beta tested.

I agree, further testing is needed to confirm the side effects of bringing the CommandScheduler run outside of the main loop

* Command Based Programming.
*/
public class CommandRobot extends TimedRobot {
private static final double kSchedulerOffset = 0.005;
Copy link
Member

Choose a reason for hiding this comment

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

Why is there an offset? RobotPy put ours in RobotPeriodic, which is where I think most examples put theirs... it feels like this could potentially lead to a difference in behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Starlight220 , @rzblue , and I had a conversation over discord about whether an offset should be included or not. The command scheduler call is normally placed in the RobotPeriodic method which is why testing is required to rule out any potential issues.

Copy link
Member

Choose a reason for hiding this comment

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

Having the scheduler run at the end of robotPeriodic can result in inconsistent timing if a lot of stuff runs before it. By adding an offset, the scheduler timing should be more consistent.

Copy link
Member

Choose a reason for hiding this comment

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

But in a command based robot, aren't most things running on the scheduler anyways?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but there's a lot of other stuff that runs in the TimedRobot loop, and teams can also choose to run code inside periodic methods as well.

@srimanachanta
Copy link
Contributor Author

/format

@rzblue
Copy link
Member

rzblue commented Dec 29, 2022

I don't think there will be any side effects- it is still being run in the same loop, just at a different time.

@auscompgeek auscompgeek changed the title Added CommandRobot class [commands] Add TimedCommandRobot class Dec 29, 2022
Copy link
Member

@Starlight220 Starlight220 left a comment

Choose a reason for hiding this comment

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

Add some unit tests, maybe?

@srimanachanta
Copy link
Contributor Author

Add some unit tests, maybe?

What should be added? This class is just TimedRobot with an addPeridic. If that works, this will

@rzblue
Copy link
Member

rzblue commented Jan 16, 2023

There should be a test to confirm that run is actually called like we want it to

@AngleSideAngle
Copy link
Contributor

Could this be extended to include event triggers for #5151?
My team used this implementation last year.
In addition, we may want to update documentation to encourage use of event triggers in a single Robot.java file.

@Starlight220
Copy link
Member

I think it's out of scope, but a subsequent PR would be totally fine.

@Starlight220
Copy link
Member

Abandoned by author.

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.

7 participants