Skip to content

Refactor valve control logic and improve error handling #118

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.

Sign up for GitHub

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
niekky wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Refactor valve control logic and improve error handling #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
611ac3c
valve-hot-fire-hotfixes
niekky Mar 2, 2025
9f4d1f3
feat: unuse tdelta calculation in engine mod
niekky Apr 13, 2026
82b1ec8
stop at valve state closed instead of valve pos
niekky Apr 13, 2026
9b234bd
Merge branch 'main' of github.com:SunDevilRocketry/mod into valve-hot…
niekky Apr 13, 2026
91efe1a
Merge branch 'liquids-compatible' of github.com:SunDevilRocketry/mod …
niekky Apr 13, 2026
f2c81a2
Added error code for valve timer init
niekky Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions common/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,10 @@ typedef enum _ERROR_CODE
ERROR_SENSOR_FORMAT_ERROR, /* unused */
ERROR_INVALID_STATE_ERROR, /* FSM has reached a state that it shouldn't */
ERROR_CONFIG_VALIDITY_ERROR, /* Loaded configuration is invalid */
ERROR_IGNITION_CONTINUITY_ERROR /* Parachute terminals do not have continuity */
} ERROR_CODE;
ERROR_IGNITION_CONTINUITY_ERROR, /* Parachute terminals do not have continuity */
ERROR_FUEL_TIM_INIT_ERROR,
ERROR_LOX_TIM_INIT_ERROR
Comment on lines +101 to +102

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

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

New enum entries ERROR_FUEL_TIM_INIT_ERROR and ERROR_LOX_TIM_INIT_ERROR were added without the inline comments used by most other ERROR_CODE values. Adding brief comments (and keeping formatting consistent with adjacent entries) will make diagnostics/telemetry easier to interpret.

Suggested change
ERROR_FUEL_TIM_INIT_ERROR,
ERROR_LOX_TIM_INIT_ERROR
ERROR_FUEL_TIM_INIT_ERROR, /* Error initializing fuel timer */
ERROR_LOX_TIM_INIT_ERROR /* Error initializing LOX timer */

Copilot uses AI. Check for mistakes.
} ERROR_CODE;


/*------------------------------------------------------------------------------
Expand Down
10 changes: 8 additions & 2 deletions sensor/sensor.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,9 @@

/* Hash table of sensor readout sizes and offsets */
static SENSOR_DATA_SIZE_OFFSETS sensor_size_offsets_table[ NUM_SENSORS ];
extern volatile uint32_t tdelta, previous_time;

#ifdef FLIGHT_COMPUTER
extern volatile uint32_t tdelta, previous_time;
extern GPS_DATA gps_data;
extern IMU_OFFSET imu_offset;
#endif
Expand Down Expand Up @@ -468,8 +468,10 @@ switch ( subcommand )
}
#endif

#ifdef FLIGHT_COMPUTER
// Reset start time
previous_time = HAL_GetTick();
#endif

