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

Add class to calculate robot resistance #4162

Closed
wants to merge 51 commits into from

Conversation

ysthakur
Copy link
Contributor

@ysthakur ysthakur commented Apr 11, 2022

This PR adds a ResistanceCalculator class to calculate resistance by getting total current and voltage periodically and doing linear regression on those points. There's only a Java class right now, I'd appreciate help making a C++ one. There's also a C++ one now, but I'd appreciate someone looking over it to make sure it's idiomatic and functional.

@calcmogul
Copy link
Member

calcmogul commented Apr 11, 2022

What's the use case for this? The docs should mention that.

If it's for wire disconnect detection, should this be built into the PDP class for convenience? You could have a sensible default threshold and print DS warnings if it's exceeded.

@ysthakur
Copy link
Contributor Author

ysthakur commented Apr 11, 2022

@calcmogul Our team's had this for a while, but I'm not actually sure why it was originally made. We were having brownouts last month and an alum mentioned that maybe it was resistance, so this could maybe help people needing to debug stuff like that.
I considered putting this into the PowerDistribution class itself, but the current and voltage need to be continually updated and I'm not sure how one would do that without making PowerDistribution a subsystem with an overriden periodic method. Any idea on how to do that?

@calcmogul
Copy link
Member

calcmogul commented Apr 11, 2022

A polling method could be added to PowerDistribution that gets called by either the DriverStation thread or a new PowerDistribution Notifier at 20 ms intervals. We've been wanting to remove unrelated stuff like MotorSafety from the DriverStation thread, so I'd prefer a PowerDistribution Notifier.

@calcmogul
Copy link
Member

@Oblarg could you provide context on this class since you mentioned it on the FRC Discord?

@Oblarg
Copy link
Contributor

Oblarg commented Apr 11, 2022

Yeah; the class internally performs a running linear regression of voltage vs current; the update function takes a new observation pair and then the object can be polled to get the resistance.

The API shape should probably be changed substantially to match WPILib conventions; instead of update we should have a calculate function that simply returns the computed resistance.

I like the idea of a powerdistribution loop to handle doing this calculation for the channels that the user is tracking; whether that's a notifier or something that hooks into the main event loop should be discussed.

@calcmogul
Copy link
Member

calcmogul commented Apr 11, 2022

Synchronous or asynchronous depends on whether the user code can do anything sensible in response to an event. If not, I think just logging it asynchronously is fine.

@Oblarg
Copy link
Contributor

Oblarg commented Apr 11, 2022

Fair point. I think users can react sensibly to learning that the resistance has gone to infinity (which this class should detect properly), at the very least.

@calcmogul
Copy link
Member

calcmogul commented Apr 11, 2022

What should their code do in response?

@Oblarg
Copy link
Contributor

Oblarg commented Apr 11, 2022

Possibly, display a fault to the driver and lock the operation of other devices that cannot be safely actuated in that fault state?

@calcmogul
Copy link
Member

calcmogul commented Apr 11, 2022

What about the Notifier detecting the events and logging to console, but also pushing the events to NT(4) so users can set up an NTEventListener to do something about it? A message queue would be better than an NTEventListener so the user's robot code stays synchronous with itself, but we don't have that yet.

@ysthakur
Copy link
Contributor Author

@Oblarg A calculate method makes more sense, but now that I'm moving the resistance stuff into PowerDistribution itself, there are two options for updating the resistance:

  • Having a calculate method and a cached field that gets used in PowerDistribution#getResistance. A cached field is slightly more annoying to put in.
  • Having separate update and getResistance methods. This makes it slightly easier to implement. It's not idiomatic but maybe the calculator class could be private or package-private anyway.

@ysthakur
Copy link
Contributor Author

@calcmogul Would it be better to let the user configure the Notifier period (with a default of 0.02 s) or just have it set to 0.02 for everyone?

@calcmogul
Copy link
Member

calcmogul commented Apr 11, 2022

  • Having a calculate method and a cached field that gets used in PowerDistribution#getResistance. A cached field is slightly more annoying to put in.
  • Having separate update and getResistance methods. This makes it slightly easier to implement. It's not idiomatic but maybe the calculator class could be private or package-private anyway.

You could have a for loop over an array of filters whose calculate() return values go into an array of cache variables.

Would it be better to let the user configure the Notifier period (with a default of 0.02 s) or just have it set to 0.02 for everyone?

Depends on the refresh rate of the voltage and current data from the PDP. If it's slow, 20 ms is probably fine. Even if it's not, I'm not sure if faster sampling will help.

