Firmware Review

Critical Issues

1. Code executes before HAL_Init() - undefined behavior, potential hardware damage

File: Core/Src/main.c · Lines 102–103

// Line 102
HAL_GPIO_WritePin(RELAY_POWER_EN_GPIO_Port, RELAY_POWER_EN_Pin, GPIO_PIN_SET);
// Line 103
AdcReadVoltage(1, 64);
// ...
// Line 113 - HAL_Init() finally called here
HAL_Init();

Problem: Both calls happen inside USER CODE BEGIN 1, which runs before MPU_Config(), HAL_Init(), and SystemClock_Config().

• HAL_GPIO_WritePin at line 102 writes to a GPIO port whose peripheral clock has not been enabled yet. The result is either a no-op (write silently ignored) or, depending on silicon state after reset, an unintended level on the relay enable line before the relay driver hardware is in a known state.
• AdcReadVoltage at line 103 calls SelectAdcMux (GPIO writes, same problem) and then AdcReadRaw, which calls HAL_SPI_Receive(&hspi2, ...). hspi2 has not been initialized - MX_SPI2_Init() is called at line 137, well after this point. Calling HAL on an uninitialized handle is undefined behavior and will produce a garbage SPI transaction or a hardfault.

Fix: Move both calls to USER CODE BEGIN 2, after all MX_*_Init() calls have completed:

/* USER CODE BEGIN 2 */
HAL_TIM_Base_Start(&htim3);
HAL_TIM_Base_Start_IT(&htim4);

AdcADS1262Init();
ExpanderPCAL6524Init();

HAL_GPIO_WritePin(RELAY_POWER_EN_GPIO_Port, RELAY_POWER_EN_Pin, GPIO_PIN_SET);
AdcReadVoltage(1, 64); // warm-up read - SPI2 now initialized
/* USER CODE END 2 */

Also remove the dead code at line 165 (relay power-off after the unreachable while(1)):

// Line 165 - unreachable, remove:
HAL_GPIO_WritePin(RELAY_POWER_EN_GPIO_Port, RELAY_POWER_EN_Pin, GPIO_PIN_RESET);

---

2. Copy-paste bug: dac_chip written twice, dac_channel never assigned for DAC2

File: Core/Src/dac_ctrl.c · Lines 73–74

// Line 73
dac_chip = 2;
// Line 74 - typo: should be dac_channel, not dac_chip
dac_chip = forced_channel - DAC_CHANNEL_COUNT;

Problem: Line 74 overwrites dac_chip instead of assigning dac_channel. After this block:

• dac_channel is uninitialized (garbage stack value).
• dac_chip holds the channel offset (e.g. forced_channel - 8), not 2.

At line 79, dacVoltageTables[dac_chip][dac_channel] is then indexed with a wrong chip number (e.g. 3, 5) and a random channel. dacVoltageTables is a 2-element array, so any chip index other than 0 or 1 is an out-of-bounds access. At line 85, DacSetCode(dac_chip, ...) is called with this same corrupted chip value.

This affects every DacSetVoltage() call targeting DAC2 channels: DAC_VDD_FV_Ref, DAC_ON_FV_Ref, DAC_VIN_FV_Ref, DAC_VOUT_ELOAD_Ref, DAC_OS_VCLAMP_Ref. All voltage tests (V, R, I, T) use at least one of these - the entire test system produces wrong DAC outputs.

Fix:

// Line 74:
dac_channel = forced_channel - DAC_CHANNEL_COUNT;  // was dac_chip

---

3. LOAD_SWITCH used as a bare constant - condition is always true

File: Core/Src/ethernet.c · Line 107 Definition: Core/Inc/ethernet.h · Line 16

// ethernet.h line 16
#define LOAD_SWITCH 1

// ethernet.c line 107
if(command_type == GENERAL_CMD || command_type == UASIC_CMD || LOAD_SWITCH)

Problem: LOAD_SWITCH expands to the integer 1, not a comparison. The expression becomes:

if(command_type == 0 || command_type == 2 || 1)

The trailing || 1 makes the entire condition unconditionally true. Every received UDP packet is processed regardless of command_type. The device-selector check - which exists to filter packets not addressed to this device - is completely bypassed.

Fix:

// Line 107:
if (command_type == GENERAL_CMD || command_type == UASIC_CMD || command_type == LOAD_SWITCH)

---

4. Off-by-one in calibration interpolation - out-of-bounds read when input is near zero

Files:

• Core/Src/adc_ctrl.c · Lines 91, 95–96
• Core/Src/calibration.c · Lines 151, 155–156
• Core/Src/dac_ctrl.c · Lines 77, 81–82

All three loops have the same pattern:

// adc_ctrl.c lines 91–96
for (uint8_t calib_point = 0; calib_point < 14; calib_point++)
{
    if (adc_raw_result < calibrationData.adc_points_bit[measured_channel][calib_point])
    {
        resolution = (...[calib_point] - ...[calib_point - 1]) / ...;  // line 95
        intercept  = ...[calib_point - 1] - ...;                       // line 96
    }
}

// Same structure at calibration.c lines 151–156 (cb ADC)
// Same structure at dac_ctrl.c lines 77–82 (DAC voltage→code)

