Advantagekit implementation v2 #27

arghunter · 2023-10-17T00:16:22Z

Implementation of advantagekit with minor code changes Logs most of what we need.

adias408

Looks like it's headed in the right direction. I think there are some cleanups we can make to avoid creating unnecessary garbage and we also ought to define a naming convention, particularly since this will be used as a template for next year

adias408 · 2023-10-17T05:19:43Z

2023-Robot/build.gradle

@@ -43,7 +44,7 @@ def deployArtifact = deploy.targets.roborio.artifacts.frcJava
 wpi.java.debugJni = false

 // Set this to true to enable desktop support.
-def includeDesktopSupport = false
+def includeDesktopSupport = true


just curious: why this change?

It was changed to enable robot simulation on desktop. Doesn't really impact the robot, however, our code isn't formatted for simulation yet. I'll change to back to false .

2023-Robot/src/main/java/frc/team670/robot/subsystems/Claw.java

adias408 · 2023-10-17T05:40:43Z

2023-Robot/src/main/java/frc/team670/robot/commands/pathplanner/Auton.java

@@ -15,20 +16,20 @@
 import frc.team670.robot.commands.arm.MoveToTarget;
 import frc.team670.robot.commands.claw.ClawEject;
 import frc.team670.robot.commands.claw.ClawInstantIntake;
+import frc.team670.robot.commands.cubeintake.CubeIntakeEject;
+import frc.team670.robot.commands.cubeintake.CubeIntakeIntake;
+import frc.team670.robot.commands.cubeintake.CubeIntakeToggle;


this looks like just some reordering of imports. Which doesn't hurt anything but is noise in the context of this PR. Let's get in the habit of not doing unrelated things

adias408 · 2023-10-17T05:42:02Z

2023-Robot/src/main/java/frc/team670/robot/subsystems/Claw.java

@@ -26,8 +29,8 @@ public enum GamePiece{
    private Claw.Status status;
    private Claw.GamePiece gamepiece=Claw.GamePiece.CONE;

-    private final String currentKey = "Claw motor current";
-    private final String clawStateKey = "Claw state";
+    private final String currentKey = "Claw/motor current";


Yes! :)

However: isn't the java naming convention such that this should be CURRENT_KEY?

Yes. Since we switched to Hungarian notation would be be kCurrentKey or CURRENT_KEY

About that: we haven't really switch to Hungarian notation. We currently have a mess where some things are hungarian-ish and a lot of things aren't. The only PRs I've seen using Hungarian have been yours. This isn't something that we should be doing without a conversation and general agreement. I'll tell you that in the wider world, Hungarian had a moment a couple of decades ago but has mostly gone out of style. In particular with IDEs, ask yourself what it's adding.

I was under the impression the prior tech leads switched most parts of our codebase to Hungarian notation. I prefer the default java naming convention. What do you recommend we do?

Fair enough - I'm not sure when it started creeping in. I've just been noticing more of it. I still think most of our code uses normal java naming conventions. I'd personally prefer to stick with that but the more important thing is that we agree and then new code conforms to whatever that agreement is. Most organizations have a style guide. Or rather: most organizations refer to an existing style guide and maybe have a few local variations that they define. This is the one we had some years back: https://docs.google.com/document/d/1rrgTm2BNigiYZ8PDRLJEujGYzFkO1Sl5vrtekVEq2RY/edit#heading=h.u6ednqa8ivih (found a link here: https://github.com/HHS-Team670/2019-Robot/wiki/GitHub-Code-Review-Process). Here's another one: https://google.github.io/styleguide/javaguide.html

But really the most important thing is let's agree and then let's enforce it through code reviews so that things don't just start changing because someone starts doing things and nobody pays attention.

adias408 · 2023-10-17T05:42:23Z

2023-Robot/src/main/java/frc/team670/robot/subsystems/Claw.java