@sciencewhiz
Copy link
Contributor

Depends on the refresh rate of the voltage and current data from the PDP. If it's slow, 20 ms is probably fine. Even if it's not, I'm not sure if faster sampling will help.

I believe it's sampled at 25ms intervals.

@calcmogul
Copy link
Member

calcmogul commented Apr 11, 2022

For reference, from the FRC Discord:

for (int i = 0; i < numChannels; ++i) {
  double current = currentFilters[i].calculate(getCurrent(i));
  if (current == 0) {
    m_resistances[i] = Double.POSITIVE_INFINITY;
  } else {
    m_resistances[i] = voltageFilters[i].calculate(getVoltage(i)) / current;
  }
}

Where m_resistances is an array of atomic doubles and voltageFilters and currentFilters are arrays of filters. They're all dynamically sized to the number of channels. I didn't look at what the exact voltage and current getter APIs are.

@Oblarg
Copy link
Contributor

Oblarg commented Apr 11, 2022

For reference, from the FRC Discord:

for (int i = 0; i < numChannels; ++i) {
  double current = currentFilters[i].calculate(getCurrent(i));
  if (current == 0) {
    m_resistances[i] = Double.POSITIVE_INFINITY;
  } else {
    m_resistances[i] = voltageFilters[i].calculate(getVoltage(i)) / current;
  }
}

Where m_resistances is an array of atomic doubles and voltageFilters and currentFilters are arrays of filters. They're all dynamically sized to the number of channels. I didn't look at what the exact voltage and current getter APIs are.

should make sure the voltage is nonzero before reporting double.positive_infinity. How do we want to handle error states? It feels like there are a few (nonsense data due to insufficient change in applied voltage, nonsense data due to no applied voltage, poor fit due to large amounts of signal noise...)

@ysthakur
Copy link
Contributor Author

ysthakur commented Apr 11, 2022

@Oblarg It kinda already makes sure voltage is nonzero before reporting infinity. If both voltage and current are 0, it returns NaN. If only voltage is 0, it returns 0. If only current is 0, it returns positive infinity. Perhaps it could simply return those values without doing anything extra there but print a warning to the console when it gets nonsense data. If there's poor fit due to large amounts of signal noise, the r^2 threshold should just keep the resistance NaN. Do we want to detect whether or not r^2 stays below that threshold for a prolonged period of time?

@Oblarg
Copy link
Contributor

Oblarg commented Apr 11, 2022

We should probably not push data points in which the current is zero, so that we don't push informative data points out with uninformative ones once we stop drawing load.

@sciencewhiz
Copy link
Contributor

Are you going to get useful data on a per channel basis? Voltage is the overall battery voltage, so I expect any per channel readings to be swamped by noise from the drivetrain and not be useful. I'd expect only total resistance to be meaningful in normal operation.

@ysthakur
Copy link
Contributor Author

@sciencewhiz My team will try looking at that at our next meeting. If it isn't useful, it'd certainly make it much easier to implement, although it'd be less great when you're actually using it.

@Oblarg
Copy link
Contributor

Oblarg commented Apr 12, 2022

Per-channel calculations definitely work, but only with independent measurements of the voltage at the motor controller. We might need to expose an API handle for the user to provide those

@calcmogul
Copy link
Member

calcmogul commented Apr 12, 2022

Every parallel branch of a circuit has the same voltage. The currents on the parallel branches sum to the output current of the battery.

@ysthakur
Copy link
Contributor Author

ysthakur commented May 7, 2022

So the resistance example has to use some sort of dummy function to supply the voltage, because it can't use a CANSparkMax or something to get the voltage from the bus.

@Starlight220
Copy link
Member

MotorController.get() * RobotController.getVoltage() should work, or see #3992

@AustinShalit
Copy link
Member

@ysthakur Do you have example data from a real robot using this?

@ysthakur
Copy link
Contributor Author

ysthakur commented May 9, 2022

@AustinShalit Unfortunately, not yet. I'll ask my team to try this out the next time they meet.

@Starlight220
Copy link
Member

If there's a minimal code sample to test this, I might be able to run it on hardware.

@ysthakur
Copy link
Contributor Author

ysthakur commented Jun 14, 2022

@Starlight220 That'd be great, especially since it looks like my team won't be able to test it themselves. There's an example project here to get the resistance of a single channel. Let me modify that to also log the robot resistance.

@calcmogul
Copy link
Member

Superseded by #5500 because the original branch doesn't allow maintainers to push to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants