Skip to content

Conversation

@krazykrayon
Copy link

Quality Assurance Checklist

To make reviews more efficient, please make sure the software feature meets the following standards and check everything off that meets the quality check. Once everything has been checked, the assigned reviewers will begin the review process. Edit this description to check off the list.

There are exceptions with all guidelines. As long as your decisions are justified, then you are good! Contact the reviewers or the leads about any exceptions.

Requirements

  • [maybe] Followed Coding Style Guide
  • [yes] Presented/discussed in some capacity with others on the Controls team
  • [yes] Code Build checks pass
  • [yes] No merge conflicts
  • [not yet] Software feature has documentation for it updated in both ReadTheDocs and the comments
  • [no yet] Software feature has documentation for it updated
  • Testing
    • [not yet] Software feature has test associated with it
    • [not yet] Test provides useful information and uses relevant data to accurately represent Controls
    • [not yet] Tested on hardware
    • NOTE: If test file already exists, use that one
  • [yes] If applicable, have you added the new feature to main.c?
  • [i think so?] Tagged appropriate issue ticket on this PR (closes New Pinout #433 )
  • [uh.... yes given i didnt break anything] Did you have fun?

Things to Consider

  • [i think so?] Even if the above are checked, is this the best way of writing my code?
    • It's possible to write code that works, but are there ways to make code more efficient?

Copy link
Contributor

@aidenmadaffri aidenmadaffri left a comment

Choose a reason for hiding this comment

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

Overall this is a great pull request. There are several other files that changes would be nice too. The new daybreak_pins.h definitions make the codebase much easier to read so they should be used everywhere possible. Other files that should be changed to take advantage of this are BSP_ADC.c and BSP_UART.c:96.

@aidenmadaffri
Copy link
Contributor

This probably shouldn't be merged into master? We should have a branch for the new board.

@Lakshay983 Lakshay983 changed the title Feature/new pinout Daybreak 2025 Integration May 24, 2025
OscarPortillo37 and others added 2 commits June 6, 2025 23:52
* Completed MVP Refactor. Still need to write tests & do the while process for MVP + Cruise. Maintains old coding style from production/M2 (at least for now) in order to not break anything.

* Updated MVP FSM. Working on MVP+Cruise FSM

* Good working draft of SendTritium MVP & MVP+Cruise

* Wrote preliminary MVP test + part of MVP+Cruise test. Need to check some possible concurrency issues & might have to instead adapt previous approach based on feedback. Once done with core test, will fix some dependencies, the makefile, & conduct some testing

* Wrote out most of the functionality. Need to decide on some gear fault/state items on the display and accel+brake press protection. Will edit CMake & begin testing soon.

* Added some basic error handling (both SendTritium files & Display file), eliminated record velocity, started adding some edits to the task file for error handling & RTOS threads. Will use this as a save point, as I'll now merge all of these SendTritium files (MVP+Cruise will become the new SendTritium & rest will be deleted).

* Merged SendTritium files...I think. Need to do some testing.

* Yay, it builds! Will test now.

* Further edits post reviews. Thanks guys:)

* Fixed a few comments

* Small miscellaneous changes

* Addressed reviews and fixed tests

* Fixing build errors

* Misc changes

* Fixed brake to be analog, added more comments, fixed cruiseSet logic per review

* Small change to the SendTritium test

* Fixed silly mistake in test

* Fixed a few things on the test & changed SendTritium.h & .c accordingly

* Initial MVP Commit

* Made test & builds

* Need to make test & working on semaphore:0

* Added brake pedals range, global bps & motor status micrium flags, & mandatory driver reset to neutral on start up. Still need to fix test

* Changed Dash driver, added forced gear reset, more edits to the global bps & motor safety flags

* Added prefixes to dash stuff to ensure no name conflicts/confusions with Minions (which will phase into being deprecated)

* Minor build error edits

* More minor build error edits

* Edited display stuff to accomodate SendTritium error (gear fault)

* Moved all the flag initialization stuff to Tasks.h/.c & cleaned up ReadCarCAN includes

* Reverting unnecessary edit to BSP/STM32F413/Makefile

* Changed the last traces of park to neutral, oopsie daisy

* Minor edits

* python test script

* More changes

* Basically done merging feature/NewPinout, I think

* build fix

* Commented out flag stuff where not needed anymore either b/c it's now in throwTaskError

* Fixed build errors

* Made event flags test

* poop and farts

* integration go brrr

