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

19 renaming ioinputs to inputs and iooutputs to outputs #22

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

minhnguyenbhs
Copy link
Contributor

very minor heading name changes, should work normally

Copy link
Member

@jkleiber jkleiber left a comment

Choose a reason for hiding this comment

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

I think the renaming looks correct, however the unrelated changes need to be explained/addresses before we merge this

@@ -32,23 +29,24 @@ public void robotInit() {
Logger.addDataReceiver(new WPILOGWriter()); // Log to a USB stick ("/U/logs")
Logger.addDataReceiver(new NT4Publisher()); // Publish data to NetworkTables
new PowerDistribution(1, ModuleType.kRev); // Enables power distribution logging
} else if (Constants.currentMode == Constants.Mode.SIM) {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we reverting this stuff? Shouldn't we maintain a sim mode separate from replay mode.

Also why are we doing this in this PR? I would expect renaming only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this was done in Aiden's reformatting branch- this is branched off of that- and there was an issue with the replaying log?

@@ -109,8 +124,8 @@ private void configureBindings() {
.onFalse(new InstantCommand(() -> intakeSubsystem.run(IntakeAction.NONE)));
}

if (Constants.FeatureFlags.runScoring) {

if (FeatureFlags.runScoring && Robot.isReal()
Copy link
Member

Choose a reason for hiding this comment

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

Why are we making this change?

One core idea behind the feature flags is to make the configuration identical in sim as on the robot. The simulated robot is a "digital twin" to the real life robot. So if we have two sets of flags that could result in configuration management problems ("it works in sim but not on the robot" because the feature flags aren't harmonized, or worse "the subsystem was disabled in sim but not in real life").

@minhnguyenbhs
Copy link
Contributor Author

I think both of these changes are mirrors of stuff on Aiden's branch- should I redo the renaming so that it is branched off of main, or should I wait until Aiden's PR solves things and rebase then? thank you!

@jkleiber
Copy link
Member

jkleiber commented Oct 2, 2024

Let's wait until that branch is fixed and then rebase @minhnguyenbhs

@aidnem
Copy link
Contributor

aidnem commented Oct 7, 2024

@minhnguyenbhs I fixed my branch and merged it, you can merge main into yours and the problems should be resolved now. Sorry for propagating all of my garbage code into your branch lol

@minhnguyenbhs minhnguyenbhs requested a review from jkleiber October 7, 2024 22:28
Copy link
Member

@jkleiber jkleiber left a comment

Choose a reason for hiding this comment

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

Approved!

@jkleiber jkleiber merged commit 7ab84f2 into main Oct 8, 2024
3 checks passed
@jkleiber jkleiber deleted the 19-renaming branch October 8, 2024 13:40
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.

3 participants