Problem: calib_point is uint8_t. When calib_point == 0 and the input value is below the first calibration point, calib_point - 1 wraps to 255 (unsigned underflow). The access ...[255] is far out of bounds - the tables have 14 or 20 entries. Whatever happens to be in memory at that offset is used as a calibration value, producing a completely wrong output voltage or DAC code.

All three calibration tables are initialized with index 0 holding the minimum value (0 bits / 0.0 V), so any measurement at or near 0 V triggers this on every read.

Fix - start the loop at index 1; index 0 is the lower sentinel:

// adc_ctrl.c line 91:
for (uint8_t calib_point = 1; calib_point < 14; calib_point++)

// calibration.c line 151:
for (uint8_t calib_point = 1; calib_point < 14; calib_point++)

// dac_ctrl.c line 77:
for (uint8_t calib_point = 1; calib_point < MAX_CALIBRATION_POINTS; calib_point++)

With the loop starting at 1, calib_point - 1 is always a valid index (0..N-2), and the existing index-0 entries (0 bits / 0.0 V) act as the correct lower bound for interpolation.

---

5. Payload extraction ignores offset - all messages after the first get wrong data

File: Core/Src/ethernet.c · Lines 96–98

// Line 77 - outer loop advances offset through multiple messages in one datagram
while (offset + MESSAGE_HEADER_SIZE <= length)
{
    // ...
    // Lines 96–98 - inner payload copy
    for (int byte = 0; byte < payload_length; byte++)
        payload[byte] = data[byte + MESSAGE_HEADER_SIZE];  // offset never used
    // ...
    offset += MESSAGE_HEADER_SIZE + payload_length;  // line 119
}

Problem: The copy always reads from data[byte + 8] - the fixed start of the buffer - regardless of where offset currently points. The first message is read correctly (offset is 0). Every subsequent message in the same datagram is given the payload bytes of the first message instead of its own. The corruption is silent: no error is returned, the wrong payload is parsed and dispatched.

Fix:

// Lines 96–98:
for (int byte = 0; byte < payload_length; byte++)
    payload[byte] = data[offset + MESSAGE_HEADER_SIZE + byte];

---

6. 32-bit shift into 64-bit relay bitmap - relays 32–49 unreachable, UB above bit 31

File: Core/Src/command_process.c · Lines 103–104

// Line 103
relayControlBitMap |= (1 << param_number[param_index]);
// Line 104
relayControlBitMap &= ~(1 << param_number[param_index]);

Problem: The literal 1 is int - a 32-bit signed type. Shifting a 32-bit value by 32 or more bits is undefined behavior in C (C11 §6.5.7). The assignment to uint64_t relayControlBitMap widens the result after the shift, but the shift itself has already produced UB. In practice on ARM Cortex-M7, shifts ≥ 32 with a 32-bit operand produce 0, so the expression silently does nothing for any relay index ≥ 32.

The relay bitmap is defined with 50 entries (bits 0–49). Bits 32–49 cover:

• DUT_ON_Pre_Buffered_RLY_CTRL (bit 32)
• DUT_VDD_PreBuffered_RLY_CTRL (bit 33)
• DUT_CAP_Pre_Buffered_RLY_CTRL (bit 34)
• DUT_VOUT_Sns_to_Pwr_CTRL (bit 36)
• DUT_RDSON_DifAmp_RLY_CTRL (bit 39)
• All MUX controls and SSRs (bits 40–49)

None of these can be set or cleared via CMD_SET_RELAYS. The same bug exists symmetrically in the ~(1 << ...) clear expression.

Fix:

// Line 103:
relayControlBitMap |=  (1ULL << param_number[param_index]);
// Line 104:
relayControlBitMap &= ~(1ULL << param_number[param_index]);

---

7. Memory leak in every test - params pointer destroyed before free(), heap exhausted in ~6 cycles

Files:

• Core/Src/tests_v.c · Lines 80, 164, 249, 334, 419
• Core/Src/tests_r.c · Lines 70, 146, 234
• Core/Src/tests_i.c · Lines 65, 135, 221
• Core/Src/tests_t.c · Lines 88, 179
• Core/Src/tests_general.c · Line 154
• STM32H743BITX_FLASH.ld · Line 40

Every TEST_CONFIG_STRUCT_RELEASE branch in every test looks like this (e.g. tests_v.c:80):

// free(test_config->params); // TODO:   ← intentionally skipped
memset(test_config, 0, sizeof(struct TestConfig));  // destroys params pointer without freeing it

Problem: memset zeroes the struct, overwriting test_config->params before the heap block it points to is freed. The allocation is permanently lost. This is called for every test in the queue when CMD_CLEAR_POOL is received.

Heap budget per pool-clear cycle at maximum queue depth:

• 60 tests × up to 22 floats × 4 bytes = 5 280 bytes leaked per cycle
• Linker script heap: _Min_Heap_Size = 0x200 = 512 bytes

The heap is exhausted after the first pool clear. On the next pool load, calloc returns NULL. NULL is not checked anywhere - the next write to test_config->params[x] is a NULL dereference, triggering a MemManage fault and entering the infinite loop in MemManage_Handler.

Fix - two changes required:

1. In every TEST_CONFIG_STRUCT_RELEASE block, free before zeroing:

