Skip to content

Fault/Contactor Logic #8

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

Open
wants to merge 16 commits into
base: Code
Choose a base branch
from
Open

Fault/Contactor Logic #8

wants to merge 16 commits into from

Conversation

Rav4s
Copy link

@Rav4s Rav4s commented May 1, 2025

motor logic tested and fully functional 4/29/2025, need to test array

Copy link
Author

@Rav4s Rav4s left a comment

Choose a reason for hiding this comment

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

.

void contactorTask(void *pvParamters) {

for (int i = 0; i < 10; i++){ //initialize system to be faultless
fault[i] = NO_FAULT;
Copy link
Author

Choose a reason for hiding this comment

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

just curious, why do this here rather than when fault is defined

@Rav4s Rav4s requested review from Tony5427, Lakshay983 and JacobG111 May 3, 2025 05:48
@Rav4s Rav4s requested a review from ParthivS20 May 6, 2025 00:06
Copy link

@ParthivS20 ParthivS20 left a comment

Choose a reason for hiding this comment

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

checked over it, looks good. Great Job! keep it up :)

Copy link

@Lakshay983 Lakshay983 left a comment

Choose a reason for hiding this comment

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

All of the if statements are really hard to read.

I think you can simplify a good chunk of the logic by using FreeRTOS timers, and in your timer callback functions you put the fault state.

https://www.freertos.org/Documentation/02-Kernel/02-Kernel-features/05-Software-timers/01-Software-timers

#include "contactor.h"
#include "statusLEDs.h"

// structs to manage contactor states

Choose a reason for hiding this comment

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

imo contactor.c should be a set of drivers for interacting with the Contactors, not smth containing all your logic. The logic should be in the task.

Look at how other code bases do Contactors, you should have a contactor enum for each contactor.
Since you have a struct for it you can make it an array that you index into via the enum.

Then have functions for enabling and disabling the Contactors given a contactor enum

Comment on lines 56 to 62
typedef struct prechargeContactor {
contactor_state_t state; // open, closed
contactor_state_t pre_ready; // Precharge ready signal
contactor_state_t sense;
contactor_state_t enable_out; // GPIO pin to control the contactor
uint32_t start_time;
} prechargeContactor;

Choose a reason for hiding this comment

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

why diff type?

Can just have a boolean for precharge vs not precharge


// trigger LEDs for all faults (and send fault message?)
bool contactors_fault_handler(void) {
if ((contactor_fault[0] | contactor_fault[1] | contactor_fault[2] | contactor_fault[3] | contactor_fault[4] | contactor_fault[5] | contactor_fault[6] | contactor_fault[7] | contactor_fault[8] | contactor_fault[9] | CAN_fault) == FAULT) {

Choose a reason for hiding this comment

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

do for loop with NUM_FAULTS

prechargeContactor arrayPre = { OPEN, OPEN, OPEN, OPEN, 0 };

uint32_t time; // HAL tick value (ms)
uint8_t contactor_fault[10] = { 0 }; // TODO: handle this better, change to fault bitmap

Choose a reason for hiding this comment

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

yeah you should make bitmap

if you keep it in an array just make the faults represented by enum

// handles all contactor and precharge logic
void contactors_handler(void) {
while (true) {
time = HAL_GetTick();

Choose a reason for hiding this comment

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

Given that you're in a task you could just use a FreeRTOS timer with a callback function that is called when there's a precharge timeout. That'll save u some extra logic.

arrayPre.pre_ready = array_precharge_ready();
arrayPre.sense = array_precharge_sense();

if (!contactors_fault_handler()) {

Choose a reason for hiding this comment

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

consider making this a while(1) loop and then when a fault is detected manually enter the fault instead of checking for every fault every iteration.

@Rav4s Rav4s mentioned this pull request May 7, 2025
11 tasks
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.

4 participants