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

Deprecate RobotInit #6623

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

spacey-sooty
Copy link
Contributor

Resolves #6622

@spacey-sooty spacey-sooty requested a review from a team as a code owner May 14, 2024 04:08
@spacey-sooty spacey-sooty force-pushed the deprecaterobotinit branch 3 times, most recently from ba66a8f to 09c25cb Compare May 14, 2024 04:23
calcmogul
calcmogul previously approved these changes May 14, 2024
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.

Examples need to be updated.

@spacey-sooty spacey-sooty requested a review from a team as a code owner May 20, 2024 10:25
@spacey-sooty spacey-sooty force-pushed the deprecaterobotinit branch 2 times, most recently from 99f537d to 10abaa0 Compare May 20, 2024 10:28
@spacey-sooty spacey-sooty requested a review from a team as a code owner May 20, 2024 10:28
@spacey-sooty spacey-sooty force-pushed the deprecaterobotinit branch 11 times, most recently from a4ac0af to 316aa89 Compare May 20, 2024 13:37
@sciencewhiz
Copy link
Contributor

Will exceptions thrown in the constructor be reported to the driver station? I thought that was one reason why we had RobotInit.

@sciencewhiz
Copy link
Contributor

I don't think there's a safe way for the project importer to update a project, so this feels like it should have a longer deprecation cycle and not remove until 2027 with the new controller.

There's also 29 pages on frc-docs that need updating.

@calcmogul
Copy link
Member

calcmogul commented May 20, 2024

Will exceptions thrown in the constructor be reported to the driver station? I thought that was one reason why we had RobotInit.

Those should get reported as well:

  // From Main.java:
  public static void main(String... args) {
    // RobotBase.startRobot() initializes HAL, then calls RobotBase.runRobot()
    RobotBase.startRobot(Robot::new);
  }

  // From RobotBase.java:
  private static <T extends RobotBase> void runRobot(Supplier<T> robotSupplier) {
    System.out.println("********** Robot program starting **********");

    T robot;
    try {
      // Robot class constructor called here
      robot = robotSupplier.get();
    } catch (Throwable throwable) {
      ...
    }
  
    if (!isSimulation()) {
      // Write to FRC_Lib_Version.ini
    }

    boolean errorOnExit = false;
    try {
      // RobotBase.robotInit() called in here
      robot.startCompetition();
    } catch (Throwable throwable) {
      ...
    } finally {
      ...
    }
  }

  // From TimedRobot.java:
  @Override
  public void startCompetition() {
    robotInit();
    ...
  }

We used to promote robotInit() because there was initialization order issues with the HAL. That's no longer the case. The main difference now is whether FRC_Lib_Version.ini is written before or after user code.

@Starlight220
Copy link
Member

Can you bring the equivalent C++ implementation? Due to value semantics, the split might be more important there and then parity should be kept.

@calcmogul
Copy link
Member

calcmogul commented May 20, 2024

In C++, it looks like RobotInit() is called immediately after the Robot constructor.

int main() {
  return frc::StartRobot<Robot>();
}

// From RobotBase.h:
template <class Robot>
int StartRobot() {
  int halInit = RunHALInitialization();
  if (halInit != 0) {
    return halInit;
  }

  static wpi::mutex m;
  static wpi::condition_variable cv;
  static Robot* robot = nullptr;
  static bool exited = false;

  if (HAL_HasMain()) {
    // Simulation-specific stuff here
  } else {
    impl::RunRobot<Robot>(m, &robot);
  }

  return 0;
}

// From RobotBase.h:
template <class Robot>
void RunRobot(wpi::mutex& m, Robot** robot) {
  try {
    // Allocates and constructs Robot once when this line is reached
    static Robot theRobot;
    {
      std::scoped_lock lock{m};
      *robot = &theRobot;
    }
    theRobot.StartCompetition();
  } catch (const frc::RuntimeError& e) {
    ...
  } catch (const std::exception& e) {
    ...
  }
}

