Added somewhat thorough documentation to lib2813#66
Conversation
|
|
|
There goes my afternoon |
kcooney
left a comment
kcooney
left a comment
There was a problem hiding this comment.
Thanks for improving our documentation.
I didn't review all of the classes (this PR is quite large).
| /** | ||
| * Open-loop duty cycle control mode. | ||
| * | ||
| * <p>Controls the motor by setting the duty cycle (percentage of full power) directly. |
There was a problem hiding this comment.
Technically, the duty cycle is the percentage of time the signal sent to the motor is "high" (see https://learn.sparkfun.com/tutorials/pulse-width-modulation/duty-cycle and https://docs.revrobotics.com/brushless/spark-max/control-interfaces).
The Spark motor code translates a 1.0 to be the maximum pulse width, and -1.0 as the minimum pulse width. If the motor receives a signal with the maximum pulse width it will drive the motor forward at full power.
| * DeviceInformation objects in hash-based collections such as | ||
| * HashMap and HashSet. | ||
| * | ||
| * <p>The hash code formula combines the ID and canbus hash using |
There was a problem hiding this comment.
Please don't document the hash algorithm. Callers should not depend on the exact implementation, and documenting it locks us in to a specific algorithm.¹
While I appreciate the attention to detail, I personally feel that the documentation you have here is a bit much. hashCode() is a well understood method, and Object.hashCode() already documents that implementations need to be consistent of the object hasn't been changed, and documents that equal values must have the same hash code.
¹ The original algorithm for String.hashCode() caused a lot of collisions. Joshua Bloch wanted to fix it, but was worried that the JLS documented the algorithm. He realized it documented it incorrectly, so he decided it should be fine to rewrite it to have less collections since no one could have depended on the documented algorithm. If the documentation was correct, we might have been stuck with that problematic algorithm.
There was a problem hiding this comment.
+1, This is probably another example of Claude trying to "fill" some meaningful documentation.
| * <li>{@code "Drive/maxAngularVelocity"} (default: {@code 3.14})</li> | ||
| * </ul> | ||
| * | ||
| * <p>For record classes with many component values of the same type, it is strongly recommended |
There was a problem hiding this comment.
Please keep in the recommendation to use builders.
| /** | ||
| * Verifies that all values of the given record equal the values returned by {@link | ||
| * #updatePreferenceValues(ValuesKind)}. | ||
| * Asserts that the record contains values matching the updated preference values. |
There was a problem hiding this comment.
I think it would be helpful to keep the part of the previous doc that this asserts that the record has the same values as the one returned by updatePreferenceValues(ValuesKind)
Same request on the next method.
There are a lot of methods that subclasses have to implement, and they have similar names, and there are a lot of subclasses, so here I'd be fine with being very detailed.
| IllegalStateException.class, | ||
| () -> PersistedConfiguration.fromPreferences(preferenceName, UnrelatedRecord.class)); | ||
|
|
||
| // Assert: Exception has expected message |
There was a problem hiding this comment.
Please add these comments back.
See https://automationpanda.com/2020/07/07/arrange-act-assert-a-pattern-for-writing-good-tests/
| // Assert: Preferences injected | ||
| assertHasConfiguredDefaults(recordWithPreferences); | ||
| // Ensure the suppliers return the same value if called multiple times. | ||
| assertHasConfiguredDefaults(recordWithPreferences); |
There was a problem hiding this comment.
Where did all these tests go?
| @@ -30,98 +30,248 @@ | |||
|
|
|||
| /** | |||
| * LimelightHelpers provides static methods and classes for interfacing with Limelight vision | |||
| * cameras in FRC. This library supports all Limelight features including AprilTag tracking, Neural | |||
| * Networks, and standard color/retroreflective tracking. | |||
| * cameras in FRC (FIRST Robotics Competition). This library supports all Limelight features | |||
There was a problem hiding this comment.
Please revert all changes to this file.
This is copied from the Limelight git repo. Keeping the changes minimal makes it easier to see the implications of changes if we upgrade to a newer version.
(not sure why they released this as source and not a library)
FWIW, I would not shed any tears if we deleted the lib2813 limelight project next year
removed redundancies Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
update voltage documentation Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
| * | ||
| * @return the corresponding {@link ControlType} for SPARK controllers | ||
| */ | ||
| public ControlType getSparkMode() { |
There was a problem hiding this comment.
This is no longer used (the last references were removed in #10). Could we just remove the method and the sparkMode1 field?
cuttestkittensrule
left a comment
cuttestkittensrule
left a comment
There was a problem hiding this comment.
Haven't looked at too much of the changes, as this is a very large change, but this does not look ready to merge.
Here are some general thoughts I have:
- Some of the code isn't formatted, which makes it more difficult to actually review the code
- I don't think the
@authoris very useful, as- Team 2813 is an entity, not a person, so it doesn't describe who the author is
- This is in lib2813, so it is pretty clear that some person on the team made it
git blameorgit shortlogare much more descriptive when it comes to who actually worked on each thing- If we are really worried about people not knowing that we made it, we should just actually put the copyright disclaimer at the top of each file
- A lot of classes, interfaces, record classes, enums, etc were changed to state what type they were, which right below the javadoc comment and will be above the documentation in an actual render.
- There is some actual functionality changed, which I don't think we should include in this PR, as the scope of this PR is already huge, and we don't need any more
- A large portion of tests are straight up removed (this is very bad)
I will look at more of this later, and give more specific comments on the docs.
| /** | ||
| * Sets the position of the encoder using a raw double value. | ||
| * | ||
| * <p>This method uses {@link ConfigUtils#phoenix6Config(Runnable)} to ensure |
There was a problem hiding this comment.
The reference inside this link is invalid
| * Sets the position of the encoder using type-safe units. | ||
| * | ||
| * <p>This method accepts any {@link Angle} measurement and converts it to rotations | ||
| * internally. It uses {@link ConfigUtils#phoenix6Config(Runnable)} to ensure |
There was a problem hiding this comment.
Same here; this is an invalid reference
| * Sets the current heading (yaw angle) of the IMU. | ||
| * | ||
| * <p>This method updates both the hardware device and the cached heading value | ||
| * used for reset recovery. It uses {@link ConfigUtils#phoenix6Config(Runnable)} |
There was a problem hiding this comment.
The reference inside this link is invalid
|
|
||
| // Static method for getting the subject factory (for use with assertAbout()) | ||
| /** | ||
| * Factory for {@link Pose2dSubject}, used with {@link assertAbout}. |
There was a problem hiding this comment.
The {@link assertAbout} is an invalid reference
|
|
||
| // Static method for getting the subject factory (for use with assertAbout()) | ||
| /** | ||
| * Factory for {@link Pose3dSubject}, used with {@link assertAbout}. |
There was a problem hiding this comment.
The {@link assertAbout} reference is invalid
|
|
||
| // Static method for getting the subject factory (for use with assertAbout()) | ||
| /** | ||
| * Factory for {@link Rotation2dSubject}, used with {@link assertAbout}. |
There was a problem hiding this comment.
The {@link assertAbout} reference is invalid
|
|
||
| // Static method for getting the subject factory (for use with assertAbout()) | ||
| /** | ||
| * Factory for {@link Rotation3dSubject}, used with {@link assertAbout}. |
There was a problem hiding this comment.
The {@link assertAbout} reference is invalid
| * <p><b>Warning:</b> This method returns position without specifying units, | ||
| * making it unsafe for reliable position calculations. The actual units | ||
| * depend on the specific encoder implementation and configuration. |
There was a problem hiding this comment.
I don't think the warnings are very useful (here or later), as they are almost identical to the deprecation warnings, which makes them redundant. Additionally, they will be below the actual deprecation warning, so unless you have a compelling reason to put the redundant warning, it should probably go.
I have attached a screenshot of what a deprecation message looks like rendered, in case you don't know what the render looks like (from main, as there are javadoc errors preventing javadoc generation on this branch).
(also, if you just want to add more detail to the deprecation message, just do that; it will be more prominently displayed anyways)
There was a problem hiding this comment.
+1. Same grudge I have with all the "filler" documentation I've been flagging.
vdikov
left a comment
vdikov
left a comment
There was a problem hiding this comment.
Comments on 10/67 files.
More to come...
| * resets that can occur during competition. | ||
| * | ||
| * <p>The wrapper maintains a cached heading value that is restored after device resets, | ||
| * ensuring consistent heading readings across power cycles. The {@link #periodicResetCheck()} |
There was a problem hiding this comment.
This documentation adds quite some useful color to these libraries.
There might be occasional factual mistakes.
Here's potentially one: I'm not convinced that this class stores the heading across power cycles. It looks like the heading is cached just in the member variable currentHeading which, as far as I can tell, lives in the RoboRio memory at best. So this state potentially survives the restarting of the robot code, but not power-cycling the robot.
@cuttestkittensrule do you have more knowledge here? It looks like this code hasn't been used since 2023 robot, so the knowledge what's it purpose was might have been lost by now.
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Wrapper class for CTRE Phoenix 6 TalonFX motor controller (Falcon 500 motor). |
There was a problem hiding this comment.
This functionality is most likely not limited to the now older Falcon 500 motors. The Krakens we use in recent years also have TalonFX motor controllers.
So I think we can drop (Falcon 500 motor). here to make the documentation more generic and accurate.
| * | ||
| * @return the corresponding {@link ControlType} for SPARK controllers | ||
| */ | ||
| public ControlType getSparkMode() { |
| * <li>Immutable design for thread safety and reliable identity</li> | ||
| * <li>Input validation for CAN ID range [0, 62]</li> | ||
| * <li>Proper equals() and hashCode() implementation for use in collections</li> | ||
| * <li>Optional-based canbus representation for null safety</li> |
There was a problem hiding this comment.
I feel Claude added some of these bullet points just to say something :)
The last two bullet points feel like "fillers". It probably really wanted to get to 4 bullet points, so it added some rather trivial design details. The first two bullet points are (more) relevant.
| * <li>{@code "canivore"} - For CANivore devices</li> | ||
| * <li>{@code "CANivore_12345"} - For CANivore with specific serial number</li> | ||
| * <li>{@code null} - For RoboRIO default CAN bus</li> | ||
| * </ul> |
There was a problem hiding this comment.
These Examples bullet-list feels like "fillers" too. Drop them altogether. The documentation becomes too verbose and overwhelming.
For a fairly simple class DeviceInformation is, with a very straightforward purpose, Claude sure created a lot of documentation.
| * <li>Default implementations to ease migration from legacy methods</li> | ||
| * <li>Backward compatibility with existing code through deprecated methods</li> | ||
| * <li>Flexible unit conversion through the units system</li> | ||
| * </ul> |
There was a problem hiding this comment.
The "Key features:" list here is also a "filler" by Claude. There are other interfaces and classes where the "Key features" make sense, but there are several classes where it is just regurgitating some trivial info. The four points it lists here are pretty much captured in the paragraph above it.
Adding too much documentation has a cost. It creates too much visual clutter for someone reading through the code / javadoc to quickly grasp what is this class all about.
Classes that are rather trivial, should "communicate" this with also shorter and rather trivial documentation. Classes that do some really complex logic - they deserve longer and detailed explanation.
| * <p><b>Warning:</b> This method returns position without specifying units, | ||
| * making it unsafe for reliable position calculations. The actual units | ||
| * depend on the specific encoder implementation and configuration. |
There was a problem hiding this comment.
+1. Same grudge I have with all the "filler" documentation I've been flagging.
| * <p><b>Warning:</b> This method returns velocity without specifying units, | ||
| * making it unsafe for reliable velocity calculations. The actual units | ||
| * depend on the specific encoder implementation and configuration (could be | ||
| * RPM, radians/sec, rotations/sec, etc.). |
There was a problem hiding this comment.
Here and below, follow @cuttestkittensrule 's recommendation to remove Warnings and instead elaborate the @deprecated message if needed.
| * | ||
| * <p>Common implementations include: | ||
| * <ul> | ||
| * <li>TalonFX (Falcon 500) controllers with integrated encoders</li> |
There was a problem hiding this comment.
Nit: Drop "(Falcon 500)". TalonFX is used for the Krakens as well.
| * <li><b>Slot 1:</b> Position control parameters</li> | ||
| * <li><b>Slot 2:</b> Motion magic parameters</li> | ||
| * <li><b>Slot 3:</b> Alternative parameters for different mechanisms or conditions</li> | ||
| * </ul> |
There was a problem hiding this comment.
Not sure if this "Common slot usage" is real or completely made up.
My understanding is that the slots are so we can make the robot execute different operations with different control modes / settings. E.g., an elevator might need a precise, close-loop operation to go up at a very specific height to deliver a game piece. Once done, the same elevator might want to go down as quickly as possibe, no precision whatsoever so that it can be moving on the field again. So, we might want to implement two different control setups for up and down, but both can be "Motion magic" based. Or the first can be motion magic, the second could be velocity controlled. Something like this.
So long story short, maybe we should drop this list, unless we sure what it says is correct.
@cuttestkittensrule , please check my logic here.
There was a problem hiding this comment.
Looks like TalonFX only supports three slots (0 through 3) while Spark motors support four.
| import java.util.function.*; | ||
|
|
||
| /** | ||
| * Initializes the fields of a Record Class from values stored in {@link Preferences}. |
There was a problem hiding this comment.
It looks like Caude steam-rolled over some carefully crafted manual javadoc. Could you please restore it.
javadoc paragraphs start with `<p>` and don't need a closing `</p>`
…nfiguration.java Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
…nfiguration.java Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
…dIntakeSubsystemTest.java Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
…dIntakeSubsystem.java Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
…dIntakeSubsystem.java Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
…ferences.java Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
…ferences.java Co-authored-by: Kevin Cooney <kcooney@users.noreply.github.com>
…downgraded to jdk 21.
|
Okay, so I fixed all the proposed changes. Build was failing because I was using JDK 25 and not 21. There were also mild formatting violations for gradle. I did make a slight modification to the InputValidationTest so that the tests would actually pass, where the phrase "is not a valid can id" was modified to be "is not a valid CAN ID". There could possibly still be errors in the other unreviewed files; I haven't looked through them thoroughly yet. |
There was a problem hiding this comment.
@codingduck106 , large pieces of the original code of this test still need to be restored. Please address that.
(Look from public static class BooleanPreferencesTest onward)
|
temporarily closed until 2 things happen.
|
@codingduck106 when you send another PR updating Javadoc, could you please break it up by Java package? The reviews would go faster, and you would have fewer merge conflicts when you try to merge |
5 participants
I created some documentation for lib2813, but I was only going off of the methods and classes themselves, and no outside information whatsoever. Please review this to make sure nothing is inaccurate or anything.
Look at the number of files changed