Jack pipelines PR BY JACK #36
Conversation
rafibaum
left a comment
rafibaum
left a comment
There was a problem hiding this comment.
My comments aren't mandates really but suggestions. I think when making a library it's important to have a certain amount of organisation when creating the code. Having power mappers be in a separate class is something that I think makes the code a lot easier to change and a lot more explicit since the math is all in one class as opposed to having bits and pieces scattered in the drive train, drive module, and as bits in the pipeline. Overall I really like your pipeline design and once you address the feedback I submitted I think it'll be good to merge.
| import edu.wpi.first.wpilibj.Joystick; | ||
|
|
||
| public class ArcadeDriveInput extends DriveTrainInput { | ||
| public class ArcadeDriveInput implements Step<DriveTrainTarget> { |
There was a problem hiding this comment.
This change makes joystick inputs indiscernible from other steps in the pipeline (previously I was using DriveTrainInput which stores the direction, rotation, and whether magnitudes were stored as speed or power) and also makes it less straightforward to create other control translators. Me and @rakar (Rich) were having a discussion on Slack earlier about whether that was a necessary distinction. Do you think that this change makes the code base easier to use overall?
There was a problem hiding this comment.
I have no clue. Yes? I think so maybe.
There was a problem hiding this comment.
Maybe a better question to ask is if human/autonomus mode inputs should be distinct from other steps in the pipeline.
There was a problem hiding this comment.
No. (an arbitrary decision, and it is simpler that way).
There was a problem hiding this comment.
Then maybe in the Pipeline class could we just have a "setInput" method which sets the first step in the pipeline and replaces whatever was previously in the first spot?
| import org.montclairrobotics.sprocket.loop.Updater; | ||
| import org.montclairrobotics.sprocket.pipeline.Pipeline; | ||
|
|
||
| public class DriveModule implements Updatable{ |
There was a problem hiding this comment.
Should all this math go in the drive module class? I think it would be better to separate the power mapping stuff into a power mapper and leave the drive module class for handling PIDs and controlling motor powers. We're giving up on the general mapper. Make it so the mappers can be swapped
There was a problem hiding this comment.
OK I will make it swappable, but I am not giving up.
There was a problem hiding this comment.
That's fine, but for kickoff the generic mapper won't be a priority.
| DriveTrainTarget target=input.get(); | ||
| Vector tgtDirection=target.getDirection(); | ||
| Angle tgtTurn=target.getTurn(); | ||
| for(DriveModule module:modules) |
There was a problem hiding this comment.
Again, I feel like all of this sort of stuff should go through a power mapper implementation and shouldn't be hard coded. Not necessarily for the sake of having power mappers be interchangeable, but just for the sake of organisation. Same as above
There was a problem hiding this comment.
Same as above
There was a problem hiding this comment.
Same as above.
|
|
||
| import java.util.ArrayList; | ||
|
|
||
| public class Pipeline <T>{ |
There was a problem hiding this comment.
How are pipelines run/updated/looped?
There was a problem hiding this comment.
pipeline.get() runs it. They are not a standalone process, they run when they are requested. This can be changed as it doesn't fit with our strategy for the rest of Sprocket, but in this instance I think it is simpler and fits with what we are trying to do. Also easier to ensure which steps happen when.
There was a problem hiding this comment.
You could make it implement Updatable but not add it to the Updater. That way we have some sense of consistency but we still maintain control over when we run the pipeline.
There was a problem hiding this comment.
I don't like that at all. Everything that implements Updatable should use Updater. That would be confusing and not clear, and it erodes the standards we set.
No description provided.