// From TimedRobot.cpp:
void TimedRobot::StartCompetition() {
  RobotInit();

  ...
}

@PeterJohnson
Copy link
Member

I feel like there should be another year before we deprecate it. What we should do first is update all the templates, examples, and documentation to not use/have it, and then we can deprecate it a year later.

@spacey-sooty
Copy link
Contributor Author

So leave the template/example updates but remove the deprecation part of this pr?

@PeterJohnson
Copy link
Member

PeterJohnson commented May 24, 2024

Yes

@calcmogul
Copy link
Member

calcmogul commented May 24, 2024

myRobot just got renamed to developerRobot, so don't forget to remove robotInit() from there (removing the deprecation means that could get missed).

Copy link
Contributor

@KangarooKoala KangarooKoala left a comment

Choose a reason for hiding this comment

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

I might've missed some more spots with the doc comment on the constructor. The accurate part is mostly referring to the "function" part of the description- Is there a different wording that would make more sense for a constructor?

There were also a couple places that moved registering sendable children to the very end of the constructor, after some of the children were inverted. I think this is fine, but I'm bring that up just to make sure.

Comment on lines 45 to +46
* {@literal @}Override
* public void robotInit() {
* public Robot() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to remove the @Override above the constructor (and preferably move the entire constructor to above copyPipelineOutputs() to match typical code style).

@@ -11,8 +11,7 @@ public class MyRobot extends TimedRobot {
* This function is run when the robot is first started up and should be used for any
* initialization code.
*/
@Override
public void robotInit() {}
public MyRobot() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the doc comment still accurate and necessary?

Comment on lines 9 to +13
/**
* This function is run when the robot is first started up and should be
* used for any initialization code.
*/
void RobotInit() override {}
MyRobot() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the doc comment still accurate?

Comment on lines -25 to -26
RobotInit();

Copy link
Contributor

Choose a reason for hiding this comment

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

Does removing this still make sense given that we aren't deprecating quite yet?

@@ -23,8 +23,7 @@ public class Robot extends TimedRobot {
* This function is run when the robot is first started up and should be used for any
* initialization code.
*/
@Override
public void robotInit() {
public Robot() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the doc comment above this still accurate and necessary?

Comment on lines 22 to +26
/**
* This function is run when the robot is first started up and should be used for any
* initialization code.
*/
@Override
public void robotInit() {
public Robot() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the doc comment still accurate and necessary?

Comment on lines -37 to -38
robotInit();

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still make sense given that we aren't deprecating quite yet?

Copy link
Member

@calcmogul calcmogul May 25, 2024

Choose a reason for hiding this comment

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

#6623 (comment)

What we should do first is update all the templates, examples, and documentation to not use/have it, and then we can deprecate it a year later.

The point is to reduce the amount of new code using robotInit(), so there's less noise when we deprecate it, because teams often don't understand the difference between a warning and an error.

@@ -14,8 +14,7 @@ public class Robot extends EducationalRobot {
* This function is run when the robot is first started up and should be used for any
* initialization code.
*/
@Override
public void robotInit() {}
public Robot() {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the doc comment above still accurate and necessary?

@@ -26,8 +26,7 @@ public class Robot extends TimedRobot {
* This function is run when the robot is first started up and should be used for any
* initialization code.
*/
@Override
public void robotInit() {
public Robot() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the doc comment above still accurate and necessary?

@@ -19,7 +19,6 @@ class RainbowTest {
void rainbowPatternTest() {
HAL.initialize(500, 0);
try (var robot = new Robot()) {
robot.robotInit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this still make sense given that we aren't deprecating quite yet?

Copy link
Member

@calcmogul calcmogul May 25, 2024

Choose a reason for hiding this comment

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

#6623 (comment)

What we should do first is update all the templates, examples, and documentation to not use/have it, and then we can deprecate it a year later.

The point is to reduce the amount of new code using robotInit(), so there's less noise when we deprecate it, because teams often don't understand the difference between a warning and an error.

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.

Deprecate robotInit()
7 participants