Review: Fixed-Point Pressure Filter Written by an AI Assistant

Your controls team asked an AI coding assistant to filter and scale the outlet pressure reading on an industrial air compressor. It opened this pull request. You are the reviewer, and nothing merges without your approval.

The assistant's description:

Moving-average filter and conversion to pascals for the 0 to 1 MPa outlet transducer. Pure integer maths, so no FPU is needed, and the running sum keeps pf_push() O(1) inside the ISR. Adds pf_over_limit() for the relief valve and a smoothed kPa value for the front panel.

Where it runs: Cortex-M0+ (32-bit int, no FPU) built with GCC at -O2 -Wall. 12-bit ADC sampled at 2 kHz. Calibration gives zero_counts of about 410 and pa_per_count_q16 = 17786470 (271.4 Pa per count). Normal outlet pressure is 500,000 to 700,000 Pa and the relief set point is 800,000 Pa. The control task starts calling pf_over_limit() straight after pf_init().

Your task

• Flag every problem you would block the merge for. Click a line number to comment, and Shift-click to cover a range.
• For each one, choose the issue type and severity, and explain what fails and how to fix it.
• You are scored on the problems you catch, the right issue type and a clear explanation. Missing a critical problem fails the round.

Aim for 20 minutes, as in a real review round.

What is the largest value pf.sum has to hold, and what type holds it?

Multiply a mid-scale counts value by pa_per_count on paper. Does it fit in 32 bits?

pf_pascals() returns a signed value and limit_pa is unsigned. What does C do when the reading is slightly negative?

What does the average look like during the first 32 samples after pf_init()?

Feed the display a steady 500 kPa from 0. Where does disp_kpa settle?

At normal operating pressure this code is wrong twice over: the running sum wraps and the scaling multiply overflows, so the relief valve logic sees nonsense. Either one blocks the merge on a safety-related input.

1. Running sum wraps (line 17, critical). 32 samples of up to 4095 sum to 131,040, which does not fit in uint16_t. Once the average passes 2048 counts (about 445 kPa here, below normal operating pressure) the sum wraps modulo 65,536: a steady 3000 counts averages to 952. Unsigned wrap is defined behaviour, so no warning appears. Make sum a uint32_t.
2. Scaling multiply overflows int32 (line 44, critical). counts * pf.pa_per_count is an int32_t multiply. With a Q16.16 scale of about 17.8 million, any reading above about 120 counts (33 kPa) exceeds INT32_MAX. Signed overflow is undefined behaviour and in practice yields garbage. Widen first: ((int64_t)counts * pf.pa_per_count) >> 16; a 64-bit multiply at 100 Hz is cheap even on an M0+.
3. Signed and unsigned comparison (line 50, major). In pf_pascals() > limit_pa the signed result is converted to uint32_t, so -50 Pa becomes 4,294,967,246 and the valve opens whenever the reading dips below zero (vented outlet, ADC noise, or start-up). -Wall alone does not warn; -Wextra does. Treat negative readings explicitly, or compare as int64_t.
4. Average is wrong until the window fills (line 41, minor). After pf_init() the window holds 32 zeros but the sum is still divided by 32, so the first 31 averages read low and below zero_counts, which is negative pressure. Combined with the comparison bug, the relief valve opens at every boot. Pre-fill the window with the first sample, or divide by the number of samples seen so far.
5. Display filter never reaches the value (line 62, minor). (4096 * diff) >> 15 is diff / 8 rounded towards minus infinity. Rising by 1 to 7 kPa adds nothing, so the display settles up to 7 kPa low (493 for a steady 500), while falling values settle exactly. Keep the state with extra fractional bits (for example kPa × 256 in an int32_t) and shift down only for display.