Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Brownout fixes #21

Closed
wants to merge 8 commits into from
Closed

Brownout fixes #21

wants to merge 8 commits into from

Conversation

ky28059
Copy link
Member

@ky28059 ky28059 commented Feb 18, 2022

For testing and (hopefully) fixes to brownout jittering behaviors.

@ky28059
Copy link
Member Author

ky28059 commented Feb 18, 2022

I believe the initial issue with nonsensical limiting was that the sum of the minimum currents for each subsystem added up to above the sustainable current, so the ratio became negative and strange things happened. The minimum currents for each subsystem should be tested, as well as the sustainable current value. Also, Liang brought up that we should run all subsystems at once (without brownout protection) and see if we are even capable of triggering a brownout on low battery.

@e3l
Copy link
Contributor

e3l commented Feb 20, 2022

A few things from testing this on the robot:

  • there is no more stuttering. that is good.
  • we frequently go over 200A. I'm not sure how we should go about actually choosing a max sustainable current.

topic 1: the current logic uses an ideal ratio of total draw to scale all subsystems.

  • reason 1 why this is bad: it scales all subsystems equally regardless of how much they're drawing. I think the point is to scale down subsystems that aren't using their limit (low ratio) to scale up subsystems that are using their limit (high ratio), e.g. enforce equivalent exchange by using a weighted average weighted by ratios.
  • reason 2 why this is bad: it keeps scaling subsytems up if we're drawing less than total sustainable. so when we're driving slowly the limit just goes higher and higher (past our "total sustainable", which is just silly!)

topic 2: calling setCurrentLimits, getCurrentDrawn, ... every loop is quite expensive

@ky28059
Copy link
Member Author

ky28059 commented Feb 20, 2022

reason 1 why this is bad: it scales all subsystems equally regardless of how much they're drawing.

The ideal ratio logic should scale subsystems based on how much they're drawing. The ratio is determined from total draw and max sustainable current, but the actual scaling for each subsystem multiplies their draw by this ratio. Higher drawing subsystems will be given higher limits than lower drawing subsystems.

@ky28059
Copy link
Member Author

ky28059 commented Feb 20, 2022

topic 2: calling setCurrentLimits, getCurrentDrawn, ... every loop is quite expensive

Do you think a thread would make this better?

@eggaskin
Copy link
Contributor

I think using a ratio based on a subsystem's minimum current (or maybe implement a max? each subsystem is only capable of drawing so much at full throttle, so a max is feasibly, moreso for mechs than dt) and its draw might be more useful. Then like Liang said we can see if a subsystem might need more power in the first place.
We can also test old brownout, see if that makes more sense limiting-wise.
Have you tested to see if it's still limiting weirdly?

@eggaskin
Copy link
Contributor

* we frequently go over 200A. I'm not sure how we should go about actually choosing a max sustainable current.

Really, with printed amounts? In my testing max was around 80, but then I guess I wasn't using DT.

@ky28059 ky28059 mentioned this pull request Feb 25, 2022
@ky28059
Copy link
Member Author

ky28059 commented Feb 26, 2022

Hopefully we can test and merge soon. We could also try and replace recursion with Ethan's limit idea to save processing power
image
but the current algorithm for brownout isn't fundamentally broken (see images on whiteboard), just suffering from some small implementation bugs.

@eggaskin eggaskin requested review from eggaskin and e3l March 5, 2022 00:16
Copy link
Contributor

@e3l e3l left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't seem like the logic has changed, so we really just need to test this to see what aspects we need to fix i guess

@eggaskin
Copy link
Contributor

eggaskin commented Mar 8, 2022

@ky28059 if you have nothing to do during 6th you should test this code, so we can try implementing it asap.

@ky28059 ky28059 changed the base branch from develop to shuffleboard-abstraction April 6, 2022 21:17
ky28059 added 5 commits April 6, 2022 14:55
Also, remove climb from brownout calculations as it will be dormant outside of climb and during climb we don't want to be limiting it.
Hopefully this was the issue with brownout limits
@ky28059
Copy link
Member Author

ky28059 commented Apr 9, 2022

Merged into #32 .

@ky28059 ky28059 closed this Apr 9, 2022
@eggaskin eggaskin deleted the brownout-fixes branch April 11, 2022 04:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants