Conversation
|
This looks like it has a lot more than just command based? Looks like it was a merge tho so it's fine |
spellingcat
left a comment
There was a problem hiding this comment.
yeah i think i already looked at most of this in other prs
| The important segment of all of this to remember is: | ||
| > Commands represent actions the robot can take. Commands run when scheduled, until they are interrupted or their end condition is met. Commands are very recursively composable: commands can be composed to accomplish more-complicated tasks. See Commands for more info. | ||
| > | ||
| > Subsystems represent independently-controlled collections of robot hardware (such as motor controllers, sensors, pneumatic actuators, etc.) that operate together. Subsystems back the resource-management system of command-based: only one command can use a given subsystem at the same time. Subsystems allow users to “hide” the internal complexity of their actual hardware from the rest of their code - this both simplifies the rest of the robot code, and allows changes to the internal details of a subsystem’s hardware without also changing the rest of the robot code. |
There was a problem hiding this comment.
not sure if you want to have this as a block quote, do you also want to add a summary for triggers?
There was a problem hiding this comment.
these are taken directly from the wpilib docs, hence the block quote. not sure if it still needs to be there since its decently redundant w the intro
Docs/Architecture/CommandBased.md
Outdated
|
|
||
| A Subsystem is a set of hardware that forms one system on our robot, like the drivetrain, elevator, arm, or intake. | ||
| Each subsystem contains some associated hardware (motors, pistons, sensors, etc.) They are the "nouns" of our robot, what it is. | ||
| Each Subsystem is generally made to contain as broad of a set of hardware that will always operate as a unit as possible. |
There was a problem hiding this comment.
"as possible" can probably be dropped here? Unless you mean 'if' but would be kind of weird to have parts of a subsystem not working in concert? Expert level choice to work around that and pretty specific circumstances.
Docs/Architecture/CommandBased.md
Outdated
|
|
||
| Subsystems are ways to organize resources that can be used by one Command at a time. | ||
|
|
||
| Some hardware might not be stored in a Subsystem if multiple things can/should use it. |
There was a problem hiding this comment.
"if multiple things can use it at the same time safely"?
| ```Java | ||
| // Runs the intake rollers forever | ||
| new RunCommand(() -> intakeSubsystem.spinRoller(), intakeSubsystem); | ||
| Commands.run(() -> intakeSubsystem.spinRoller(), intakeSubsystem); |
There was a problem hiding this comment.
intakeSubsystem::spinRoller
| // Note that the real hardware doesn't move instantly | ||
| // But we only need to set it once in code | ||
| new InstantCommand(() -> intakeSubsystem.retract(), intakeSubsystem); | ||
| Commands.runOnce(() -> intakeSubsystem.retract(), intakeSubsystem); |
There was a problem hiding this comment.
intakeSubsystem::retract
| // The subsystem does it implicitly | ||
| this.run(() -> spinRoller()); | ||
|
|
||
| this.runOnce(() -> retract()); |
There was a problem hiding this comment.
this::spinRoller and this::retract
Docs/Architecture/WPILib.md
Outdated
| WPILib is an open source library built for FRC which abstracts away the low level code to allow us to focus on the high-level control logic. | ||
| The project has a large number of features and is the main building block that our code is built on top of. | ||
| It also has extensive documentation both for it's own API and for the concepts and math behind many of its features. | ||
| Venders (companies that sell us parts) also often have libraries to interface with their parts. |
kevinclark
left a comment
There was a problem hiding this comment.
Generally very good work. Couple of minor notes.
| ### Electrical system diagram | ||
|
|
||
|  | ||
| <img alt="Rev electrical system diagram" src="../../Assets/RevElectricalSystemDiagram.png" width="400" height="400"> |
There was a problem hiding this comment.
Not loving the markdown inline?
There was a problem hiding this comment.
this was to make all images the same size, which did not appear to be possible with the ![]() syntax
bruh i thought i set it to merge to 2024 not main |
kevinclark
left a comment
There was a problem hiding this comment.
There are a variety of changes here that look like they shouldn't compile, or if they do, are internally inconsistent.
| > | ||
| > Subsystems represent independently-controlled collections of robot hardware (such as motor controllers, sensors, pneumatic actuators, etc.) that operate together. Subsystems back the resource-management system of command-based: only one command can use a given subsystem at the same time. Subsystems allow users to “hide” the internal complexity of their actual hardware from the rest of their code - this both simplifies the rest of the robot code, and allows changes to the internal details of a subsystem’s hardware without also changing the rest of the robot code. | ||
| - [WPILib intro to functional programming](https://docs.wpilib.org/en/stable/docs/software/basic-programming/functions-as-data.html). | ||
| Read through this article on lambda expressions and functional programming. |
There was a problem hiding this comment.
Is this covered elsewhere?
There was a problem hiding this comment.
it got moved up in the list
| - Pneumatics | ||
| - Sensors | ||
| - Mutex, prevents multiple things from using the same hardware at the same time | ||
| - [Mutex](https://en.wikipedia.org/wiki/Lock_(computer_science)), prevents multiple things from using the same hardware at the same time |
There was a problem hiding this comment.
Might note mutex comes from "mutual exclusion/mutually exclusive".
|
|
||
| public CommandBase setVoltagesCommand(DoubleSupplier left, DoubleSupplier right) { | ||
| return new RunCommand(() -> this.setVoltages(left.getAsDouble(), right.getAsDouble()), this); | ||
| return new RunCommand(this.setVoltages(left.getAsDouble(), right::getAsDouble), this); |
There was a problem hiding this comment.
This seems wrong. RunCommand is being passed a value not a function. And one of the two values to setVoltages is a function while the other is a value. Either acquiring left should be passed as a function or the whole thing should still be a lambda, otherwise we're binding to a constant parameter.
| () -> modifyJoystickAxis(controller.getLeftY()), | ||
| () -> modifyJoystickAxis(-controller.getLeftX()), | ||
| modifyJoystickAxis(controller::getLeftY), | ||
| modifyJoystickAxis(-controller::getLeftX), |
There was a problem hiding this comment.
A negative function seems wrong. This is one of the cases where you want to pass a lambda instead of a method reference - we need to change the value that comes out. Alternatively you could chain a function that negates the value but that's probably a lot more typing than just a lambda unless you have a 'not' implementation sitting around.
| // Command that wraps drive method | ||
| public CommandBase driveCommand(DoubleSupplier speed, DoubleSupplier angle, BooleanSupplier isClosedLoop) { | ||
| return new RunCommand(() -> drive(speed.getAsDouble(), angle.getAsDouble(), isClosedLoop.getAsBoolean()), this); | ||
| return new RunCommand(drive(speed.getAsDouble(), angle.getAsDouble(), isClosedLoop::getAsBoolean), this); |
There was a problem hiding this comment.
This shouldn't compile (passing void to RunCommand) and is a problem for the same issues above.
| () -> modifyJoystick(controller.getLeftY()), | ||
| () -> modifyJoystick(controller.getRightX()))); | ||
| modifyJoystick(controller::getLeftY), | ||
| modifyJoystick(controller::getRightX))); |
There was a problem hiding this comment.
This doesn't look like it should compile.
this is why i shouldnt have used a regex to find and replace stuff... |
| ```Java | ||
| public CommandBase setVoltagesCommand(DoubleSupplier left, DoubleSupplier right) { | ||
| return new RunCommand(() -> this.setVoltages(left.getAsDouble(), right.getAsDouble()), this); | ||
| return new RunCommand(this.setVoltages(left.getAsDouble(), right::getAsDouble), this); |
There was a problem hiding this comment.
This one probably still needs the lambda and to revert the right.getAsDouble() call.
| () -> modifyJoystick(controller.getLeftY()), | ||
| () -> modifyJoystick(controller.getRightX()))); | ||
| modifyJoystick(controller::getLeftY), | ||
| modifyJoystick(controller::getRightX))); |
There was a problem hiding this comment.
This probably needs to be reverted.
| var speeds = DifferentialDrive.arcadeDriveIK(drive.getAsDouble(), steer.getAsDouble(), false); | ||
| this.setVoltages(speeds.left * 12, speeds.right * 12); | ||
| }, this); | ||
| }); |
There was a problem hiding this comment.
These need to be reverted, yeah? setVoltages is void.
There was a problem hiding this comment.
this.run replaces the this at the end of the RunCommand constructor
| // Command that wraps drive method | ||
| public CommandBase driveCommand(DoubleSupplier speed, DoubleSupplier angle, BooleanSupplier isClosedLoop) { | ||
| return new RunCommand(() -> drive(speed.getAsDouble(), angle.getAsDouble(), isClosedLoop.getAsBoolean()), this); | ||
| return this.run(drive(speed.getAsDouble(), angle.getAsDouble(), isClosedLoop.getAsBoolean())); |
|
|
||
| public CommandBase setVoltagesCommand(DoubleSupplier left, DoubleSupplier right) { | ||
| return new RunCommand(() -> this.setVoltages(left.getAsDouble(), right.getAsDouble()), this); | ||
| return this.run(this.setVoltages(left.getAsDouble(), right.getAsDouble())); |
…ax checker would be a good idea
Awesomeplayer165
left a comment
There was a problem hiding this comment.
mostly fine, one comment about PathPlanner --> Choreo vendor
| Vendors (companies that sell us parts) also often have libraries to interface with their parts. | ||
| These include the [Phoenix library](https://v6.docs.ctr-electronics.com/en/stable/) made by CTRE, the manufacturer of the majority of our motor controllers. | ||
|
|
||
| We also use other libraries for other tasks that WPILib doesn't support. One of these is [Photonvision](../Specifics/Vision.md), a library that processes our camera feeds to turn them into useful target information. |
There was a problem hiding this comment.
on line 17, maybe change PathPlanner to Choreo, so we are more up to date.
* wording tweaks and updates to BasicGit.md * remove last names from BasicGit.md * brush up git workflow doc * add mac/linux git install link * pr feedback word tweaks * Vision 2024 (#69) * merged wpilib.md and wpilibintro.md * hide spectrum talk a bot more * specify reference sheet type Co-authored-by: spellingcat <70864274+spellingcat@users.noreply.github.com> * convert electronics pictures to html and add size tags * add `s to latex * fix readme wpilib link * add apriltag image * 2024 vision updates * remove obj detection link * feedback * hopefully explained multitag better --------- Co-authored-by: Lewis-Seiden <lewy@seiden.us> Co-authored-by: Lewis-Seiden <70111132+Lewis-Seiden@users.noreply.github.com> * added example stage 3 writeup * began supersturcture writeup * add explanation of a state trigger binding * first draft of sim article edits (#73) * first draft of sim article edits * feedback * fixed latex * never mind * it's working in the vscode preview hello * i'm so sorry * more writing * add conclusion to writeup * Update Stage-3-Writeups/Superstructure-Triggers.md Co-authored-by: Jacob Trentini <jacobtrentinidevelopment@gmail.com> * fix monolithic typo * Akit code structure reference (#81) * added akit structure reference doc * tweaked mermaid graph code * more tweaks to try to get mermaid graph working * more tweaks * format please * example works? * what i care about works? * wrote reference material * Prettified Code! * pneumatics are for losers booo [updated code examples to reflect current practices, no substantive content changes] --------- Co-authored-by: Lewis-Seiden <aPotatoeWithANaginata@gmail.com> Co-authored-by: spellingcat <spellingcat@users.noreply.github.com> * Docs: Choreo Stack and Codebase Integration (#74) * add choreo.md * add numerical optimization * update choreo article --------- Co-authored-by: Jacob Trentini <dev@jacob2.dev> Co-authored-by: spellingcat <70864274+spellingcat@users.noreply.github.com> * Command based docs updates 2024 (#70) * editing pass on CommandBased.md * kevin pr feedback * fixed examples * fix kitbot examples\ (who wouldve thought that installing a java syntax checker would be a good idea * replace pathplanner reference to choreo reference in wpilib intro * update projects to 2025 (surely there's a better way to do this 😭) * import basic project also * update command based articles --------- Co-authored-by: Lewis-Seiden <lewy@seiden.us> Co-authored-by: spellingcat <70864274+spellingcat@users.noreply.github.com> --------- Co-authored-by: Lewis-Seiden <lewy@seiden.us> Co-authored-by: spellingcat <70864274+spellingcat@users.noreply.github.com> Co-authored-by: Jacob Trentini <jacobtrentinidevelopment@gmail.com> Co-authored-by: spellingcat <spellingcat@users.noreply.github.com> Co-authored-by: Jacob Trentini <dev@jacob2.dev>
No description provided.