Generic single motor subsystem

[ThatXliner]

Implemented for Intake as an example.

The use of these generic classes are up to developer preference. It should not be used for subsystems that aren't finalized to have a single motor (as changing the subsystem implementation requires a minuscule amount of effort, although still copy+pasting nonetheless)

[chaoticthegreat]

this kind of ruins the purpose of using io layers. just doglog atp

[ThatXliner]

this kind of ruins the purpose of using io layers. just doglog atp

I don't see how, and either way it reduces the code duplication by a lot

[chaoticthegreat]

if ur gona abstract all of this away, you can just use doglog and pass in a motor

[ThatXliner]

if ur gona abstract all of this away, you can just use doglog and pass in a motor

IO layers let us swap an implemention with a simulated component easily

[chaoticthegreat]

That's not hard to do with doglog, its maybe an extra 10-15 lines of code in the subsystem file

[chaoticthegreat]

The idea is that your IO layers are actually different for each subsystem, like we don't need to temp monitor a pivot motor.

[ThatXliner]

The idea is that your IO layers are actually different for each subsystem, like we don't need to temp monitor a pivot motor.

If that's what you're worried about, then it could be done under a feature flag. Either way, isn't the idea to log as much as you can?

That's not hard to do with doglog, its maybe an extra 10-15 lines of code in the subsystem file

Agreed, however a lot of existing code uses the autologged classes and i'm pretty sure AdvantageScope gives it special treatment (at least visually)

[chaoticthegreat]

No that's not what I'm worried about, it was just an example. Logging useless stuff just increases canbus utilization. Advantagescope giving it special treatment is not a real reason. Just put it under the subsystem and under the device.

[chaoticthegreat]

Again the IO classes are not supposed to be all the same, in fact a lot of them right now could have stuff removed. With what you are doing right now it just making a wrapper around TalonFX and logging it. Which if you really wanted to do, you could again just doglog, or B just use ctre status signal logger and turn off signals you don't need. I agree right now it looks like its a code dupe, but that's not how its supposed to be.

See 8033 as an example.

[ThatXliner]

With what you are doing right now it just making a wrapper around TalonFX and logging it. Which if you really wanted to do, you could again just doglog

Yes, but that's literally what the existing code is doing as well.

B just use ctre status signal logger and turn off signals you don't need

def should probably look into that

[chaoticthegreat]

We already do the CTRE status signal logger. But it also adds an extra step since the files need to be converted and it doesn't stream to NT afaik.

The existing code is supposed to get rid of functionality on motors we don't need while also logging the necessary values. So if we wanted to just control a motor off just voltage it would only expose a setVoltage method. Right now for motors that don't need PID we add it anyways just in case.

[ThatXliner]

...in other words you want me to split it into a servosubsystem and flywheelsubsystem (or positionsubsystem and velocitysubsystem)

[chaoticthegreat]

If this is such a must have thing, just switch to doglog. I don't see the point at all, besides just making debugging harder and if something was needed for an IO layer specific to that subsystem it would just render this useless.

[realjoshuau]

This is ramble-y because these are not my full thoughts in regards to this, and there were many different points brought up here and because I need to college apps)

I have a few more thoughts (especially with regards to opting-into signals, I think all of them should be published, but at a low freq) I'll post later, but @ThatXliner - the concept behind your PR is good and might(?) be functional (idk, I have not tested it). I also don't agree that it completely ruins the purpose of IO layers but that's for later.

However, my thoughts are that you need to minimize maintenance burden on your code base -- just because it's a few more lines or a couple extra files shouldn't warrant an abstraction class for them*, and this burden increases if you reuse it year-over-year. There are scenarios in which you can abstract it away, but so far, we have not needed to.

*This can depend on how much logic is implemented. ServoSubsystems are common to be abstracted which is iffy but there is a good reasoning usually for it.

(This is not a review.)

[chaoticthegreat]

I think an actual good middle ground is similar to 6328, have one for an elevator, an arm, a flywheel, and rollers.

[ThatXliner]

if something was needed for an IO layer specific to that subsystem it would just render this useless.

yes it would, and i would hope so ( although subclassing can still help a lot with reducing boilerplate). But so far, that simply isn't the case for a lot of subsystems

debugging harder

Perhaps, but at the same time I don't see how (except for potentially convoluted stack traces)