// All 14 occurrences listed above - same pattern:
free(test_config->params);                          // must come first
memset(test_config, 0, sizeof(struct TestConfig));

1. Increase the heap in STM32H743BITX_FLASH.ld line 40:

// Before:
_Min_Heap_Size = 0x200;
// After:
_Min_Heap_Size = 0x4000;   /* 16 KB - covers max queue depth with margin */

---

8. Test_VddVil sweep loop never executes - test always returns 0.0

File: Core/Src/tests_v.c · Line 107

// TEST_CONFIG_STRUCT_INIT (lines 145–146) - start > stop for a descending sweep:
params[TESTS_V_START_VOLTAGE_PARAM_INDEX] = 2.7;   // start high
params[TESTS_V_STOP_VOLTAGE_PARAM_INDEX]  = 1.2;   // stop low

// RUN_TEST (line 107) - condition uses < instead of >:
for (float force_v = test_config->params[TESTS_V_START_VOLTAGE_PARAM_INDEX];
     force_v < test_config->params[TESTS_V_STOP_VOLTAGE_PARAM_INDEX];   // 2.7 < 1.2 → false
     force_v -= test_config->params[TESTS_V_STEP_VOLTAGE_PARAM_INDEX])

Problem: The initial condition 2.7 < 1.2 is false on the first evaluation. The loop body never executes. params[VDD_VIL_PARAM - 1] retains its calloc-initialized value of 0.0, which is returned as the measurement result regardless of the actual DUT behaviour. VDD_VIL is a permanently dead test.

Note that the step is correctly negative (force_v -= step); only the loop condition is wrong - it was copied from the ascending Test_VddVih without inverting the comparison.

Fix:

// Line 107 - change < to >:
for (float force_v = test_config->params[TESTS_V_START_VOLTAGE_PARAM_INDEX];
     force_v > test_config->params[TESTS_V_STOP_VOLTAGE_PARAM_INDEX];
     force_v -= test_config->params[TESTS_V_STEP_VOLTAGE_PARAM_INDEX])

---

9. Test_Rdis uses wrong constant for result slot; pass/fail check inconsistently mixes two constants

File: Core/Src/tests_r.c · Lines 25, 30 Definition: Core/Inc/tests_ctrl.h · Lines 40, 42

// tests_ctrl.h
#define RDIS_PARAM     9
#define RPULLDN_PARAM  9   // same numeric value

// tests_r.c line 25 - result stored under RPULLDN name, inside Test_Rdis:
test_config->params[RPULLDN_PARAM - 1] = v_vout / i_vout;

// tests_r.c line 30 - convergence check mixes both constants:
if ((test_config->params[RPULLDN_PARAM - 1] > test_config->params[TESTS_R_LOW_ERROR_PARAM_INDEX]) &&
    (test_config->params[RDIS_PARAM - 1]    < test_config->params[TESTS_R_HIGH_ERROR_PARAM_INDEX])) break;

Problem: Test_Rdis stores its result using RPULLDN_PARAM - 1 - the constant that belongs to a different test. The pass/fail check on line 30 then uses RDIS_PARAM - 1 for the upper-bound comparison.

The two constants happen to share the value 9 today, so both expressions resolve to index 8 and no actual wrong-slot access occurs at runtime. However:

1. The naming is actively misleading - reading Test_Rdis requires knowing that RPULLDN_PARAM == RDIS_PARAM to understand what is happening. A future change to either constant (which is inevitable as the tests evolve) silently breaks the measurement or the limit check without a compile error.
2. Line 24 has a separate bug in the same function - the measurement cycle count for the current sense ADC read is divided by gain and shunt values, yielding ~11 cycles instead of the intended 256:

// Line 24 - second argument evaluates to ~11, not 256:
float i_vout = AdcReadVoltage(ADC_VOUT_FV_I_sense_Signal,
    test_config->params[TESTS_R_ADC_MEASUREMENTS_PARAM_INDEX] / RDIS_OPAMP_GAIN / SHUNT_VOUT_RDIS);
//  256 / 21.8 / 1 ≈ 11.7 → truncated to uint8_t 11

The voltage and current are averaged over very different numbers of samples, introducing systematic measurement asymmetry.

Fix:

// Line 25 - use the correct constant:
test_config->params[RDIS_PARAM - 1] = v_vout / i_vout;

// Line 30 - use the same constant for both bounds:
if ((test_config->params[RDIS_PARAM - 1] > test_config->params[TESTS_R_LOW_ERROR_PARAM_INDEX]) &&
    (test_config->params[RDIS_PARAM - 1] < test_config->params[TESTS_R_HIGH_ERROR_PARAM_INDEX])) break;

// Line 24 - pass cycle count directly, do not divide by hardware constants:
float i_vout = AdcReadVoltage(ADC_VOUT_FV_I_sense_Signal,
    test_config->params[TESTS_R_ADC_MEASUREMENTS_PARAM_INDEX]);

---

10. ExpanderPCAL6524PinState reads [chip-1] but writes [chip]

File: Core/Src/calibration.c · Lines 207, 214

void ExpanderPCAL6524PinState(uint8_t chip, uint8_t port, uint8_t pin, expanderPinState state)
{
    uint8_t tx_byte = 0;
    tx_byte = expander_previous_state[chip - 1][port];  // line 207 - reads [chip-1]
    // ...
    expander_previous_state[chip][port] = tx_byte;       // line 214 - writes [chip]
    // ...
}

