Refactor valve control logic and improve error handling by niekky · Pull Request #118 · SunDevilRocketry/mod

niekky · 2026-04-13T19:49:51Z

This pull request introduces several improvements and bug fixes across the sensor, solenoid, and valve modules, with a focus on timing management, valve control logic, and communication behavior. The most significant changes include the addition of slow fuel valve opening functionality, improved timing and state handling for flight computers, and corrections to main valve state bit assignments. Below are the key changes grouped by theme:

Valve Control Enhancements:

Added a new function valve_slow_open_fuel_valve to allow for slow opening of the main fuel valve, including its declaration in valve.h and implementation in valve.c. [1] [2]
Updated logic for detecting valve closed positions for both LOX and fuel valves to use valve_get_*_valve_state() instead of relying solely on position counters, improving reliability. [1] [2]
Corrected the bit assignments for main valve states: LOX and fuel valve open states are now properly mapped to bits 6 and 7, respectively, in the main_valve_states variable.

Sensor Timing and State Management:

Wrapped timing variable updates (tdelta, previous_time) in #ifdef FLIGHT_COMPUTER blocks to ensure they are only manipulated on the flight computer, preventing unintended side effects on other platforms. [1] [2] [3] [4]

Communication Behavior:

Modified solenoid and valve communication logic in HOTFIRE mode: now, data is sent via USB if USB_MODE is enabled, otherwise it defaults to valve transmission, ensuring the correct communication channel is used. [1] [2]

Error Code Extensions:

Added new error codes ERROR_FUEL_TIM_INIT_ERROR and ERROR_LOX_TIM_INIT_ERROR to the ERROR_CODE enum in common.h for improved diagnostics during TIM initialization.

…-fire-hotfixes

…into valve-hot-fire-hotfixes

Copilot

Pull request overview

This PR updates embedded firmware modules around valve/solenoid command handling, sensor polling timing, and shared error codes to improve correctness and platform-specific behavior (e.g., flight computer vs. others, HOTFIRE comms routing).

Changes:

Added a new valve API entry point for “slow opening” the main fuel valve and adjusted main valve state bit packing.
Updated HOTFIRE command responses to optionally transmit over USB instead of the valve UART.
Guarded sensor timing globals/updates behind FLIGHT_COMPUTER and extended ERROR_CODE with TIM init errors.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
valve/valve.h	Declares new `valve_slow_open_fuel_valve()` API.
valve/valve.c	Adds HOTFIRE USB transmit option, introduces slow-open function, changes close detection logic, and fixes valve-state bit assignments.
solenoid/solenoid.c	Routes HOTFIRE SOL_GETSTATE responses via USB when enabled.
sensor/sensor.c	Limits timing globals/updates to `FLIGHT_COMPUTER` builds.
common/common.h	Adds new TIM init error codes to the shared error enum.

Comments suppressed due to low confidence (1)

valve/valve.c:298

case VALVE_GETSTATE_CODE does not break or return after transmitting, so execution will fall through to default: and return VALVE_UNRECOGNIZED_SUBCOMMAND even on success. Add an explicit break; (or return VALVE_OK;) at the end of this case.

	case VALVE_GETSTATE_CODE:
		{
		main_valve_states = valve_get_valve_states();
		#if defined( HOTFIRE )
			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 );
		#elif defined( TERMINAL )
			usb_transmit( &main_valve_states, 
			              sizeof( main_valve_states ),
						  HAL_DEFAULT_TIMEOUT );
		#endif
		}

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-13T19:55:04Z

@@ -543,9 +547,11 @@ switch ( subcommand )
 				/* STOP Executtion */


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

Suggested change

/* STOP Executtion */

/* STOP Execution */

Copilot · 2026-04-13T19:55:04Z

+		#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 );


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 · 2026-04-13T19:55:05Z

+    ERROR_FUEL_TIM_INIT_ERROR,
+    ERROR_LOX_TIM_INIT_ERROR


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 · 2026-04-13T19:55:05Z

+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;


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 · 2026-04-13T19:55:05Z

+fuel_valve_opening = true;
+HAL_TIM_PWM_Start( &( VALVE_FUEL_TIM ), VALVE_FUEL_TIM_CHANNEL );
+return VALVE_OK;
+} /* valve_open_fuel_valve */


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 · 2026-04-13T19:55:06Z

 		//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;
 			}


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 · 2026-04-13T19:55:06Z

 		//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;
 			}


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 · 2026-04-13T19:55:06Z

+			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 );


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.

ETSells · 2026-04-19T23:39:04Z

Recommend some changes here:

Valve and solenoid have been moved to driver on the active dev branch. Common no longer exists -- these error codes should move to error_sdr.

Please target the active dev branch or staging to avoid diverging history.

ETSells · 2026-06-08T04:21:51Z

This branch is heavily conflicted. We'll need to verify whether or not all of this stuff works.

niekky added 6 commits March 2, 2025 13:53

valve-hot-fire-hotfixes

611ac3c

feat: unuse tdelta calculation in engine mod

9f4d1f3

stop at valve state closed instead of valve pos

82b1ec8

Merge branch 'main' of github.com:SunDevilRocketry/mod into valve-hot…

9b234bd

…-fire-hotfixes

Merge branch 'liquids-compatible' of github.com:SunDevilRocketry/mod …

91efe1a

…into valve-hot-fire-hotfixes

Added error code for valve timer init

f2c81a2

niekky requested a review from Copilot April 13, 2026 19:49

Copilot started reviewing on behalf of niekky April 13, 2026 19:50 View session

Copilot AI reviewed Apr 13, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Refactor valve control logic and improve error handling#118

Refactor valve control logic and improve error handling#118
niekky wants to merge 6 commits into
mainfrom
liquids-compatible

niekky commented Apr 13, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

Copilot AI Apr 13, 2026

Uh oh!

ETSells commented Apr 19, 2026

Uh oh!

ETSells commented Jun 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		@@ -543,9 +547,11 @@ switch ( subcommand )
		/* STOP Executtion */

	} /* valve_open_fuel_valve */
	} /* valve_slow_open_fuel_valve */

Uh oh!

Conversation

niekky commented Apr 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

ETSells commented Apr 19, 2026

Uh oh!

ETSells commented Jun 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants