DeepBlueRobotics / lib199

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

Auto-generate *Sim classes for CAN devices #98

Open brettle opened 1 month ago

brettle commented 1 month ago

Currently, using the sim support provided by lib199 is error prone because it requires knowing the names of the various devices and properties and using those names (as strings) to create and use SimDeviceSim objects. To address this:

  1. Use the asyncapi-generator like we do in DeepBlueSim, to auto-generate *Sim classes for the various CAN devices we create SimDevices for. These would have names like CANEncoderSim, CANMotorSim, CANDutyCycleSim, etc. The constructors would take the class name (e.g. CANSparkMax), port number, and an optional "input" name (for devices with multiple inputs).
  2. Make the public API provided by the *Sim classes as consistent as possible with the API provided by WPI's *Sim classes. In particular, the register*Callback methods should return a CallbackStore subclass and any cancel*Callback methods should not be public.
  3. Create simple subclasses of the above *Sim classes for each of the actual device objects we simulate (e.g. CANSparkMaxSim extends CANMotorSim) and have the constructors for those take the actual device objects (e.g. a CANSparkMax) and construct the appropriate class name to pass to the *Sim super constructor.

Side note: Although we could reuse these *Sim classes in DeepBlueSim.jarWPIWebSockets.jar (instead of auto-generating separate *Sim classes there), that would have the following disadvantages:

  1. Reusing the result of 1 above would involve either making DeepBlueSim.jarWPIWebSockets.jar depend on lib199.jar or putting the *Sim classes in a separate jar that both lib199.jar and WPIWebSockets.jar (and robot projects) depend on.
  2. Doing 2 would make DeepBlueSim.jarWPIWebSockets.jar depend on wpilib.
  3. Doing 3 would make DeepBlueSim.jarWPIWebSockets.jar depend on vendordeps if the sims weren't split into a separate jar.

Those extra dependencies don't seem worth the effort since the "real" common API is defined by the wpilib-ws.yaml file use to auto-generate the *Sim classes in both projects anyways.

CoolSpy3 commented 1 month ago

Implementation note for 1: Because these are basically SimDevices. It could make sense to extend SimDevice, and provide a constructor that takes (int handle), and then add create methods that have similar logic to SimDevice.create().

For 3, I'm not sure about accepting actual device objects as part of the constructor. This works for CANCoders because phoenix provides a programmatic sim interface for them, but I'm not sure if such a thing exists for other phoenix/rev devices. This would also affect our ability to intercept method calls. We could do this by extending the vendor classes, but

  1. This would still call the constructors of those classes, and
  2. We then couldn't extend the *Sim classes

I think a better option might be to do this same thing, but just let the <Device>Sim classes replace the mock implementation classes that we currently use.

For the note section, I think you mean to refer to WPIWebSockets rather than DeepBlueSim. DeepBlueSim already depends on WPILib and does not define any *Sim classes. I agree with the points you posted here, and (to add another), the *Sim classes defined in WPIWebSockets are designed to operate after being connected to a running robot project. They would not function correctly in lib199. Furthermore, it would be preferable to tie into the existing WPILib framework rather than a custom websocket backend.

UPDATE: I found out that CTRE does have a simulation API for most devices. This makes means that a possible (and imo, best) solution is to deprecate the current phoenix simulation classes in favor of the method mentioned above. I'm still not sure about rev tho

brettle commented 1 month ago

Implementation note for 1: Because these are basically SimDevices. It could make sense to extend SimDevice, and provide a constructor that takes (int handle), and then add create methods that have similar logic to SimDevice.create().

To be clear, this issue is about creating *Sim classes that are wrappers around SimDeviceSim objects that work with the existing sim data currently produced by lib199. Those *Sim classes would be used in robot code for testing/simulation. They would be more closely related to SimDeviceSim than SimDevice. Creating a *Sim should generally not result in the creation of a SimDevice. The SimDevice should (and currently is) created when the real device (e.g. CANSparkMax) is created. In short, what you're proposing doesn't make sense to me.

For 3, I'm not sure about accepting actual device objects as part of the constructor.

The *Sim class would just use the actual device object to get the device's port/id so that it could construct an appropriate SimDeviceSim object. A constructor that takes the actual device object is handy because it doesn't depend on the caller to make sure they are using the same port/id as the actual device they are intending to create the sim for.

This would also affect our ability to intercept method calls.

I don't see how. I suspect there is just a miscommunication.

I think a better option might be to do this same thing, but just let the <Device>Sim classes replace the mock implementation classes that we currently use.

I disagree. The *Sim classes provide the simulation-facing api. The mocks provide the robot-facing api. The communication between them goes through wpilib's sim infrastructure (via the SimDevice<-->SimDeviceSim connection).

For the note section, I think you mean to refer to WPIWebSockets rather than DeepBlueSim.

Yep. My bad. Fixed now.

**UPDATE: I found out that CTRE does have a simulation API for most devices. This makes means that a possible (and imo, best) solution is to deprecate the current phoenix simulation classes in favor of the method mentioned above.

The CTRE sim support is not compliant with the wpi-ws spec, at least in that a CANCoder does not have the correct device name. Before switching to just using their simulation we'd also want to see how complete it is. For example, does it accurately simulate controller-based PID? In the interim, I recommend we keep the mock and reuse/wrap their simulation support wherever we can. Further discussion on this topic should probably move to another issue.

I'm still not sure about rev tho**

As of this writing, Rev's simulation support is extremely limited and not tied into the wpilib sim infrastructure afaict. Bottom line, it isn't suitable for our purposes.

CoolSpy3 commented 1 month ago

Ahhh, my bad. I thought you were suggesting creating wrappers around existing SimDevices in order to avoid creating the relevant SimDevices/SimValues manually. If you're instead talking about creating SimDeviceSims that correspond to the higher-level classes for use in something like RobotCode2024/subsystems/Arm, these suggestions make much more sense. Feel free to disregard my previous comment.

brettle commented 1 month ago

Yep, we're on the same page now.