-
Notifications
You must be signed in to change notification settings - Fork 0
led code #10
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
led code #10
Conversation
Luddo183
left a comment
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.
Overall, the logic and control is good. There's a few implementation things that need to be tweaked, and the code needs to rewritten to use the latest Phoenix6 API. Start by deleting the Phoenix5 vendordep, and see what you need to rewrite from there.
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.
Add a function to set the entire strip (all it needs to do is call setFirst and setSecond, but a single function to do both would be nice)
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.
Avoid using the Phoenix specific Animation type in the generic IO. You could make a custom class that extends Animation, or a class that can convert itself to a Phoenix Animation (eg. PIDFFConfigs).
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.
Remember integers aren't always perfectly divisible by 2
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.
Not sure how hard it would be to implement, but it would be possible to also animate halves (eg. top half is the rainbow animation, bottom is the fire animation)? Not necessary though.
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.
Avoid using driver/operator naming, stick to the first/second naming.
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.
The comments on the class are nice, but don't explain the functions in the class doc. Just a high-level explanation of what the class does.
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.
If you are using a periodic() function, would it useful to have the class extend SubsystemBase so the periodic function gets called automatically? It would also let you use commands to set the LEDs, and let the CommandScheduler handle conflicts.
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.
The default state for the LEDs should always be off, and not flashing white.
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.
Isn't there a better way to do flashing? If you already using animations, why not create a flashing animation?
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.
Going forward, avoid changing stuff in other files not related to the task at hand. These are just comments, so I'll leave them, but if you want to edit something else, make another branch.
vendordeps/Phoenix5-5.35.1.json
Outdated
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.
Phoenix5 is no longer necessary, Phoenix6 now includes the CANdle stuff.
LedIO + CANdle and Sim implementations
LedStrip for high level control