* should've fixed getGear()

* the dashboard driver test is back baby

* pee

* Fixed getGear() stuff hopefully. Lakshay pookie bookie wookie:0

* poop

* akshay gaitonde

* fix active precharge can timeouts

* funny timeout

* i forgot what i changed

* send iostate msg in fault

* fix my silly little contactors_get code for motor

* pp

* lmao skill diff no balls

* some changes for the flags to centralize things

* some error assertion logic with the flags

* build fix

* change modyfy bits to be more intuative with setting vs clearign

* addressed comments + change stuff

* made seperate test for motor_safe

* Error rewrite (#457)

* changed error paradigm to send error as a param rather than a global error buffer; also did some formatting (slightly accidentally)

* oopsie, build errors

* fixed build error

* finished error stuff (and touched more files 😈)

* some stuff

* bug fixes in script and in errmsg + touched hella files

* hi roie

* goated sendcarcan

* ishdeshpa.com

* motor go brrrrrrrrrrrrrrrrrrrrrrrrrrrr

---------

Co-authored-by: root <root@oscar>
Co-authored-by: sodaplayzz <sc.seaofthieves@gmail.com>
Co-authored-by: Lakshay Gupta <56173382+Lakshay983@users.noreply.github.com>
Co-authored-by: Lakshay983 <lakshay45gupta@gmail.com>
Co-authored-by: Akshay G <a.g@utexas.edu>
Co-authored-by: guytonde <72145434+guytonde@users.noreply.github.com>
* poop and balls

* shows array timeout fault now + remove minion :)

* motor go brrr with pedals

* updated some display stuff

* whoops, i forgot to push

* ESTOP

* started adding special funny things about bps fault

* remove fake current

* show bps fault

* pp poo poo powered run go brrrrrrr

* add motor limit display stuff

* show motor limit on display

* fixed pinout

* poopooo

* no work

* show left and right indiactors

* new display config

* poo poo

* a

* lmao actual pedals integration

* add more motor can messgaes

* added back power cmd

* Turn Signal Lights (#459)

* initial commit -- untested lights without PWM

* janktacular

* bruh

* test main

* ts works fr it better ong

* hardfault = all leds on :)

* balls2 (thanks lakshay)

* balls - lakshay (working)

* cleanup

* test blinky on car and add it in the bps fault handler

---------

Co-authored-by: Diya Rajon <diya.rajon@gmail.com>
Co-authored-by: Lakshay983 <lakshay45gupta@gmail.com>

* require accel to be reset before spinning motor

* adjusted thresholds