/* Start polling sensors */
while ( sensor_poll_cmd != SENSOR_POLL_STOP )
Expand Down Expand Up @@ -513,8 +515,10 @@ switch ( subcommand )
/* Poll Sensors */
case SENSOR_POLL_REQUEST:
{
#ifdef FLIGHT_COMPUTER
tdelta = HAL_GetTick() - previous_time;
previous_time = HAL_GetTick();
#endif /* FLIGHT_COMPUTER */
sensor_status = sensor_poll( &sensor_data ,
&poll_sensors[0],
num_sensors );
Expand Down Expand Up @@ -543,9 +547,11 @@ switch ( subcommand )
/* STOP Executtion */

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

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

Typo in comment: “STOP Executtion” should be “STOP Execution”.

Suggested change
/* STOP Executtion */
/* STOP Execution */

Copilot uses AI. Check for mistakes.
case SENSOR_POLL_STOP:
{
// Reset timing
#ifdef FLIGHT_COMPUTER
// Reset timing for flight computer
previous_time = 0;
tdelta = 0;
#endif
break;
} /* case SENSOR_POLL_STOP */

Expand Down
9 changes: 6 additions & 3 deletions solenoid/solenoid.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,13 @@ switch(solenoid_base_code)
case SOL_GETSTATE_CODE:
{
sol_state = solenoid_get_state();
#if defined( TERMINAL )
#if defined( TERMINAL )
usb_transmit( &sol_state, sizeof( sol_state ), HAL_DEFAULT_TIMEOUT );
#elif defined( HOTFIRE )
valve_transmit( &sol_state, sizeof( sol_state ), HAL_DEFAULT_TIMEOUT );
#elif defined( HOTFIRE )
if (USB_MODE)
usb_transmit( &sol_state, sizeof( sol_state ), HAL_DEFAULT_TIMEOUT );
else
valve_transmit( &sol_state, sizeof( sol_state ), HAL_DEFAULT_TIMEOUT );
Comment on lines +170 to +174

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

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

USB_MODE is referenced here but does not appear to be defined anywhere in the repository (no #define or global symbol found). As-is, this will fail to compile for HOTFIRE builds unless the build system injects it; consider defining it in a shared header (or using an existing compile-time flag) and documenting its expected meaning/type.

Copilot uses AI. Check for mistakes.
#endif
break;
}
Expand Down
95 changes: 80 additions & 15 deletions valve/valve.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,9 +282,14 @@ switch( subcommand )
{
main_valve_states = valve_get_valve_states();
#if defined( HOTFIRE )
valve_transmit( &main_valve_states ,
sizeof( main_valve_states ),
HAL_DEFAULT_TIMEOUT );
if (USB_MODE){
usb_transmit( &main_valve_states ,
sizeof( main_valve_states ),
HAL_DEFAULT_TIMEOUT );
} else
valve_transmit( &main_valve_states ,
sizeof( main_valve_states ),
HAL_DEFAULT_TIMEOUT );
Comment on lines +285 to +292

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

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

USB_MODE is referenced here but does not appear to be defined anywhere in the repository (no #define or global symbol found). As-is, this will fail to compile for HOTFIRE builds unless the build system injects it; consider defining it in a shared header (or using an existing compile-time flag) and documenting its expected meaning/type.

Copilot uses AI. Check for mistakes.
#elif defined( TERMINAL )
usb_transmit( &main_valve_states,
sizeof( main_valve_states ),
Expand Down Expand Up @@ -569,6 +574,58 @@ return VALVE_OK;
} /* valve_open_fuel_valve */


/*******************************************************************************
* *
* PROCEDURE: *
* valve_open_fuel_valve *
* *
* DESCRIPTION: *
* Open the main fuel valve *
* *
*******************************************************************************/
VALVE_STATUS valve_slow_open_fuel_valve
(
void
)
{
/*------------------------------------------------------------------------------
Local Variables
------------------------------------------------------------------------------*/
VALVE_STATUS valve_status; /* Status return codes from valve API */


/*------------------------------------------------------------------------------
Initializations
------------------------------------------------------------------------------*/
valve_status = VALVE_OK;


/*------------------------------------------------------------------------------
Implementation
------------------------------------------------------------------------------*/

/* Check if valve is already open */
if ( fuel_valve_pos == VALVE_OPEN_POS )
{
return VALVE_OK;
}

/* Set the direction */
valve_status = fuel_driver_set_direction( STEPPER_DRIVER_CW );
if ( valve_status != VALVE_OK )
{
return valve_status;
}

/* Actuate the valve */
fuel_valve_opening = true;
HAL_TIM_PWM_Start( &( VALVE_FUEL_TIM ), VALVE_FUEL_TIM_CHANNEL );
return VALVE_OK;
Comment on lines +586 to +623

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

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

valve_slow_open_fuel_valve() is currently identical to valve_open_fuel_valve() (same direction set + fuel_valve_opening = true + HAL_TIM_PWM_Start), so it does not actually perform a “slow open”. Either implement a genuinely slower actuation path (e.g., reduced step frequency / different timer config / ramp) or remove/rename this API to avoid misleading callers.

Copilot uses AI. Check for mistakes.
} /* valve_open_fuel_valve */

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

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

The function header and end-of-function comment for valve_slow_open_fuel_valve still refer to valve_open_fuel_valve and “Open the main fuel valve”. This makes the documentation misleading; update the PROCEDURE/DESCRIPTION block and the trailing comment to match the actual function name/behavior.

Suggested change
} /* valve_open_fuel_valve */
} /* valve_slow_open_fuel_valve */

Copilot uses AI. Check for mistakes.




/*******************************************************************************
* *
* PROCEDURE: *
Expand Down Expand Up @@ -922,22 +979,22 @@ if ( HAL_GPIO_ReadPin( LOX_ENC_GPIO_PORT, LOX_ENC_A_PIN ) )
if ( !lox_channelB_state )
{
//lox_valve_pos -= 1;
dec_lox_encoder();
// dec_lox_encoder();

/* Detect valve closed position */
if ( ox_valve_closing && ( lox_valve_pos == VALVE_CLOSED_POS ) )
{
HAL_TIM_PWM_Stop( &( VALVE_LOX_TIM ), VALVE_LOX_TIM_CHANNEL );
ox_valve_closing = false;
}
/*
// if ( ox_valve_closing && ( lox_valve_pos == VALVE_CLOSED_POS ) )
// {
// HAL_TIM_PWM_Stop( &( VALVE_LOX_TIM ), VALVE_LOX_TIM_CHANNEL );
// ox_valve_closing = false;
// }

if ( ox_valve_closing && ( valve_get_ox_valve_state() == VALVE_CLOSED ) )
{
HAL_TIM_PWM_Stop( &( VALVE_LOX_TIM ), VALVE_LOX_TIM_CHANNEL );
ox_valve_closing = false;
lox_valve_pos = 0;
}
Comment on lines 981 to 996

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

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

dec_lox_encoder() and the encoder-count-based closed-position stop logic are commented out, so lox_valve_pos no longer decrements during closing and the close-stop condition relies solely on valve_get_ox_valve_state(). This removes the encoder position as a fallback safety limit; consider restoring the decrement/update and/or adding a max-steps/timeout safeguard so a photogate failure can’t leave the PWM running indefinitely.

Copilot uses AI. Check for mistakes.
*/

}
}
/* High to Low Transition */
Expand Down Expand Up @@ -1015,13 +1072,21 @@ if ( HAL_GPIO_ReadPin( KER_ENC_GPIO_PORT, KER_ENC_A_PIN ) )
if ( !fuel_channelB_state )
{
//fuel_valve_pos -= 1;
dec_fuel_encoder();
// dec_fuel_encoder();

/* Detect valve closed */
if ( fuel_valve_closing && ( fuel_valve_pos == VALVE_CLOSED_POS ) )
// if ( fuel_valve_closing && ( fuel_valve_pos == VALVE_CLOSED_POS ) )
// {
// HAL_TIM_PWM_Stop( &( VALVE_FUEL_TIM ), VALVE_FUEL_TIM_CHANNEL );
// fuel_valve_closing = false;
// }
// }

if ( fuel_valve_closing && ( valve_get_fuel_valve_state() == VALVE_CLOSED ) )
{
HAL_TIM_PWM_Stop( &( VALVE_FUEL_TIM ), VALVE_FUEL_TIM_CHANNEL );
fuel_valve_closing = false;
fuel_valve_pos = 0;
}
Comment on lines 1074 to 1090

Copilot AI Apr 13, 2026

Copy link

Choose a reason for hiding this comment

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

dec_fuel_encoder() and the encoder-count-based closed-position stop logic are commented out, so fuel_valve_pos no longer decrements during closing and the close-stop condition relies solely on valve_get_fuel_valve_state(). This removes the encoder position as a fallback safety limit; consider restoring the decrement/update and/or adding a max-steps/timeout safeguard so a photogate failure can’t leave the PWM running indefinitely.

Copilot uses AI. Check for mistakes.
}
}
Expand Down Expand Up @@ -1273,13 +1338,13 @@ main_valve_states = 0;
/* Check the LOX valve */
if ( valve_get_ox_valve_state() == VALVE_OPEN )
{
main_valve_states |= ( 1 << 7 );
main_valve_states |= ( 1 << 6 );
}

/* Check the fuel valve */
if ( valve_get_fuel_valve_state() == VALVE_OPEN )
{
main_valve_states |= ( 1 << 6 );
main_valve_states |= ( 1 << 7 );
}

return main_valve_states;
Expand Down
6 changes: 6 additions & 0 deletions valve/valve.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,12 @@ VALVE_STATUS valve_open_fuel_valve
void
);

/* Slowly open the main fuel valve */
VALVE_STATUS valve_slow_open_fuel_valve
(
void
);

/* Close the main oxidizer valve */
VALVE_STATUS valve_close_ox_valve
(
Expand Down
Loading