DeepBlueRobotics / lib199

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

Subclass Vendor Objects Rather than Mocking Them #101

Open CoolSpy3 opened 4 months ago

CoolSpy3 commented 4 months ago

This issue is long and contains a bunch of different ideas. It might be better to work on it as multiple separate issues, but for now, we can just use it to start discussion.

We arrived at the current mocking framework for our simulation classes because, in the past, initializing vendor devices in a sim environment led to crashes. However, as discussed in #44, this is no longer the case. Thus, someone should look into whether it would be beneficial to move to a framework where we instead subclass the relevant vendor objects.

Benefits

Costs

Implementation note: Phoenix already provides SimCollection objects for many of their devices. For these devices, it might be preferable to instead adopt a model similar to the current implementation of MockedCANCoder. Depending on how much CTRE has implemented (which someone should check), it might prevent the need to handle a bunch of features ourselves.

Furthermore, while Phoenix devices don't exactly conform to the WPILib spec, they're pretty close. If all of their API endpoints are fully situatable (which someone should check), It might be worth trying to contact them to ask if they would bring it up to spec. This would remove the need for us to do any work whatsoever for CTRE devices. In my (very quick) skim, the only things I noticed were

  1. CANcoders have their position set as an output rather than an input. Imo, they should rewrite this to be an input and set it using a SimDevice rather than directly, but given how their API works, I see why they'd do this.
  2. The values introduced in wpilibsuite/allwpilib#6651 are not implemented. (Although those were added <1 wk ago, so I'd be surprised if they were.)
brettle commented 4 months ago

Another potential disadvantage to the subclassing approach is that if any of the classes we want to subclass are final, or any of the methods we need to override are final, the subclassing approach won't work. Moreover, even if that isn't a problem at the time we decide to pursue subclassing, if the vendor later makes such things final, we would likely need to revert to using Mockito.