DeepBlueRobotics / lib199

Code that we reuse in different projects/years.
Other
2 stars 0 forks source link

Unnecessary Simulation Check #44

Closed CoolSpy3 closed 1 month ago

CoolSpy3 commented 1 year ago

https://github.com/DeepBlueRobotics/lib199/blob/432cd16685f4d50eafe6f345dd897543401f1cae/src/main/java/org/carlmontrobotics/lib199/MotorControllerFactory.java#L122 It looks like Rev might've fixed the need for this in 2022.1.0. Someone should investigate.

brettle commented 1 month ago

Looks like this was fixed here.

CoolSpy3 commented 1 month ago

Could you clarify? The check remains in-place here. The point of this issue (although, I admit my description above is terrible) was to investigate whether rev simulation (or at least the necessity of not calling rev constructors in sim) was still relevant after the listed update. If it wasn't the simulation check could be removed, and we could possibly investigate less-hacky (non-Mockito) ways to perform simulation.

brettle commented 1 month ago

Oh. I misunderstood and thought you were referring to something else.

To answer your question, I believe the rev update you are referring to prevents crashing when the calling constructors in sim (though I haven't verified on all platforms), but the actual simulation support it provides is not connected to the wpilib sim infrastructure at all. As a result, I believe we still need the mock.

CoolSpy3 commented 1 month ago

Good to know. Although, if the constructor no longer crashes the simulation, the idea was that we could consider extending CANSparkMax and simply overriding the relevant methods, which may have less overhead than Mockito (although, it would make our Mock classes less generalizable).

brettle commented 1 month ago

Oh. Why didn't I think of subclassing?! Can you create a separate issue to investigate doing that and in it describe in more detail what you see as the possible advantages/disadvantages?

CoolSpy3 commented 1 month ago

Done!