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

Prepare for RobotInit deprecation by updating examples #6623

Merged
merged 9 commits into from
Jul 17, 2024

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 12 times, most recently from 316aa89 to 367756d Compare May 20, 2024 13:42
@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.

@spacey-sooty spacey-sooty requested a review from calcmogul June 5, 2024 06:04
@spacey-sooty
Copy link
Contributor Author

/format

@spacey-sooty spacey-sooty changed the title Deprecate RobotInit Prepare for RobotInit deprecation by updating examples Jun 5, 2024
@PeterJohnson PeterJohnson merged commit 57fa388 into wpilibsuite:main Jul 17, 2024
29 checks passed
@spacey-sooty spacey-sooty deleted the deprecaterobotinit branch July 17, 2024 03:27
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()
8 participants