Brownout2 #18

eggaskin · 2022-01-25T01:55:20Z

If this works for everyone, we will use this as a base for our subsystem power management/brownout. Please review.

* Create README.md (#11) (#14) Co-authored-by: dx44383 <[email protected]> * Upgrade to WPILib 2022.1.1. (#13) Co-authored-by: Ethan <[email protected]> Co-authored-by: dx44383 <[email protected]>

proan · 2022-01-25T04:08:21Z

src/main/java/frc/robot/PowerController.java

+
+    private ControllableSubsystem tankSubsystem;
+
+//    private ControllableSubsystem climbSubsystem;


Alignment

Also, is there a reason you have an empty line between each subsystem? I would think they'd make a nice block of 4 or so line.

proan · 2022-01-25T04:12:50Z

src/main/java/frc/robot/PowerController.java

+
+   // private ControllableSubsystem shooterSubsystem;
+
+    private int currentCurrentLimit;


This is a personal style recommendation: when I name variables that correspond to physical things I like to append the units to the variable name. In this case it would be currentCurrentLimit_A. This way I can check units by looking at variable names.

proan

I'd like to see the current limits be constants at a minimum.

proan · 2022-01-25T04:13:45Z

src/main/java/frc/robot/PowerController.java

+
+        this.tankSubsystem = tankSubsystem;
+
+        currentCurrentLimit = 350;


Could these be constants, or will they change during a match?

Are you sure you want to do integer arithmetic with these?

proan · 2022-01-25T04:18:12Z

src/main/java/frc/robot/PowerController.java

+
+        System.out.println("Checking for a brownout...");
+        // Check PDP voltage; if close to a brownout:
+        if (PDP.getVoltage() < 7) {


Does PDP.getVoltage() return an integer or a floating point number? You should make your comparison value match.

proan · 2022-01-25T04:20:19Z

src/main/java/frc/robot/PowerController.java

+        int channels = 0;
+
+        for (int channel : PDPChannel) {
+            sum += PDP.getCurrent(channel);


Does PDP.getCurrent() return an integer or a floating point number?

proan · 2022-01-25T04:22:20Z

src/main/java/frc/robot/PowerController.java

+        //scale all subsystems by set (sensible) amount
+        for (ControllableSubsystem subsystem : subsystems) {
+            int currDrawn = subsystem.getTotalCurrentDrawn();
+            int tempscale = currDrawn - (int)(currDrawn*0.2);


I'm concerned about all the integer arithmetic. Do we know what the expected range of values are here?

It's hard, because PDP.getCurrent() returns double, but CANSparkMax expects an int for setSmartCurrentLimit(). I can get rid of some of the intermediary int casts but it will still have to be cast to int in the end for scaling.

Yes, let's do that. Since you measure in double let's use that for all the math and then cast (or better yet, design a rounding function) to an int for the current limit. CS math isn't quite the same as pencil-and-paper math, so it's best to stick with double for the math (that only has issues with equals, not multiplication and division).

src/main/java/frc/robot/subsystems/tank/TankSubsystem.java

proan · 2022-01-25T04:44:44Z

src/main/java/frc/robot/subsystems/tank/TankSubsystem.java

+  public void setCurrentLimit(int limit) {
+    this.currentLimit = limit;
+
+    int motorLimit = currentLimit/4; //number of motors


Be careful with integer division. Do you know what happens for currentLimit not being evenly divisible by 4?

ky28059 · 2022-01-25T07:02:13Z

Instead of having a checkPriority(ControllableSubsystem) method which returns an int from 1-n and then having to iterate to find the lowest priority, can we instead have a Queue<ControllableSubsystem> in order from low to high priority and have scale() keep popping from that? The only reason I can think of for this maybe being a little complex is converting the array in the constructor to a Queue; maybe we can assume the subsystems are passed in in order of priority?

ky28059 · 2022-01-25T07:09:39Z

Although I suppose returning an int priority is better for dynamically updating priority based on conditions like whether the turret is ready to shoot or whether its climb time; in that case though, I think maybe a double priority would allow for better tuning?

ky28059 · 2022-01-25T18:28:28Z

src/main/java/frc/robot/PowerController.java

+    for (ControllableSubsystem subsystem : subsystems) {
+      double currDrawn = subsystem.getTotalCurrentDrawn();
+      int tempscale = (int) (currDrawn * 0.8);
+      subsystem.setCurrentLimit(tempscale);


Why does brownout scaling set everything to a flat 0.8 scale while scale() sets everything to its minimums? Are there cases where the minimum value is above 0.8 * currDrawn and the brownout scaling is less harsh than normal scaling?

Maybe that 80% should be a default value?
BTW this is a good design question to answer.

ky28059 · 2022-01-25T18:30:13Z

src/main/java/frc/robot/PowerController.java

+    int lowPriority = 10; // This int should be higher than the number of subsystems
+
+    // If we've already scaled many subsystems, just scale more dramatically for everything
+    if (checked.size() > 3) {


Why does scale() give up after 3 subsystems? Also, if brownout scaling is sometimes more forgiving than minCurrent, would this result in the lowest priority subsystems getting more power than if they had been scaled normally?

ky28059 · 2022-01-25T18:31:00Z

src/main/java/frc/robot/PowerController.java

+    }
+
+    double currDrawn = lowestPriority.getTotalCurrentDrawn();
+    lowestPriority.setCurrentLimit((int) lowestPriority.minCurrent());


Seems pretty harsh, perhaps doing some math and scaling things proportionally would be better?

ky28059 · 2022-01-25T18:32:03Z

src/main/java/frc/robot/subsystems/tank/TankSubsystem.java

+
+  @Override
+  public double getTotalCurrentDrawn() {
+    return PowerController.getCurrentDrawnFromPDP(fLeftMotorPort, fRightMotorPort, bLeftMotorPort, bRightMotorPort);


Perhaps this can instead be a getPDPPorts method and have PowerController call getCurrentDrawn itself.

src/main/java/frc/robot/subsystems/tank/TankSubsystem.java

proan · 2022-01-25T20:21:50Z

Regarding priorities for your mechs/modules, @ky28059 has raised some good questions:

Do priorities shift during a match (My thought is "yes" when you transition to "climb" but because that's the end phase, it doesn't significantly impact the priority system design)
How to represent priority? Is it a number, a position in a list or queue, or some other kind of custom class or datatype? (An example from Linux is the udev system, which uses integers from 00 to 99 with 99 being last and thus most important)

ky28059 · 2022-01-30T01:20:45Z

src/main/java/frc/robot/RobotContainer.java

@@ -13,14 +13,21 @@
 import edu.wpi.first.wpilibj.Filesystem;
 import edu.wpi.first.wpilibj.GenericHID;
 import edu.wpi.first.wpilibj.XboxController;
+<<<<<<< HEAD


Github made you commit merge conflicts? Have you resolved these or are they just on the branch?

By the way, you should keep HEAD for all of these. The conflicts look to be from very old changes from before pathfollowing was merged.

…into brownout2

I think Aditi bulldozed over my old commits when merging her branch with remote, so this merge readds those commits and makes it compatible with the new changes.

This allows easier shared logic between subsystems (an interface would require each subsystem to implement their own versions of every method as it does not have access to fields). The original idea was to use this for climb cleanup as well as brownout, but basically anything that needs to be shared across subsystems can exist here.

ky28059 · 2022-02-02T03:41:20Z

The idea for weighted average (and priority setting) is for subsystems to be able to somewhat scale themselves based on current situations. For example, in InternalSubsystem.java,

@Override
public void periodic() {
    double priority = shotRequested ? 8.0 : 5.0;
    powerController.setPriority(this, priority);
    powerController.setPriority(turretSubsystem, priority);
    // ...
}

With weights, it was detemined that base * dynamic would give too harsh a difference between a subsystem with base priority 4.0 and a subsystem with base priority 2.0 (here, tank would get double the allotted current as turret for just existing). However, some form of multiplication would be really useful for being able to nuke the priority of a subsystem during something like climb; climbInit() might set a subsystem's dynamic priority to 0 so that during climb, current isn't wasted by being allotted to a deactivated subsystem.

Or we could do something else for climb, idk.

ky28059 · 2022-02-02T03:55:54Z

The precise values of the base priorities can be played around with, but these values seem reasonable (comparing tank to turret when both are at resting dynamic priority and when turret is at a higher priority).

ky28059 · 2022-02-02T04:54:42Z

Alternative brownout strategy from Liang:
Make sure at all times that the sum of the current limits of all the subsystems adds up to the sustainable current. When a subsystem begins to approach its limit (the ratio of used to unused current increases), allocate it more limit by taking limit from a subsystem whose ratio is lower.

This would naturally fix the climb (and climbInit) problem as climb will not use any current before the climb phase which means its limit would be distributed to other users (a similar process holds true for subsystems during climb).

I can attempt to implement this soon.

ky28059 · 2022-02-02T06:04:59Z

Although, an issue with this approach is that in the scenario where the current draw is pushing the sustainable current there isn't really a clean way for a subsystem to say "my ratio might be the same as this other system but screw them and give me more power"

This makes the calculations a little less clean, but is needed to prevent subsystems "locking" when drawing 0 current (resulting in their limits being set to 0) as well as give each subsystem some breathing room.

ky28059

I'm self-reviewing here, but I think brownout is mergeable. The new plan is really really clean!! (and its one issue with minimum and 0 currents was just fixed). I know @proan brought up the difference between sustainable current draw vs sustainable voltage, but given that we'll be using new batteries I think we can measure the sustainable current value and trust that maintaining that will sustain the voltage at a desired level for the entire match.

changes were made

ky28059

The bugs that came up during testing were fixed so hopefully everything is working swell. The totalSustainableCurrent needs to be measured, as does limits for each subsystem, but this branch can be merged so that I can rebase shooter.

Brownout!

dx44383 and others added 4 commits November 19, 2021 16:01

Create README.md (#11)

49a24aa

Upgrade to 2022 (#15)

b87526b

* Create README.md (#11) (#14) Co-authored-by: dx44383 <[email protected]> * Upgrade to WPILib 2022.1.1. (#13) Co-authored-by: Ethan <[email protected]> Co-authored-by: dx44383 <[email protected]>

current limit implementation base logic

fdefb6f

dynamic scaling + more structure things

68b3f7b

eggaskin requested review from proan, e3l, seasew, ky28059, Ippyb and Ayeeti January 25, 2022 01:55

Ippyb previously approved these changes Jan 25, 2022

View reviewed changes

proan reviewed Jan 25, 2022

View reviewed changes

proan previously requested changes Jan 25, 2022

View reviewed changes

Code cleanup

1234d84

ky28059 dismissed Ippyb’s stale review via 1234d84 January 25, 2022 06:59

ky28059 reviewed Jan 25, 2022

View reviewed changes

proan reviewed Jan 25, 2022

View reviewed changes

src/main/java/frc/robot/subsystems/tank/TankSubsystem.java Show resolved Hide resolved

ky28059 and others added 2 commits January 25, 2022 18:37

Change PDP to PDH :)

323482a

more logic and prioritization

d7bbfdf

eggaskin force-pushed the brownout2 branch from 323482a to d7bbfdf Compare January 28, 2022 01:10

Ayeeti added 2 commits January 28, 2022 18:07

github is making me commit this to switch branches

00e05a5

added checkDynamicPriority and getDynamic Priority

c6ec132

ky28059 reviewed Jan 30, 2022

View reviewed changes

eggaskin and others added 3 commits January 31, 2022 16:48

1-10 priority system with base low current scaling code

26d4f76

syntax and PDH

757ccfe

Merge branch 'brownout2' of https://github.com/grt192/GRTCommandBased …

ab63944

…into brownout2

ky28059 added 5 commits January 31, 2022 20:36

Code recleanup

3aacef2

I think Aditi bulldozed over my old commits when merging her branch with remote, so this merge readds those commits and makes it compatible with the new changes.

Limit based brownout scaling

c2fdb30

Fix changePriority logic

69a1f01

Implement weighted average scaling

a714c9a

Use multiplication, adjust base priorities

f3dd8d0

ky28059 requested a review from proan February 2, 2022 04:05

ky28059 added 2 commits February 2, 2022 11:36

Implement new brownout plan, thread based updating

52b9956

Add minimum current logic

32fbea6

This makes the calculations a little less clean, but is needed to prevent subsystems "locking" when drawing 0 current (resulting in their limits being set to 0) as well as give each subsystem some breathing room.

ky28059 previously approved these changes Feb 3, 2022

View reviewed changes

Recursive minCurrent scaling

4ba12af

ky28059 dismissed their stale review via 4ba12af February 3, 2022 06:19

ky28059 added 3 commits February 2, 2022 22:28

Small cleanup

7593355

Remove thread-based updating in favor of periodic

e54039d

Fix testing bugs

7703df0

ky28059 approved these changes Feb 4, 2022

View reviewed changes

ky28059 merged commit caa612c into develop Feb 4, 2022

ky28059 deleted the brownout2 branch February 4, 2022 23:44

e3l pushed a commit that referenced this pull request Mar 5, 2022

Brownout2 (#18)

78fce73

Brownout!

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brownout2 #18

Brownout2 #18

eggaskin commented Jan 25, 2022

proan Jan 25, 2022

proan Jan 25, 2022

proan left a comment

proan Jan 25, 2022

proan Jan 25, 2022

proan Jan 25, 2022

proan Jan 25, 2022

ky28059 Jan 25, 2022

proan Jan 25, 2022

proan Jan 25, 2022

ky28059 commented Jan 25, 2022

ky28059 commented Jan 25, 2022

ky28059 Jan 25, 2022

proan Jan 25, 2022

ky28059 Jan 25, 2022

ky28059 Jan 25, 2022

ky28059 Jan 25, 2022

proan commented Jan 25, 2022

ky28059 Jan 30, 2022

ky28059 Jan 30, 2022

ky28059 commented Feb 2, 2022 •

edited

Loading

ky28059 commented Feb 2, 2022

ky28059 commented Feb 2, 2022

ky28059 commented Feb 2, 2022

ky28059 left a comment

ky28059 left a comment


		private ControllableSubsystem tankSubsystem;

		// private ControllableSubsystem climbSubsystem;


		// private ControllableSubsystem shooterSubsystem;

		private int currentCurrentLimit;


		this.tankSubsystem = tankSubsystem;

		currentCurrentLimit = 350;

Brownout2 #18

Brownout2 #18

Conversation

eggaskin commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

proan 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

ky28059 commented Jan 25, 2022

ky28059 commented Jan 25, 2022

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

proan commented Jan 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ky28059 commented Feb 2, 2022 • edited Loading

ky28059 commented Feb 2, 2022

ky28059 commented Feb 2, 2022

ky28059 commented Feb 2, 2022

ky28059 left a comment

Choose a reason for hiding this comment

ky28059 left a comment

Choose a reason for hiding this comment

ky28059 commented Feb 2, 2022 •

edited

Loading