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

Faults base class #119

Merged
merged 18 commits into from
Dec 31, 2019
Merged

Faults base class #119

merged 18 commits into from
Dec 31, 2019

Conversation

tanishqaggarwal
Copy link
Member

@tanishqaggarwal tanishqaggarwal commented Dec 26, 2019

Adds a fault class to the flight software. This fault class has persistence, so an error needs to be signaled multiple times before the fault is set.

I'm still looking for good, non-confusing names for these fault functions. Let me know if you have any good ideas.

It'd be great if everyone can review this before the merge, since this is so core to the infrastructure

@tanishqaggarwal tanishqaggarwal changed the title Faults Faults base class Dec 26, 2019
src/FCCode/Fault.cpp Outdated Show resolved Hide resolved
src/FCCode/Fault.hpp Outdated Show resolved Hide resolved
test/test_fsw_fault/test_fsw_fault.cpp Show resolved Hide resolved
src/FCCode/Fault.cpp Show resolved Hide resolved
src/FCCode/Fault.hpp Show resolved Hide resolved
Copy link
Collaborator

@shihaocao shihaocao left a comment

Choose a reason for hiding this comment

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

I think this PR is very good, just a comment and a request.

A comment: If we're overriding or suppressing the fault, we can still accumulate num_consecutive_faults which may have unintended consequences when we turn off the suppression or override. Just wanted to make this behavior clear.

Also, I feel like there needs to be a way to command an unsiginal() from the ground. If we're not doing any kind of autonomous fault response, I think this is a necessary reset functionality. Perhaps through a writeable state field that is automatically set back to false?

@tanishqaggarwal
Copy link
Member Author

tanishqaggarwal commented Dec 27, 2019

A comment: If we're overriding or suppressing the fault, we can still accumulate num_consecutive_faults which may have unintended consequences when we turn off the suppression or override. Just wanted to make this behavior clear.

Good point. I'll make the following change: when overriding or suppressing, num_consecutive_faults is set to zero. This will prevent any unwanted autonomous response that occurs when the ground commands are turned off.

Also, I feel like there needs to be a way to command an unsiginal() from the ground. If we're not doing any kind of autonomous fault response, I think this is a necessary reset functionality. Perhaps through a writeable state field that is automatically set back to false?

Right now, signal and unsignal are an interface for autonomous behavior, and override and suppress are the interface for manual behavior. Do you see circumstances where we would want to call unsignal instead of just setting suppress?

@shihaocao
Copy link
Collaborator

Right now, signal and unsignal are an interface for autonomous behavior, and override and suppress are the interface for manual behavior. Do you see circumstances where we would want to call unsignal instead of just setting suppress?

I'm thinking of the following case:

  1. A fault flag becomes tripped.
  2. On the ground, we deduce what caused it, and also realize a set of commands to upload to prevent it from occurring in the future.
  3. We upload those commands, and merely ask the satellite to unsignal(), that way if somehow if it happens again, we can be notified.

Or slightly differently:

  1. A fault flag becomes tripped.
  2. We deduce what caused it, and realized there's nothing we can do to prevent it from being tripped again (say it'll happen every few days), but there is a set of commands to properly recover from it.
  3. We upload the recovery commands, as well as the unsignal() command so that the satellite can continue to function until the fault condition occurs again (and still protect itself from the fault).

Copy link
Collaborator

@shihaocao shihaocao left a comment

Choose a reason for hiding this comment

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

We'll go back and talk about nuanced fault response later. Details to come in an issue, and hopefully resolved in a future PR.

@tanishqaggarwal tanishqaggarwal mentioned this pull request Dec 30, 2019
@@ -60,6 +60,7 @@ lib_archive = false
lib_compat_mode = off
test_build_project_src = true
test_filter = test_fsw_*
test_ignore = test_fsw_EEPROM
Copy link
Member Author

@tanishqaggarwal tanishqaggarwal Dec 31, 2019

Choose a reason for hiding this comment

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

@fatimayousuf I thought it would be a good idea to exclude EEPROM from the desktop unit testing, since it requires a Teensy to actually test the EEPROM.

I'm curious, how does the test still happen to work on the desktop?

Copy link
Member

Choose a reason for hiding this comment

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

The test asserts in the test file are protected with #ifndef Desktop macros, so the test should work on a desktop

@@ -60,6 +60,7 @@ lib_archive = false
lib_compat_mode = off
test_build_project_src = true
test_filter = test_fsw_*
test_ignore = test_fsw_EEPROM
Copy link
Member

Choose a reason for hiding this comment

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

The test asserts in the test file are protected with #ifndef Desktop macros, so the test should work on a desktop

@tanishqaggarwal tanishqaggarwal merged commit 7693b18 into master Dec 31, 2019
@tanishqaggarwal tanishqaggarwal deleted the faults branch December 31, 2019 23:45
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.

None yet

6 participants