Problem: The read index and write index are inconsistent by one. expander_previous_state is declared as [EXPANDERS_COUNT][3] = [2][3], valid indices 0–1.

• chip == PCAL6524_CHIP1 (value 0): line 207 reads expander_previous_state[-1][port] - an out-of-bounds read into the memory before the array. Reads garbage, potentially the end of another global. The merged value is then written to the expander, driving pins to an unknown state.
• chip == PCAL6524_CHIP2 (value 1): line 207 reads index 0 (CHIP1's state) instead of index 1 (its own state). The read-modify-write cycle uses the wrong chip's state as the base, so previously set pins on CHIP2 are silently cleared on every call.

Fix - use the same index for both read and write:

// Line 207:
tx_byte = expander_previous_state[chip][port];   // was chip - 1

// Line 214 unchanged:
expander_previous_state[chip][port] = tx_byte;

---

11. VLA on stack in UDP receive path overflows the 1 KB stack

Files: Core/Src/command_process.c · Lines 14, 17–19, 28 Linker script: STM32H743BITX_FLASH.ld · Line 41

// ProcessService() stack frame - all VLAs, allocated on entry:
uint8_t output_payload[servise_payload_length];   // line 14
float   fOutput[params_count];                    // line 17
float   finalOutput[test_revision / 4];           // line 18
uint8_t finalOutput_Payload[test_revision / 4 * 5]; // line 19

// Inside CMD_POOL_START branch:
uint8_t output_results[resultsQueueLength * MIN_PAYLOAD_LENGHT]; // line 28

Problem: output_results alone can reach 540 × 5 = 2 700 bytes (60 tests × 9 results × 5 bytes). The entire stack budget is _Min_Stack_Size = 0x400 = 1 024 bytes. The VLA at line 28 overflows the stack on the first pool-start with more than ~200 result bytes queued. SendResponce (called immediately after) then allocates another uint8_t responce[8 + responce_length] on the same overflowed stack.

Stack overflow on Cortex-M7 without an MPU stack guard corrupts whatever is below the stack in RAM - in this linker layout, the .bss section containing all global state.

Fix - two changes required:

1. Replace the output_results VLA with a static buffer sized to the known maximum:

// Line 28 - replace VLA with static:
static uint8_t output_results[TESTS_EXECUTION_QUEUE_MAX_SIZE * OS_NUM_RESULTS * MIN_PAYLOAD_LENGHT];
// = 60 * 9 * 5 = 2700 bytes, allocated once in .bss, not on the stack

1. Increase the stack for the remaining call-chain depth:

// STM32H743BITX_FLASH.ld line 41:
_Min_Stack_Size = 0x1000;   /* 4 KB minimum */

---

12. testCounter never reset on CMD_CLEAR_POOL

File: Core/Src/command_process.c · Lines 3–4, 54, 269–270

// Lines 3–4 - file globals:
uint resultsQueueLength = 0;
uint testCounter = 0;        // TODO: ??

// CMD_CLEAR_POOL handler (lines 48–54) - resets length but not testCounter:
testsExecutionQueueLength = 0;
// resultsQueueLength = 0;  ← also missing
// testCounter = 0;         ← missing

// ProcessTest case 1 - adds to pool (lines 268–270):
testsExecutionQueueLength = test_queue_length + 1;
resultsQueueLength += TestsExecutionQueue[testCounter].num_of_results;  // testCounter, not test_queue_length
testCounter++;

Problem: testCounter is used to index TestsExecutionQueue for the num_of_results lookup on line 269, but it is never reset when the pool is cleared. After the first pool-clear-reload cycle, testCounter is at 60 (or wherever it stopped), so TestsExecutionQueue[testCounter] accesses entries that were memset to zero by TEST_CONFIG_STRUCT_RELEASE. num_of_results reads as 0, resultsQueueLength is never incremented, and the response buffer at line 28 is zero-sized.

Additionally, resultsQueueLength itself is not reset in CMD_CLEAR_POOL (line 54 only resets testsExecutionQueueLength), so it accumulates indefinitely across pool cycles.

The correct index to use is test_queue_length (the local snapshot taken at line 243), which already points to the freshly initialized queue slot.

Fix:

// CMD_CLEAR_POOL block - add the two missing resets after line 54:
testsExecutionQueueLength = 0;
resultsQueueLength = 0;   // add
testCounter = 0;          // add

// Line 269 - use test_queue_length, not testCounter:
resultsQueueLength += TestsExecutionQueue[test_queue_length].num_of_results;
// Line 270 - testCounter no longer needed; remove:
// testCounter++;

---

13. TESTS_T_RELAYS contains a raw GPIO pin mask instead of a relay bit index - wrong relays energized during timing tests

File: Core/Inc/relay_ctrl.h · Line 61 Definition: Core/Inc/main.h · Line 192 HAL header: Drivers/STM32H7xx_HAL_Driver/Inc/stm32h7xx_hal_gpio.h · Line 98

// relay_ctrl.h line 61:
#define TESTS_T_RELAYS (... | ON_For_TIM_RLY_CTRL | DUT_VOUT_For_TIM_Pin )
//                                                   ^^^^^^^^^^^^^^^^^^^^ wrong symbol

// main.h line 192 - HAL-generated GPIO pin mask:
#define DUT_VOUT_For_TIM_Pin   GPIO_PIN_13   // = (uint16_t)0x2000 = 8192

// The correct relay control symbol (relay_ctrl.h line 14):
#define DUT_VOUT_For_TIM_RLY_CTRL  (1ULL << 14)  // = 0x0000000000004000

Problem: DUT_VOUT_For_TIM_Pin is the HAL GPIO pin bitmask (0x2000 = 8192), not a relay bitmap constant. When ORed into the 64-bit relay bitmap and passed to SetRelay(), the value 0x2000 sets bit 13 (VOUT_FV_RLY_CTRL) instead of the intended bit 14 (DUT_VOUT_For_TIM_RLY_CTRL).

Consequence during Test_Ton and Test_Toff:

• VOUT_FV_RLY is closed (bit 13) - connects VOUT to the force/measure line.
• DUT_VOUT_For_TIM is left open (bit 14) - the comparator input for timing capture is disconnected.

The timing measurement sees no signal edge and timerCapturedValue[] is never updated, so T_ON and T_OFF results are stale or zero. The unintended VOUT_FV_RLY closure may also create a signal path conflict with other relays in the bitmap, potentially short-circuiting the DUT VOUT rail.

Fix:

// relay_ctrl.h line 61 - replace the Pin symbol with the RLY_CTRL symbol:
#define TESTS_T_RELAYS (VIN_FV_1A_RLY_CTRL | ON_FV_RLY_CTRL | ON_FV_MUX_CTRL | \
                        VDD_FV_MUX_CTRL | CAP_CAP_RLY_CTRL | VOUT_FV_1A_RLY_CTRL | \
                        R_LOAD_1A_RLY_CTRL | ON_For_TIM_RLY_CTRL | DUT_VOUT_For_TIM_RLY_CTRL)