@@ -200,8 +203,8 @@ public void mustangPeriodic() {

    @Override
    public void debugSubsystem() {
-        SmartDashboard.putNumber(currentKey, motor.getOutputCurrent());
-        SmartDashboard.putString(clawStateKey, status.toString());
+        Logger.getInstance().recordOutput(currentKey, motor.getOutputCurrent());


don't call getInstance() more than you need to

Have you seen the 2024 version of the library?

I managed to find the new version of AdvantageScope but not the beta version of AdvantageKit, I'll take a look soon.

I was just mentioning it because all this getInstance() stuff will go away

Ok, then i'll leave it like this for now and then change it when we update to 2024 advantagekit.

adias408 · 2023-10-17T05:46:11Z

2023-Robot/src/main/java/frc/team670/robot/subsystems/arm/Arm.java

+        double transformedShoulderAngle=getShoulder().getCurrentAngleInDegrees()-90;
+        double transformedElbowAngle= -(transformedShoulderAngle-getElbow().getCurrentAngleInDegrees()+90);
+        double transformedWristAngle= -transformedElbowAngle+getWrist().getCurrentAngleInDegrees();
+        MechanismLigament2d shoulderLig = m2dr.append(new MechanismLigament2d("Shoulder", 0.66,transformedShoulderAngle));


we should get a sense for how expensive this is. unclear whether it's worth it

Good idea, I'll move it into the debug subsystem method with a separate boolean for if this should be logged so that it is toggleable.

adias408 · 2023-10-17T05:51:12Z

2023-Robot/src/main/java/frc/team670/robot/subsystems/arm/Elbow.java

-    private final String current = "Elbow current";
+    private final String positionDeg = "Elbow/position (deg)";
+    private final String absEncoderPos = "Elbow/abs encoder position";
+    private final String positionRot = "Elbow/position (rotations)";


more of the same boilerplate comments about constant naming, calling a function repeatedly, etc.

Also: we should spend some time on naming conventions as we're likely to have a bunch of the same kinds of keys and it gets to be very annoying if they're named slightly differently. For example: above, we have "position (deg)" and then we have "abs encoder position" - two positions, one of which specifies a unit and one of which doesn't.

Usually people avoid spaces in these kinds of keys. It makes it easier to know how to read them. And they generally wouldn't use parentheses either. You generally either use snake_case or camelCase. And you always include units where appropriate and if you have units then you make sure you name them the same so it's not deg sometimes and degs other times and degrees still somewhere else. So one possibility could be something like:
motor_position_degrees
abs_encoder_rotations
motor_position_rotations

or something

will establish a naing convention over the weekend

I never saw any followup on this. It's something the team as a whole will have to use. I'd expect to see a written proposal and then some discussion to settle on something we'll use.

adias408 · 2023-10-17T05:52:11Z

2023-Robot/src/main/java/frc/team670/robot/subsystems/arm/Shoulder.java

@@ -92,9 +95,9 @@ public HealthState checkHealth() {
        boolean followerOK = (followerRotatorError == REVLibError.kOk);

        if (!leaderOK && !followerOK) {
-            Logger.consoleLog("Shoulder error! Leader error is " + leaderRotatorError.toString());


why do we want to get rid of this?

also: why are we not recording the errors to the new spiffy logger?

adias408 · 2023-10-17T05:53:18Z

2023-Robot/src/main/java/frc/team670/robot/subsystems/arm/VoltageCalculator.java

@@ -44,7 +45,7 @@ public VoltageCalculator(ArmSegment... armSegments) {
    public ArrayList<Double> calculateVoltages(double... anglesRelativeToPrevious) {
        // First, ensure that the number of received angles is correct
        if(anglesRelativeToPrevious.length != armSegments.length) {
-            Logger.consoleError("VoltageCalculator received the wrong number of angles! Received " + anglesRelativeToPrevious.length + " but expected " + armSegments.length);
+            // Logger.consoleError("VoltageCalculator received the wrong number of angles! Received " + anglesRelativeToPrevious.length + " but expected " + armSegments.length);


this seems like a catastrophic logic error. why are we silencing it?

adias408 · 2023-10-17T05:55:45Z

2023-Robot/src/main/java/frc/team670/robot/subsystems/arm/VoltageCalculator.java

@@ -70,7 +71,7 @@ public ArrayList<Double> calculateVoltages(double... anglesRelativeToPrevious) {

        }

-        SmartDashboard.putString(voltageString, voltages.toString());
+        Logger.getInstance().recordOutput("Arm/"+voltageString, voltages.toString());


would it be more useful if we recorded each of the voltages independently? (no idea, just asking)

arghunter · 2023-10-24T03:42:53Z

Note to self: add swerve module logging

arghunter · 2023-10-24T03:44:14Z

Also, this branch was well tested today at drive practice and seems to work pretty well.

…HS-Team670/2023-Robot into advantagekit-implementation-v2

arghunter added 2 commits October 12, 2023 08:27

advantagekit works

8a66cd9

minor advantagekit changes

f4c204b

arghunter requested review from KattuparambilAditi and adias408 October 17, 2023 00:16

adias408 requested changes Oct 17, 2023

View reviewed changes

save work

e00e3e1

arghunter and others added 14 commits October 28, 2023 11:56

semantic and naming updates also tep disabled leds

fd53832

minor arm fix

7d081fd

k

752141f

renabled console logging

91495ef

Partially fixes fro logging

d82d0f1

Updated string constants in Shoulder

361ac39

Added _KEY onto incomplete string vars

cbfa329

small fixes

769d9c0

Merge branch 'advantagekit-implementation-v2' of https://github.com/H…

53a689b

…HS-Team670/2023-Robot into advantagekit-implementation-v2

Updated debug strings in SwerveDrive.java

5e3f1cc

Updated debug string in VoltageCalculator.java

db57338

Update CubeIntake.java

b7b9004

Updated Shoulder

f5be3ae

minor fix

b5567d8

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Advantagekit implementation v2 #27

Advantagekit implementation v2 #27

arghunter commented Oct 17, 2023

adias408 left a comment

adias408 Oct 17, 2023

arghunter Oct 24, 2023

adias408 Oct 17, 2023

adias408 Oct 17, 2023

arghunter Oct 26, 2023

adias408 Oct 29, 2023

arghunter Oct 29, 2023

adias408 Oct 29, 2023

adias408 Oct 17, 2023

adias408 Oct 24, 2023

arghunter Oct 29, 2023

adias408 Oct 29, 2023

arghunter Nov 1, 2023

adias408 Oct 17, 2023

arghunter Nov 1, 2023

adias408 Oct 17, 2023

arghunter Oct 20, 2023

adias408 Oct 29, 2023

adias408 Oct 17, 2023

adias408 Oct 17, 2023

adias408 Oct 17, 2023

adias408 Oct 17, 2023

arghunter commented Oct 24, 2023

arghunter commented Oct 24, 2023

Advantagekit implementation v2 #27

Are you sure you want to change the base?

Advantagekit implementation v2 #27

Conversation

arghunter commented Oct 17, 2023

adias408 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arghunter commented Oct 24, 2023

arghunter commented Oct 24, 2023