* Canbus receive fault state (#461)

* canbus recieve go brrrr

* fix build error

---------

Co-authored-by: guytonde <72145434+guytonde@users.noreply.github.com>
Co-authored-by: Akshay G <a.g@utexas.edu>

* debugging can send fault state

* half working lights code

* lights

* laksee

* Display fixes (#463)

* update display stuff (and some shit)

* fixed moco faults not coming

* Revert "Display fixes (#463)"

This reverts commit 0bebaf1.

* display_rewrite

* Motor current setpoint scale (#462)

* build fix

* fixy

* brake works

---------

Co-authored-by: Akshay G <a.g@utexas.edu>
Co-authored-by: guytonde <72145434+guytonde@users.noreply.github.com>

* poop

---------

Co-authored-by: sodaplayzz <62674929+sodaplayzz@users.noreply.github.com>
Co-authored-by: Diya Rajon <diya.rajon@gmail.com>
Co-authored-by: guytonde <72145434+guytonde@users.noreply.github.com>
Co-authored-by: Akshay G <a.g@utexas.edu>
Co-authored-by: Ravi Shah <ravidhruvishshah@gmail.com>
@Lakshay983
Copy link

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a significant and well-structured refactoring for the Daybreak 2025 integration. The changes include a new centralized error handling system, the use of event flags for better task synchronization, and the replacement of the generic 'Minions' driver with more specific drivers like 'Ignition', 'Dashboard', and 'Lights'. The motor control logic has been simplified and made safer. Overall, these changes greatly improve the modularity, safety, and readability of the codebase. I've identified a few issues, including a critical bug in a printf format string, some typos, and areas with incomplete or unclear logic that need attention.

printf("Pedal Accel Percent: %u\n\r", get_accelPedalPercent());
printf("Current Gear: %s\n\r", GEAR_STRING[get_gear()]);
print_float("Current Setpoint: ", get_currentSetpoint());
print_float("Velocity Setpoint: %s\n\r", get_velocitySetpoint()); // velocitySetpoint

Choose a reason for hiding this comment

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

critical

The format specifier %s is used here for get_velocitySetpoint(), which returns a float. This will lead to incorrect output or a crash. The print_float function likely takes a string prefix and a float, not a format string.

Suggested change
print_float("Velocity Setpoint: %s\n\r", get_velocitySetpoint()); // velocitySetpoint
print_float("Velocity Setpoint: ", get_velocitySetpoint()); // velocitySetpoint

Comment on lines +213 to +220
case C_ERR_RTR_MOTOR_WDOG_TRIP:
// Try to restart the motor a few times and then fail out
if (++motor_fault_cnt > RESTART_THRESHOLD) {
// throwTaskError(m_err, !EVAC_NEEDED, NULL, OPT_LOCK_SCHED, OPT_NONRECOV, CAN_NONE_BPS);
} else {
// throwTaskError(m_err, !EVAC_NEEDED, handler_ReadTritium_HallError, OPT_NO_LOCK_SCHED, OPT_NONRECOV, CAN_NONE_BPS);
}
break;

Choose a reason for hiding this comment

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

high

The error handling for C_ERR_RTR_MOTOR_WDOG_TRIP appears to be incomplete, as the throwTaskError calls are commented out. This means a motor watchdog trip will not be handled, which is a significant safety risk. Please implement the intended logic or remove the case if it's not meant to be handled here.

Comment on lines +335 to +337
// (TODO) This should be UART4 IRQ Handler???? IDK BSP HELPPPPP
// This was USART3_IRQHandler... I tried some stuff, likely to be wrong
void UART4_IRQHandler(void) {

Choose a reason for hiding this comment

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

high

The TODO comment here indicates uncertainty about the correct IRQ handler for UART4. This is a critical piece of functionality for the display. Please verify that UART4_IRQHandler is the correct handler and that it is properly registered in the interrupt vector table. Once confirmed, this comment should be removed.


#define GEAR_FAULT_THRESHOLD 3 // number of times gear fault can occur before it is considered a fault

#define ACCCEL_PEDAL_RESET_THRESHOLD 20

Choose a reason for hiding this comment

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

medium

There is a typo in this macro name. It should be ACCEL_PEDAL_RESET_THRESHOLD.

Suggested change
#define ACCCEL_PEDAL_RESET_THRESHOLD 20
#define ACCEL_PEDAL_RESET_THRESHOLD 20

Comment on lines +31 to +33
// don't ask me why BRAKE_LIGHT_PIN is a diff naming convention
#define BRAKE_LIGHT_PORT PORTC
#define BRAKE_LIGHT_PIN GPIO_Pin_6

Choose a reason for hiding this comment

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

medium

The comment on line 31 mentions an inconsistent naming convention. To improve consistency across the file, consider renaming BRAKE_LIGHT_PIN to BRAKE_LIGHT.

Suggested change
// don't ask me why BRAKE_LIGHT_PIN is a diff naming convention
#define BRAKE_LIGHT_PORT PORTC
#define BRAKE_LIGHT_PIN GPIO_Pin_6
#define BRAKE_LIGHT_PORT PORTC
#define BRAKE_LIGHT GPIO_Pin_6

Comment on lines +35 to +90
#define RIGHT_INDICATOR_PORT PORTA
#define RIGHT_INDICATOR GPIO_Pin_15

// #define PA8_PORT PORTA
// #define PA8 GPIO_Pin_8
#define LEFT_INDICATOR_PORT PORTC
#define LEFT_INDICATOR GPIO_Pin_10

// #define MOTOR_C_SENSE_PORT PORTA
// #define MOTOR_C_SENSE GPIO_Pin_9
#define OS_FAULT_PORT PORTA
#define OS_FAULT GPIO_Pin_5

// #define MOTOR_CONTACTOR_PORT PORTA
// #define MOTOR_CONTACTOR GPIO_Pin_10
#define BPS_HAZARD_PORT PORTA
#define BPS_HAZARD GPIO_Pin_6

// #define PA15_PORT PORTA
// #define PA15 GPIO_Pin_15
#define CRUISE_SET_PORT PORTA
#define CRUISE_SET GPIO_Pin_7

// #define BPS_FAULT_PORT PORTB
// #define BPS_FAULT GPIO_Pin_0
#define MOTOR_C_SENSE_PORT PORTA
#define MOTOR_C_SENSE GPIO_Pin_9

// #define IG1_PORT PORTB
// #define IG1 GPIO_Pin_1
#define MOTOR_CONTACTOR_PORT PORTA
#define MOTOR_CONTACTOR GPIO_Pin_10

// #define FORWARD_PORT PORTB
// #define FORWARD GPIO_Pin_2
#define RIGHT_IND_PORT PORTA
#define RIGHT_IND GPIO_Pin_15

// #define PB7_PORT PORTB
// #define PB7 GPIO_Pin_7
#define BPS_FAULT_PORT PORTB
#define BPS_FAULT GPIO_Pin_0

// #define ARRAY_PBC_SENSE_PORT PORTB
// #define ARRAY_PBC_SENSE GPIO_Pin_8
#define IG1_PORT PORTB
#define IG1 GPIO_Pin_1

// #define MOTOR_PBC_SENSE_PORT PORTB
// #define MOTOR_PBC_SENSE GPIO_Pin_9
#define FORWARD_PORT PORTB
#define FORWARD GPIO_Pin_2

// #define IG2_PORT PORTB
// #define IG2 GPIO_Pin_10
#define IG2_PORT PORTB
#define IG2 GPIO_Pin_10

// #define MOTOR_CTRL_FAULT_PORT PORTB
// #define MOTOR_CTRL_FAULT GPIO_Pin_14
#define MOTOR_CTRL_FAULT_PORT PORTB
#define MOTOR_CTRL_FAULT GPIO_Pin_14

// #define REVERSE_PORT PORTB
// #define REVERSE GPIO_Pin_15
#define REVERSE_PORT PORTB
#define REVERSE GPIO_Pin_15

// #define CONTROLS_FAULT_PORT PORTC
// #define CONTROLS_FAULT GPIO_Pin_4
#define CONTROLS_FAULT_PORT PORTC
#define CONTROLS_FAULT GPIO_Pin_4

// #define CRUISE_ENABLE_PORT PORTC
// #define CRUISE_ENABLE GPIO_Pin_5
#define CRUISE_ENABLE_PORT PORTC
#define CRUISE_ENABLE GPIO_Pin_5

// #define TIMER_CLK_PORT PORTC
// #define TIMER_CLK GPIO_Pin_6
#define HEARTBEAT_PORT PORTC
#define HEARTBEAT_PIN GPIO_Pin_7

// #define HEARTBEAT_PORT PORTC
// #define HEARTBEAT GPIO_Pin_7
#define CRUISE_IND_PORT PORTC
#define CRUISE_IND GPIO_Pin_8

// #define CRUISE_IND_PORT PORTC
// #define CRUISE_IND GPIO_Pin_8
#define LEFT_IND_PORT PORTC
#define LEFT_IND GPIO_Pin_10

Choose a reason for hiding this comment

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

medium

There appear to be duplicate definitions for the indicator light pins, which could lead to confusion.

  • RIGHT_INDICATOR_PORT and RIGHT_INDICATOR (lines 35-36) are duplicates of RIGHT_IND_PORT and RIGHT_IND (lines 56-57).
  • LEFT_INDICATOR_PORT and LEFT_INDICATOR (lines 38-39) are duplicates of LEFT_IND_PORT and LEFT_IND (lines 89-90).

Please remove the duplicate definitions. I'd recommend keeping the more descriptive names (e.g., RIGHT_INDICATOR_PORT).

Comment on lines +199 to +213
inline void delay_ms(uint32_t ms) {
// Adjusted loop count per ms based on empirical timing
// Originally: 20,000 per ms (80,000 cycles / 4 cycles/iter)
// Observed: ~3.77× slower → need ~5300 iterations per ms
uint32_t count = ms * 5300;

__asm__ volatile(
"1: \n"
"subs %[cnt], %[cnt], #1 \n"
"bne 1b \n"
: [cnt] "+r"(count)
:
: "cc"
);
}

Choose a reason for hiding this comment

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

medium

The delay_ms function uses a magic number 5300 for loop iterations per millisecond, which was derived empirically. This approach is brittle and highly dependent on the specific hardware and compiler settings, making it prone to breaking with future changes.

For better portability and reliability, please consider using OS-provided delay functions like OSTimeDlyHMSM. If a busy-wait is absolutely necessary for very short, precise delays, please add detailed comments explaining the calibration environment (e.g., compiler version, optimization flags, and hardware specs).


float Car_Velocity = Motor_Velocity * MPH_CONVERSION; // Car vel is in mph
// Display can't take negative values, and reverse puts Car_Velocity in the negative
Car_Velocity = (Car_Velocity < 0) ? -Car_Velocity : Car_Velocity;

Choose a reason for hiding this comment

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

medium

For clarity and to follow standard library usage, consider using fabsf() from <math.h> to get the absolute value of the float. You will need to add #include <math.h> at the top of the file.

Suggested change
Car_Velocity = (Car_Velocity < 0) ? -Car_Velocity : Car_Velocity;
Car_Velocity = fabsf(Car_Velocity);

@guytonde
Copy link

/gemini help

@gemini-code-assist
Copy link

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

@Lakshay983
Copy link

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a significant integration for "Daybreak 2025", involving a large-scale refactoring of the codebase. Key changes include a new centralized error handling system, updated drivers for new hardware, and a rewrite of several application tasks. While the new architecture appears to be an improvement, I've identified several critical and high-severity issues. These include a printf format string bug that could lead to a crash, disabled safety features like CAN watchdogs and BPS trip handlers, and the use of inefficient busy-wait delays. There are also numerous instances of commented-out code that should be cleaned up to improve maintainability.

printf("Pedal Accel Percent: %u\n\r", get_accelPedalPercent());
printf("Current Gear: %s\n\r", GEAR_STRING[get_gear()]);
print_float("Current Setpoint: ", get_currentSetpoint());
print_float("Velocity Setpoint: %s\n\r", get_velocitySetpoint()); // velocitySetpoint

Choose a reason for hiding this comment

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

critical

There is a format specifier mismatch in this print_float call. The format string includes %s, which is for a string, but get_velocitySetpoint() returns a float. This will result in undefined behavior and likely crash the program. Assuming print_float handles the formatting, the format specifier should be removed from the string literal.

Suggested change
print_float("Velocity Setpoint: %s\n\r", get_velocitySetpoint()); // velocitySetpoint
print_float("Velocity Setpoint: ", get_velocitySetpoint()); // velocitySetpoint

Comment on lines +218 to +222
if (++motor_fault_cnt > RESTART_THRESHOLD) {
// throwTaskError(m_err, !EVAC_NEEDED, NULL, OPT_LOCK_SCHED, OPT_NONRECOV, CAN_NONE_BPS);
} else {
// throwTaskError(m_err, !EVAC_NEEDED, handler_ReadTritium_HallError, OPT_NO_LOCK_SCHED, OPT_NONRECOV, CAN_NONE_BPS);
}

Choose a reason for hiding this comment

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

critical

The error handling for a motor watchdog trip is commented out. A watchdog trip indicates a serious problem with the motor controller, and failing to handle it is a critical safety risk. This logic should be uncommented and fully implemented to ensure the system responds correctly to this fault.

// BPS has a fault and we need to enter fault state
if (dataBuf.data[0] == BPS_TRIP_MESSAGE) {
// kill motor contactor and enter a nonrecoverable fault
//assertReadCarCANError(C_ERR_RCC_BPS_TRIP);

Choose a reason for hiding this comment

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

critical

The BPS trip handler is commented out. A BPS trip is a critical fault that must be handled immediately to ensure the safety of the vehicle and its driver. This line should be uncommented to ensure the system enters a non-recoverable fault state as intended.

Suggested change
//assertReadCarCANError(C_ERR_RCC_BPS_TRIP);
assertReadCarCANError(C_ERR_RCC_BPS_TRIP);

Comment on lines +22 to +23
// #define BPS_CAN_WATCHDOG
// #define PRECHARGE_CAN_WATCHDOG

Choose a reason for hiding this comment

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

high

The CAN watchdogs for BPS and Precharge messages are disabled via #ifdef. These are critical safety features that ensure the system is receiving timely updates from these components. If they are intended to be active, they should be enabled by default to prevent running in an unsafe state.

Comment on lines +199 to +213
inline void delay_ms(uint32_t ms) {
// Adjusted loop count per ms based on empirical timing
// Originally: 20,000 per ms (80,000 cycles / 4 cycles/iter)
// Observed: ~3.77× slower → need ~5300 iterations per ms
uint32_t count = ms * 5300;

__asm__ volatile(
"1: \n"
"subs %[cnt], %[cnt], #1 \n"
"bne 1b \n"
: [cnt] "+r"(count)
:
: "cc"
);
}

Choose a reason for hiding this comment

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

high

This delay_ms function uses a busy-wait loop, which is inefficient in an RTOS environment as it consumes CPU cycles without yielding to other tasks. The timing is also dependent on CPU clock speed and compiler optimizations, making it unreliable. It's recommended to use RTOS-provided delay functions like OSTimeDlyHMSM() for predictable and efficient delays.

Comment on lines +26 to +35
// /**
// * Error types
// */
// typedef enum {
// UPDATEDISPLAY_ERR_NONE,
// // UPDATEDISPLAY_ERR_FIFO_PUT, // Error putting command in fifo
// // UPDATEDISPLAY_ERR_FIFO_POP, // Error popping command from fifo
// UPDATEDISPLAY_ERR_PARSE_COMP, // Error parsing component/val in SetComponent
// UPDATEDISPLAY_ERR_DRIVER // Driver call returned an error
// } controls_error_e;

Choose a reason for hiding this comment

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

medium

The old error enum for UpdateDisplay is commented out. This should be removed to avoid clutter and confusion, especially since a new centralized error handling system is in place.


#define GEAR_FAULT_THRESHOLD 3 // number of times gear fault can occur before it is considered a fault

#define ACCCEL_PEDAL_RESET_THRESHOLD 20

Choose a reason for hiding this comment

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

medium

There is a typo in the macro name. It should be ACCEL_PEDAL_RESET_THRESHOLD.

Suggested change
#define ACCCEL_PEDAL_RESET_THRESHOLD 20
#define ACCEL_PEDAL_RESET_THRESHOLD 20

Comment on lines +21 to +24
// typedef enum {
// IOSTATE_ERR_NONE = 0x0000, // No error
// IOSTATE_ERROR = 0xFFFF, // More than one state is high at a time
// } IOState_error_code_t;

Choose a reason for hiding this comment

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

medium

This IOState_error_code_t enum is commented out. If it's no longer needed due to the new centralized error handling system, it should be removed to improve code clarity and reduce clutter.

Comment on lines +24 to +37
// typedef enum{
// T_HARDWARE_OVER_CURRENT_ERR = (1<<0),
// T_SOFTWARE_OVER_CURRENT_ERR = (1<<1),
// T_DC_BUS_OVERVOLT_ERR = (1<<2),
// T_HALL_SENSOR_ERR = (1<<3),
// T_WATCHDOG_LAST_RESET_ERR = (1<<4),
// T_CONFIG_READ_ERR = (1<<5),
// T_UNDER_VOLTAGE_LOCKOUT_ERR = (1<<6),
// T_DESAT_FAULT_ERR = (1<<7),
// T_MOTOR_OVER_SPEED_ERR = (1<<8),
// T_INIT_FAIL = (1<<9), //motor controller fails to restart or initialize
// T_MOTOR_WATCHDOG_TRIP = (1 << 15),
// T_NONE = 0x00,
// } tritium_error_code_t;

Choose a reason for hiding this comment

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

medium

The tritium_error_code_t enum is commented out. With the new centralized error handling, this is likely obsolete and should be removed to improve code clarity.

Comment on lines +20 to +32
// /**
// * Error types
// */
// typedef enum
// {
// READCARCAN_ERR_NONE = 0x0000,
// //READCARCAN_ERR_CHARGE_DISABLE = 0xAAAA, // Received a charge disable msg
// READCARCAN_ERR_BPS_MISSED_MSG = 0xBBBB, // Didn't receive a BPS charge msg in time
// READCARCAN_ERR_PCHG_MISSED_MSG = 0xCCCC, // Didn't receive a precharge msg in time
// //READCARCAN_ERR_DISABLE_CONTACTORS_MSG = 0xCCCC, // Ignition is turned to neither (off due to LV) or both at the same time (impossible) are on at
// READCARCAN_ERR_BPS_TRIP = 0xDDDD, // Received a BPS trip msg (0 or 1)
// READCARCAN_ERR_ACTIVE_PRECHARGE_FAULT = 0xEEEE, // Active precharge sent a fault message
// } ReadCarCAN_error_code_t;

Choose a reason for hiding this comment

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

medium

The old ReadCarCAN_error_code_t enum is commented out. Since a new centralized error enum controls_error_e has been introduced, this old definition should be removed to avoid confusion and keep the codebase clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request M2.5 Issues to be addressed before July 2025 Race

Projects

None yet

Development

Successfully merging this pull request may close these issues.

New Pinout