//                                                                  ^^^^^^^^^^^^^^^^^^^^^^^^ was DUT_VOUT_For_TIM_Pin

---

Important Issues

1. Calibration data is zero at power-up - DAC outputs an uncontrolled rail voltage before host sends CMD_READ_EEPROM_STRUCT

Files: Core/Src/command_process.c · Lines 193–202 Core/Src/main.c · Lines 144–154 Core/Src/dac_ctrl.c · Lines 77–88 Core/Src/adc_ctrl.c · Lines 91–100

calibrationData is a BSS global (zero-initialized on reset). The only code path that populates it is the CMD_READ_EEPROM_STRUCT handler (command_process.c:195–201), which requires the host to explicitly send that command after boot.

Two failure modes before the host loads calibration:

1. ADC returns 0.0 for every measurement. adc_ctrl.c:93 checks adc_raw_result < adc_points_bit[ch][0]. With adc_points_bit all zero, raw_result < 0 is never true (raw values are unsigned), so the loop exits without setting out_voltage. The variable holds its uninitialized stack value or zero. Every AdcReadVoltage() call silently returns wrong data.

2. DAC drives to a rail - hardware damage risk. dac_ctrl.c:79 checks voltage < dacVoltageTables[dac][ch][0]. With the table zeroed, any positive voltage argument satisfies voltage < 0 - false - so the condition is never met, DacSetCode is never called, and the DAC retains whatever output it had from DacsHardReset(). More dangerously, if the zero table causes the loop to match at calib_point == 0 (depending on exact zero-point behaviour after C-5 is fixed), the interpolation computes:

   resolution = (0 - 0) / (0 - 0)  →  0 / 0  →  NaN
   set_bits   = (voltage - NaN) / NaN  →  NaN
   DacSetCode(..., (uint16_t)NaN)   →  0 or 65535
   

   The DAC is driven to 0 V or full-scale (~6 V) on whichever rail the test next calls DacSetVoltage() on - with no warning, before any DUT is characterized.

Fix - load calibration during startup, before any peripheral output is enabled:

// Core/Src/main.c, USER CODE BEGIN 2, after ExpanderPCAL6524Init():

/* Load calibration data from EEPROM before any DAC/relay output */
for (uint16_t page = 0; page < EEPROM_64K_PAGES_COUNT; page++)
{
    EepromControl(EEPROM_AT24CS64_ADDRESS, page * EEPROM_64K_PAGE_SIZE,
                  eeprom64kMem[page], EEPROM_64K_PAGE_SIZE, DUT, READ);
    HAL_Delay(2);
}
memcpy(&calibrationData, &eeprom64kMem[0], EEPROM_64K_PAGES_COUNT * EEPROM_64K_PAGE_SIZE);

This mirrors exactly what CMD_READ_EEPROM_STRUCT already does at command_process.c:195–201 - extract that block into a shared function rather than duplicating it.

---

2. calloc return value never checked - NULL dereference on heap exhaustion

Files:

• Core/Src/tests_v.c · Lines 54, 138, 222, 307, 392
• Core/Src/tests_r.c · Lines 45, 121, 206
• Core/Src/tests_i.c · Lines 40, 110, 189
• Core/Src/tests_t.c · Lines 55, 146
• Core/Src/tests_general.c · Line 119

