-
Notifications
You must be signed in to change notification settings - Fork 4
Create UNSET enum #29
base: master
Are you sure you want to change the base?
Conversation
There shouldn't be an NPE anywhere with this, but somewhere you should be checking to see if the color is UNSET. I think there's somewhere in the code where there's a null check? Maybe one should go before here:
|
All of this color sensing code seems very complex. I wonder if there's a way to simplify it. |
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 agree with Tim that it's surprising no if statements that previously checked for null have to be changed to UNSET. It might be that we'll have a use for it when we bind the Control Panel Color Spin command to a button.
@@ -12,7 +12,8 @@ | |||
RED(new Color(0.325, 0.453, 0.221)), | |||
GREEN(new Color(0.269, 0.550, 0.181)), | |||
BLUE(new Color(0.175, 0.469, 0.356)), | |||
YELLOW(new Color(0.188, 0.503, 0.310)); | |||
YELLOW(new Color(0.188, 0.503, 0.310)), | |||
UNSET(null); |
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 would actually make this UNKNOWN
rather than UNSET
because without context, if a color isn't one of those it's uknown. Only in the context of the driverstation sending a color is it unset. It'll also make sense if you return UNKNOWN
when the confidence for the color matcher is low, whereas UNSET
wouldn't make as much sense in that context
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.
And I think you should give it a default value of either 0,0,0
or 1,1,1
. Setting null will cause an NPE when the color matcher tries to match to it.
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.
Actually I do realize now that the addition of the UNSET/UNKNOWN constant would break the math for the turn to color calculation. I have an idea on how to simplify the Color/Colorsenor classes but I'll make an issue for that.
Fixes #25
I added the enum but I'm pretty sure this is just adding another layer of abstraction for a NPE error.