-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add lights on java side to tell difference between number of cameras … #273
Conversation
…ag design might not reflect changing to custom and change back to default
return 1; | ||
} | ||
// If none of the cameras are working, return 0 | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this logic doesn't match up with what you declared in the enum above:
currentState = 8 should be no cameras working
9 should be all cameras working
10 should be some cameras working
the same applies for custom autos as well.
src/main/java/competition/Robot.java
Outdated
@@ -59,7 +59,7 @@ protected void initializeSystems() { | |||
var autoSelector = getInjectorComponent().autonomousCommandSelector(); | |||
|
|||
autoSelector.setCurrentAutonomousCommand(defaultAuto); | |||
autoSelector.setIsDefault(true); | |||
//autoSelector.setIsDefault(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐ suggest just removing this if we're not going to support it anymore
@@ -32,6 +36,12 @@ public class LightSubsystem extends BaseSubsystem { | |||
public enum LightsStateMessage{ | |||
NoCode(15), // we never send this one, it's implicit when the robot is off | |||
// and all of the DIOs float high | |||
SomeCamerasWorkingCustom(13), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐ someone just reading these might not know what Custom vs Default refer to, consider being more explicit.
Example idea: DisabledDefaultAutoSomeCamerasWorking
Also do these states not replace 6-7 in principal?
@@ -433,4 +433,17 @@ public void refreshDataFrame() { | |||
} | |||
} | |||
} | |||
|
|||
public int cameraWorkingState() { | |||
if (allCameras.stream().anyMatch(state -> !state.isCameraWorking())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐ ⭐ ⭐ I think this logic might have a problem, this is just saying any of the cameras aren't working, so wouldn't this be true if they're all not working?
Would suggest flipping the logic to:
- check if all are working, return 0
- else if any are working, return 2
- else return 1
DisabledCustomAutoAllCamerasWorking(11), | ||
DisabledDefaultAutoSomeCamerasWorking(10), | ||
DisabledDefaultAutoNoCameraWorking(9), | ||
DisabledDefaultAutoAllCamerasWorking(8), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⭐ why not still use 6-7 up vs sliding everything over?
…working
Why are we doing this?
Add camera states into the light system
Asana task URL:
Whats changing?
Questions/notes for reviewers
How this was tested