Every TEST_CONFIG_STRUCT_INIT branch assigns the calloc result directly without checking it (example from tests_v.c:54):

test_config->params = calloc(VDD_VIH_PARAM, sizeof(float));
// no NULL check - next line uses test_config->params[x] unconditionally

Problem: When the heap is exhausted (which Critical Issue #7 guarantees within a few pool-clear cycles), calloc returns NULL. The immediately following default-value assignments write through a NULL pointer:

test_config->params[TESTS_V_NUMBER_PARAM_INDEX] = 20;  // NULL[0] = 20 → MemManage fault

On Cortex-M7 this triggers a MemManage fault, entering the infinite loop in MemManage_Handler with no diagnostic output and no recovery. The fault is silent from the host's perspective - the device stops responding.

Fix - check after every calloc call (same pattern for all 14 occurrences):

test_config->params = calloc(VDD_VIH_PARAM, sizeof(float));
if (test_config->params == NULL)
{
    // Send NACK so the host knows the command was rejected
    SendResponce(RESP_NACK, 0, NULL, test_config->test_id);
    return 0;
}

---

3. Param index from UDP payload used without bounds check - heap out-of-bounds write

Files:

• Core/Src/tests_v.c · Lines 71–72, 155–156, 240–241, 325–326, 410–411
• Core/Src/tests_r.c · Lines 61–62, 137–138, 225–226
• Core/Src/tests_i.c · Lines 56–57, 126–127, 212–213
• Core/Src/tests_t.c · Lines 79–80, 170–171
• Core/Src/tests_general.c · Lines 144–145

Every test's payload-parsing loop has the same pattern (example tests_v.c:71–72):

uint8_t temp_param_number = data[byte * MIN_PAYLOAD_LENGHT];   // index from network
memcpy(&test_config->params[temp_param_number], &data[1 + byte * MIN_PAYLOAD_LENGHT], sizeof(float));

Problem: temp_param_number is a raw uint8_t from the received UDP payload, range 0–255. It is used directly as an index into test_config->params, which was allocated with at most IOUT_PARAM = 19 elements. Any packet with a param index ≥ the allocated size writes a float 4 bytes past the end of the heap allocation. At temp_param_number = 255, the write lands 1020 bytes beyond the allocation, overwriting heap metadata or adjacent allocations and causing silent data corruption or a subsequent heap fault.

Fix - validate before writing (same pattern for all 20 occurrences):

uint8_t temp_param_number = data[byte * MIN_PAYLOAD_LENGHT];
if (temp_param_number < test_config->num_of_params)
    memcpy(&test_config->params[temp_param_number], &data[1 + byte * MIN_PAYLOAD_LENGHT], sizeof(float));

---

Improvements

1. Every test function repeats the same INIT/RELEASE/GET_ID skeleton - extract into a dispatch macro

Files: All tests_*.c - every test function (~14 functions, 80–170 lines each)

Every test function has four branches, three of which are identical boilerplate across all tests:

float Test_VddVih(TestConfig *tc, OperationMode mode, uint8_t *data, uint16_t len)
{
    if (tc == NULL) return VDD_VIH_TEST_ID;            // GET_TEST_ID

    if (mode == TEST_CONFIG_STRUCT_RELEASE) { ... }    // same in every test
    if (mode == TEST_CONFIG_STRUCT_INIT)    { ... }    // same structure, different constants
    if (mode == RUN_TEST)                   { ... }    // only unique part
}

Copy-paste bugs in Critical Issues #7, #8, and Important Issues #2, #3 all live in the boilerplate sections. Every future test will reproduce them.

Approach - split each test into a dispatch macro + a defaults function + a run function:

1. Add to tests_ctrl.h - a macro that handles RELEASE, INIT, GET_ID:

#define TEST_DISPATCH(test_id, num_params, num_results, defaults_fn, self_ptr)          \
    if (test_config == NULL) return (float)(test_id);                                   \
    if (operation_mode == TEST_CONFIG_STRUCT_RELEASE) {                                 \
        free(test_config->params);                                                      \
        memset(test_config, 0, sizeof(struct TestConfig));                              \
        return 0;                                                                       \
    }                                                                                   \
    if (operation_mode == TEST_CONFIG_STRUCT_INIT) {                                    \
        test_config->test_id        = (test_id);                                        \
        test_config->num_of_results = (num_results);                                    \
        test_config->num_of_params  = (num_params);                                     \
        test_config->params = calloc((num_params), sizeof(float));                      \
        if (!test_config->params) { SendResponce(RESP_NACK,0,NULL,(test_id)); return 0; } \
        (defaults_fn)(test_config);                                                     \
        for (uint16_t _b = 0; _b < data_length / MIN_PAYLOAD_LENGHT; _b++) {           \
            uint8_t _idx = data[_b * MIN_PAYLOAD_LENGHT];                               \
            if (_idx < (num_params))                                                    \
                memcpy(&test_config->params[_idx],                                      \
                       &data[1 + _b * MIN_PAYLOAD_LENGHT], sizeof(float));              \
        }                                                                               \
        test_config->TestFuncPtr = (self_ptr);                                          \
        return 0;                                                                       \
    }

2. Each test becomes ~20 lines instead of ~120:

// tests_v.c
static void VddVih_Defaults(TestConfig *tc)
{
    tc->params[TESTS_V_START_VOLTAGE_PARAM_INDEX] = 1.2f;
    tc->params[TESTS_V_STOP_VOLTAGE_PARAM_INDEX]  = 2.7f;
    tc->params[TESTS_V_STEP_VOLTAGE_PARAM_INDEX]  = 0.001f;
    // ... remaining defaults ...
}

float Test_VddVih(TestConfig *test_config, OperationMode operation_mode,
                  uint8_t *data, uint16_t data_length)
{
    TEST_DISPATCH(VDD_VIH_TEST_ID, VDD_VIH_PARAM, TESTS_V_NUM_RESULTS,
                  VddVih_Defaults, Test_VddVih);

    // Only RUN_TEST reaches here:
    DacsHardReset();
    SetLoad(...);
    // ... hardware logic only ...
    return 0;
}

The macro centralises the three fixes that currently need to be applied 14 times each: free before memset (Critical #7), calloc NULL check (Important #2), and param bounds check (Important #3).

---

2. Magic constants with no explanation - document or name with units

Files:

• Core/Src/adc_ctrl.c · Line 115
• Core/Inc/calibration.h · Line 85
• Core/Inc/tests_ctrl.h · Line 134
• Core/Src/command_process.c · Line 56

// adc_ctrl.c:115 - what is 3.16? hardware gain? resistor divider ratio?
return ((float)code / ((float)ADC_RESOLUTION / 2)) * ADC_VREF * calculation_polarity * 3.16;

// calibration.h:85 - typo in name ("DEVIRED" → "DERIVED"); 0.33 is a resistor divider ratio
// but no schematic reference or formula given
#define ANI6_INPUT_DEVIRED  0.33

// tests_ctrl.h:134 - 295.12 is presumably an op-amp gain or resistor network ratio,
// but the formula is not shown
#define VIN_VOUT_DROP_GAIN  295.12

// command_process.c:56 - 0x600 is an undocumented upper bound on the service command range;
// the TODO comment confirms nobody knows what it means
else if (group_id >= CMD_TASK && group_id < 0x600) // TODO: add description of the 0x600

Problem: Each constant encodes a hardware-derived value (op-amp gain, resistor divider, ADC front-end scaling). Without the formula or schematic reference, it is impossible to verify correctness, recalculate for a board revision, or detect a typo. 3.16 in particular has no name at all - it cannot be found by grepping and cannot be updated without understanding the entire signal chain.

Fix - replace each with a named constant that shows its derivation:

// adc_ctrl.c - replace 3.16 with a named ratio; fill in the actual formula:
#define ADC_INPUT_DIVIDER_GAIN   3.16f   /* R_top=?? R_bot=?? → (R_top+R_bot)/R_bot */
// ...
return ((float)code / ((float)ADC_RESOLUTION / 2)) * ADC_VREF * calculation_polarity * ADC_INPUT_DIVIDER_GAIN;

// calibration.h - fix typo in name, add formula comment:
#define ANI6_INPUT_DIVIDER_RATIO  0.33f  /* R_bot/(R_top+R_bot): R_top=2k, R_bot=1k */

// tests_ctrl.h - add formula comment:
#define VIN_VOUT_DROP_GAIN  295.12f  /* diff-amp gain: 1 + (R_f/R_in) × ... */

// command_process.h - add a named constant and explain the range:
#define CMD_SERVICE_RANGE_END  0x600  /* upper bound of direct hardware command group IDs */
// command_process.c:56:
else if (group_id >= CMD_TASK && group_id < CMD_SERVICE_RANGE_END)

---

3. DAC_CHANNEL_COUNT and MAX_DAC_CHANNELS are the same value defined in two headers

Files:

• Core/Inc/dac_ctrl.h · Line 9
• Core/Inc/main.h · Line 267

// dac_ctrl.h:9
#define DAC_CHANNEL_COUNT   8

// main.h:267  (CubeMX-generated)
#define MAX_DAC_CHANNELS    8

Both constants are 8 and are used interchangeably across the codebase - DAC_CHANNEL_COUNT drives the channel-routing logic in dac_ctrl.c:66, 74, MAX_DAC_CHANNELS sizes the calibration arrays in calibration.h:32–36 and is used in command_process.c:89 and dac_ctrl.c:96. If either is changed independently, the calibration table dimensions and the routing boundary silently diverge.

Fix - remove MAX_DAC_CHANNELS from main.h and replace all uses with DAC_CHANNEL_COUNT:

// main.h:267 - remove:
// #define MAX_DAC_CHANNELS    8

// calibration.h:32–36 - replace MAX_DAC_CHANNELS with DAC_CHANNEL_COUNT:
uint32_t dac1_points_bit[DAC_CHANNEL_COUNT][MAX_CALIBRATION_POINTS];
// ... (4 array declarations)

// command_process.c:89 - replace:
DacSetVoltage(...param_number[param_index] + DAC_CHANNEL_COUNT, ...);

// dac_ctrl.c:96 - replace:
for (uint8_t dac_channel = 0; dac_channel < DAC_CHANNEL_COUNT; dac_channel++)

MAX_DAC_CHANNELS lives in a CubeMX-generated header and could be overwritten on next .ioc regeneration, making DAC_CHANNEL_COUNT in dac_ctrl.h the safer canonical definition.

---

4. DacsHardReset() has no delay - reset pulse width may be below the DAC's minimum spec

File: Core/Src/dac_ctrl.c · Lines 31–34

void DacsHardReset()
{
    DAC1_HARD_RESET_LOW();   // line 31
    DAC2_HARD_RESET_LOW();   // line 32
    DAC1_HARD_RESET_HIGH();  // line 33 - asserted for ~10 ns at 200 MHz
    DAC2_HARD_RESET_HIGH();  // line 34
}

Problem: Each HARD_RESET_LOW/HIGH macro is a single BSRR register write. At 200 MHz with no pipeline stalls, consecutive writes to the same peripheral bus are separated by ~5–10 ns. The DAC8568 (the likely target, given the 16-bit SPI protocol used in DacSetCode) specifies a minimum RST pulse width of 20 ns. The current pulse width is at best marginal and at worst below the minimum, leaving the DAC in an undefined state after reset. The function is called at the start of every test.

Fix - add a brief delay between assert and de-assert:

void DacsHardReset()
{
    DAC1_HARD_RESET_LOW();
    DAC2_HARD_RESET_LOW();
    DelayUs(1);              // 1 µs >> 20 ns minimum reset pulse width
    DAC1_HARD_RESET_HIGH();
    DAC2_HARD_RESET_HIGH();
    DelayUs(1);              // settling time before first SPI transaction
}

---

5. TempSensor() discards its result - dead code

File: Core/Src/calibration.c · Lines 255–267

void TempSensor()
{
    // ... I2C read ...
    float temperature_value = raw_data * 0.0625;  // line 266 - local, never returned or stored
}                                                  // temperature_value goes out of scope here

Problem: temperature_value is a local variable. It is computed and immediately discarded when the function returns. The I2C transaction runs (consuming ~50 µs at 400 kHz), the conversion is performed, and the result is lost. The function is not called anywhere in the codebase. A compiler with warnings enabled will flag this as an unused variable.

Fix - either expose the result or remove the function:

// Option A - return the value:
float TempSensor(void)
{
    // ...
    return raw_data * 0.0625f;
}

// Option B - remove entirely if temperature monitoring is not yet implemented.
// Also remove the declaration from calibration.h:166.

---

6. .gitignore is nearly empty - IDE files, build artifacts, and secrets are untracked

File: .gitignore · Line 1

Current content:

/Debug

Problem: Only the Debug/ build output directory is ignored. The repository currently tracks everything else, including machine-specific and generated files that should never be committed:

• .settings/ - STM32CubeIDE workspace prefs (language.settings.xml, org.eclipse.core.resources.prefs, stm32cubeide.project.prefs) contain absolute machine paths and auto-generated indexer data. They change on every developer machine and on every clean rebuild.
• .vscode/settings.json - editor-local config, should not be shared.
• .cproject, .project - Eclipse CDT project files are partially generated by CubeMX and partially IDE-managed. Committing them causes constant spurious diffs when the IDE auto-updates paths.
• *.launch - the debug launch file (AMT_STM_LS.launch) embeds absolute ST-LINK server paths that differ per machine.
• Build artifacts if a Release/ config is ever added: *.elf, *.hex, *.bin, *.map, *.lst, *.su.

Fix - replace .gitignore with:

# Build output
/Debug/
/Release/

# STM32CubeIDE / Eclipse
.settings/
.cproject
.project
*.launch

# VS Code
.vscode/

# Build artifacts (if generated outside Debug/)
*.elf
*.hex
*.bin
*.map
*.lst
*.su

---

7. calibration.c/h is a god-module - five unrelated drivers in one file

Files: Core/Src/calibration.c, Core/Inc/calibration.h

calibration.h currently declares and calibration.c implements all of the following, with no logical connection between them beyond "they were convenient to put somewhere":

As a result, calibration.h is included by almost everything (adc_ctrl.h, dac_ctrl.h, tests_ctrl.h) pulling all six unrelated drivers into every translation unit. A change to the I/O expander forces a rebuild of every test file.

Recommended split:

Core/Inc/calibration.h      - dumpEEPROM struct, EepromControl, extern calibrationData
Core/Inc/cb_adc_ctrl.h      - AdcADS1262* functions (calibration board ADC)
Core/Inc/io_expander.h      - ExpanderPCAL6524*, AdcADS1262CsControl, SetMux*

dac_ctrl.h and adc_ctrl.h then only need to include calibration.h; test files never need to know about the I/O expander.

---

8. Naming conventions and recurring API typos

Files: Throughout Core/Inc/ and Core/Src/

---

Casing conventions

The codebase has an established convention that is mostly followed but has specific violations:

---

Macro / constant casing violations

adc_ctrl.h - buffered channel names

adc_ctrl.h - signal names

relay_ctrl.h - relay control bitmasks

---

Recurring typos in public API names

These are baked into public function signatures and shared defines. Fix them in a single coordinated rename before the API stabilizes - changing them after external tooling or host software has adopted them is much more painful.

MIN_PAYLOAD_LENGHT is the highest-impact typo - it appears in every test file's payload parsing loop. Fix it with a single project-wide search-and-replace, then add a spell-checker to the editor config (e.g. cSpell for VS Code) to prevent recurrence.