-
Notifications
You must be signed in to change notification settings - Fork 0
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
Adds fault monitors #199
Comments
@jkleiber Should I make the custom condition monitor class use generics to accept any type, or can we reasonably assume that all values dealt with will be doubles? |
Let's go as generic as possible - there's always a chance we'll want to monitor integers/strings/booleans for example |
Awesome. Would it be preferable to have a callback when the fault is triggered or do we want to only poll the monitors and act upon a fault later? I'm thinking the callback route could look something like the following: MonitorManager.addMonitor(
new CustomMonitor(
"armEncoderUnplugged", // Name to log callback under
true, // Sticky fault (remain faulted even after conditions become acceptable again
() -> !(DriverStation.isEnabled()
&& (aimerInputs.aimAppliedVolts > ScoringConstants.aimerkS * 2
&& aimerInputs.aimVelocityRadPerSec
< ScoringConstants.aimerMovementThresholdRadPerSec)), // isStateValid function to check whether the state is currently valid or not
ScoringConstants.maxAimUnresponsiveTimeSeconds, // timeToFault (how long can state be invalid before a fault occurs)
() -> { // faultCallback, the function run every tick when the monitor has detected a fault.
ScoringSubsystem.setOverrideMode(true);
ScoringSubsystem.setOverrideVolts(0.0);
})); I'm not sure entirely how I'd incorporate MonitorManager into the robot, but basically, it would just keep track of all of the monitors and run their periodic functions every tick. |
@aidnem yeah this looks good! If you want to avoid the complications of managing the singleton MonitorManager (mapping all the variables/suppliers/consumers might be a mess in RobotContainer), you could make a MonitoredSubsystem that inherits from Subsystem and then turn the ScoringSubsystem into a MonitoredSubsystem. The periodic method of MonitoredSubsystem would then run all registered monitors at the start, and then call a method that can be overridden by the user. Then the scope of all these variables would be contained in the subsystem they are relevant to |
@jkleiber Good idea. I had no idea how I would make the MonitorManager singleton yet so this will probably be a better solution. I'll look into doing that when I have time. |
@jkleiber just one question: wouldn't we want the monitors to be checked before the periodic function runs but then acted upon at the end of periodic? This way their behavior would override any behavior from the periodic function of the subsystem itself. |
Yeah you could also just check and act at the end |
Yeah, but I wanted the monitors to be able to view the IO and the state of the physical world in the same timeframe. If we check at the end, it'll be the next frame's IO but the state of the robot after being controlled by the previous frame's IO. At a glance, this seems like it would cause false positives. I'll just check and act at the end, and if this becomes an issue later we can swap to check at the start, act at the end. |
We could also make this subsystem do things in a defined order: CopperheadSubsystem execution order:
Then we could also register IO classes as a list and define them with a common interface such that they get update inputs/outputs called automatically |
If you don't mind, I'd rather push that to a future issue to prevent this one from getting overwhelming. If you want, we could replace this issue with a CopperheadSubsystem feature instead. I have one other question/note: The multiple different types of monitors listed above seem unnecessary. Wouldn't it be simpler to make the base monitor class accept a function, and then define all monitors using suppliers? If you have reasoning for having multiple different monitor classes I'll gladly do it, I was just wondering about the thought process behind it. |
Implementing the monitors via user defined functions/lambdas is totally valid. We can go with that, I mainly had this idea as someone who works in c++ a lot where that design pattern isn't common. I think the copperhead subsystem could be follow on work, but getting the initial set up might be very useful for this issue |
Basic Monitor and MonitoredSubsystem implementations are committed and pushed. I didn't create a PR yet since it's so early but I can do that soon if you like. |
@jkleiber we now have an arm on high key as well. Want to move this over to coppercore so we can (hopefully) not destroy that one as well? |
@aidnem yes let's move it to coppercore. We can consider the elastic feature as well, but at first I just want a simple "if violation for persistence then take an action" general monitor architecture |
@aidnem if you create a ticket I can drop some ideas in there and we can discuss at shop |
Yeah, that's fair. We could also extend the Monitor class in future to add a MonitorWithNotification class or something similar. I'll create a coppercore issue for that to be included under our wpi interface. |
Purpose
The purpose of this addition is to add an easily expansible system of monitors to detect various faults on the robot.
One use of this is to create performance monitors for detecting performance degradation.
In autonomous systems, a "monitor" is something that keeps track of variables of interest to ensure they remain within acceptable ranges. If the variable goes outside the acceptable range for a persistence, the monitor goes into alarm and typically takes an action.
For example, let's say you have a battery system that could experience a thermal runaway. To mitigate this risk and protect the system, a monitor could be used to shut off power if the system exceeds a certain temperature. The following monitor could be appropriate for this application:
In this addition we should add some basic monitors that we can use on our robot, such as:
Project Scope
The text was updated successfully, but these errors were encountered: