Conversation
AniKashyap
left a comment
AniKashyap
left a comment
There was a problem hiding this comment.
Good work making this work! Needs some work on the refactor the make it compatible and easy to integrate into a future codebase.
| public static final CANdi mCandi = getEndeffectorCandi(); | ||
|
|
||
| public static CANdi getEndeffectorCandi() { | ||
| CANdi candi = new CANdi(Ports.CANDI.id, Ports.CANDI.bus); | ||
|
|
||
| CANdiConfiguration candiConfiguration = new CANdiConfiguration(); | ||
| candiConfiguration.DigitalInputs.S1CloseState = S1CloseStateValue.CloseWhenNotHigh; | ||
| candiConfiguration.DigitalInputs.S1FloatState = S1FloatStateValue.PullLow; | ||
| candiConfiguration.DigitalInputs.S2CloseState = S2CloseStateValue.CloseWhenNotHigh; | ||
| candiConfiguration.DigitalInputs.S2FloatState = S2FloatStateValue.PullLow; | ||
|
|
||
| candi.getConfigurator().apply(candiConfiguration); | ||
|
|
||
| return candi; | ||
|
|
||
| } |
There was a problem hiding this comment.
Can we not define the creation of the CANdi and all this supplementary stuff like a beam break, in a different place? Would like a super simple call in Superstructure.java
| return new BeamBreakIODigitalIn( | ||
| Ports.END_EFFECTOR_CORAL_BREAMBREAK.id, | ||
| return BeamBreakIOCANdi.makeInverted( | ||
| 1, |
There was a problem hiding this comment.
This is bad coding practice to define the channel in this file. This needs to be defined in ports.java
| return new BeamBreakIODigitalIn( | ||
| Ports.END_EFFECTOR_ALGAE_BEAMBREAK.id, | ||
| return BeamBreakIOCANdi.makeInverted( | ||
| 2, |
There was a problem hiding this comment.
This is bad coding practice to define the channel in this file. This needs to be defined in ports.java
There was a problem hiding this comment.
@AniKashyap can I make the I/O port a boolean because the CANdi only accepts two DI/Os?
There was a problem hiding this comment.
Personally, I think it is better to take it as an int as it is possible that in the future, CANDi's will be able to support more DI/O ports and is much eaiser to read as you can tell which port it is on instantly but that is peronal prefences to be honest.
JaydenHuang555
left a comment
JaydenHuang555
left a comment
There was a problem hiding this comment.
The code looks awesome!!! There are a few areas that I would change but it is great overall!!
| }; | ||
| } | ||
|
|
||
| public BeamBreakIOCANdi(boolean isChannelOne, Time debounce, String name, CANdi candi) { |
There was a problem hiding this comment.
Personally, I think this will be more easier to read if we made a BeamBreakCANdiConfig class that stores the params that getted passed in (port, configuration, etc) rather than just passing in all of these params.
| this.mCANdi = candi; | ||
|
|
||
| this.isChannelOne = isChannelOne; | ||
|
|
There was a problem hiding this comment.
The caller should pass the config in since the config might not be the same for all candi beambreaks
| public static CANdi getCANdi() { | ||
| CANdi candi = new CANdi(Ports.CANDI.id, Ports.CANDI.bus); | ||
|
|
||
| CANdiConfiguration candiConfiguration = new CANdiConfiguration(); |
There was a problem hiding this comment.
The applying configuration is called twice. Once in this method, the other is in the constructor of the BeamBreakCANdiIO
| return new BeamBreakIODigitalIn( | ||
| Ports.END_EFFECTOR_ALGAE_BEAMBREAK.id, | ||
| return BeamBreakIOCANdi.makeInverted( | ||
| 2, |
There was a problem hiding this comment.
Personally, I think it is better to take it as an int as it is possible that in the future, CANDi's will be able to support more DI/O ports and is much eaiser to read as you can tell which port it is on instantly but that is peronal prefences to be honest.
| import com.ctre.phoenix6.configs.CANdiConfiguration; | ||
| import com.ctre.phoenix6.hardware.CANdi; | ||
| import com.ctre.phoenix6.signals.S1CloseStateValue; | ||
| import com.ctre.phoenix6.signals.S1FloatStateValue; | ||
| import com.ctre.phoenix6.signals.S2CloseStateValue; | ||
| import com.ctre.phoenix6.signals.S2FloatStateValue; | ||
|
|
3 participants
No description provided.