src/main/drivers/barometer/barometer_bmp580.c (2)

227-240: VLA in embedded context; consider a fixed-size buffer.

uint8_t buf[length + 1] is a variable-length array. While the callers currently pass small lengths (≤6), VLAs are generally avoided in embedded firmware due to unpredictable stack usage and are undefined behavior in some C standards when the size is zero. A fixed-size local buffer based on BMP580_DATA_FRAME_SIZE + 1 would be safer.

Proposed fix

 static bool bmp580ReadRegisterBuffer(const extDevice_t *dev, uint8_t reg, uint8_t *data, uint8_t length)
 {
     if (dev->bus->busType == BUS_TYPE_SPI) {
-        // For SPI: first byte is dummy on BMP5xx read
-        uint8_t buf[length + 1];
-        bool ret = busReadRegisterBuffer(dev, reg, buf, length + 1);
+        // For SPI: first byte is dummy on BMP5xx read
+        uint8_t buf[BMP580_DATA_FRAME_SIZE + 1 + 1]; // max data + SPI dummy + chip ID read
+        if (length + 1 > (int)sizeof(buf)) {
+            return false;
+        }
+        bool ret = busReadRegisterBuffer(dev, reg, buf, length + 1);
         if (ret) {
             memcpy(data, buf + 1, length);
         }

---

203-216: Static global state limits driver to a single instance and leaves debug variables in production.

The static globals (bmp580_up, bmp580_ut, bmp580_chip_id, sensor_data, bmp580_last_raw_temp, bmp580_last_raw_press) are typical for Betaflight barometer drivers (single-instance design). However, bmp580_last_raw_temp and bmp580_last_raw_press (lines 215-216) are only assigned but never read — they are dead code unless a debugger is attached.

Suggested: guard debug variables

+#ifdef DEBUG
 // Debug: last raw values for troubleshooting
 static uint32_t bmp580_last_raw_temp = 0;
 static uint32_t bmp580_last_raw_press = 0;
+#endif

And in bmp580GetUP:

+#ifdef DEBUG
     // Store for debug
     bmp580_last_raw_temp = bmp580_ut;
     bmp580_last_raw_press = bmp580_up;
+#endif

src/main/drivers/barometer/barometer_bmp580.c (2)

228-241: Replace VLA with a fixed-size stack buffer.

uint8_t buf[length + 1] (line 232) is a variable-length array. While length is always small here (1 or 6), VLAs are risky on resource-constrained MCUs and some compilers/configurations reject them. Use a fixed-size buffer based on the maximum expected read.

Proposed fix

 static bool bmp580ReadRegisterBuffer(const extDevice_t *dev, uint8_t reg, uint8_t *data, uint8_t length)
 {
     if (dev->bus->busType == BUS_TYPE_SPI) {
-        // For SPI: first byte is dummy on BMP5xx read
-        uint8_t buf[length + 1];
+        // For SPI: first byte is dummy on BMP5xx read
+        uint8_t buf[BMP580_DATA_FRAME_SIZE + 1];  // max read size + SPI dummy byte
+        if (length > BMP580_DATA_FRAME_SIZE) {
+            return false;
+        }
         bool ret = busReadRegisterBuffer(dev, reg, buf, length + 1);
         if (ret) {
             memcpy(data, buf + 1, length);
         }
         return ret;

---

215-217: Dead stores: debug variables are written but never read.

bmp580_last_raw_temp and bmp580_last_raw_press are set in bmp580GetUP (lines 449–450) but never consumed. They aren't guarded by #ifdef DEBUG either. Consider removing them or wrapping in #ifdef DEBUG to avoid dead writes in release builds.

src/main/drivers/barometer/barometer_bmp580.c (4)

243-256: VLA on constrained stack — prefer a fixed-size buffer.

uint8_t buf[length + 1] is a variable-length array. While callers in this file only pass small lengths (1 or 6), VLAs are fragile in embedded contexts and prohibited by MISRA C. Consider a fixed-size stack buffer based on the maximum expected read (data frame + 1 SPI dummy byte).

Proposed fix

 static bool bmp580ReadRegisterBuffer(const extDevice_t *dev, uint8_t reg, uint8_t *data, uint8_t length)
 {
     if (dev->bus->busType == BUS_TYPE_SPI) {
-        // For SPI: first byte is dummy on BMP5xx read
-        uint8_t buf[length + 1];
-        bool ret = busReadRegisterBuffer(dev, reg, buf, length + 1);
+        // For SPI: first byte is dummy on BMP5xx read
+        uint8_t buf[BMP580_DATA_FRAME_SIZE + 1 + 1]; // max read size + SPI dummy + margin
+        if (length + 1 > (int)sizeof(buf)) {
+            return false;
+        }
+        bool ret = busReadRegisterBuffer(dev, reg, buf, length + 1);
         if (ret) {
             memcpy(data, buf + 1, length);
         }

---

230-232: Remove unused debug variables.

bmp580_last_raw_temp and bmp580_last_raw_press are written (lines 472–473) but never read anywhere. They consume static RAM for no production benefit. Based on learnings, debug output was explicitly removed per Betaflight standards.

Proposed fix

-// Debug: last raw values for troubleshooting
-static uint32_t bmp580_last_raw_temp = 0;
-static uint32_t bmp580_last_raw_press = 0;

And remove the assignments at lines 472–473:

-    // Store for debug
-    bmp580_last_raw_temp = bmp580_ut;
-    bmp580_last_raw_press = bmp580_up;

---

375-378: Ambiguous IIR coefficient comment — clarify register value vs. filter order.

The comment says "coefficient 4" but the code uses BMP580_IIR_COEF_15, which per the defines means register value 0x04 mapping to a filter coefficient of 15. The PR description also says "IIR filter coefficient 4" which adds to the confusion. Consider clarifying:

-    // Configure IIR filter coefficient 4 (gives coefficient ~15) for both pressure and temperature
+    // Configure IIR filter: register value 4 selects coefficient 15 for both pressure and temperature

---

220-228: Consider making bmp580_chip_id local to bmp580Detect.

bmp580_chip_id is only used within bmp580Detect. Keeping it as a file-scoped static wastes a byte of static RAM after detection completes. The same applies to the sensor_data pointer setup at line 329 which duplicates the SPI dummy-byte handling already done inside bmp580ReadRegisterBuffer.

src/main/drivers/barometer/barometer_bmp580.c (3)

254-257: VLA in embedded context.

uint8_t buf[length + 1] is a variable-length array on the stack. While length is always small in practice (1 or 6), VLAs are generally discouraged in embedded firmware due to unpredictable stack usage and potential issues with some compilers. Consider using a fixed-size buffer based on BMP580_DATA_FRAME_SIZE + 1.

Proposed fix

 static bool bmp580ReadRegisterBuffer(const extDevice_t *dev, uint8_t reg, uint8_t *data, uint8_t length)
 {
     if (dev->bus->busType == BUS_TYPE_SPI) {
         // For SPI: first byte is dummy on BMP5xx read
-        uint8_t buf[length + 1];
-        bool ret = busReadRegisterBuffer(dev, reg, buf, length + 1);
+        uint8_t buf[BMP580_DATA_FRAME_SIZE + 1 + 1]; // Max read size + SPI dummy
+        bool ret = busReadRegisterBuffer(dev, reg, buf, MIN(length + 1, (uint8_t)sizeof(buf)));
         if (ret) {
             memcpy(data, buf + 1, length);
         }

---

398-401: Misleading comment — "coefficient 4" is the register value, not the filter coefficient.

The comment says "IIR filter coefficient 4" but the actual IIR filter coefficient is 15 (register value 0x04 maps to coefficient 15 per the encoding at lines 169-177). The parenthetical "(gives coefficient ~15)" clarifies, but the leading text is confusing. The @note at line 325 correctly says "coefficient 15". Consider aligning the comment.

Proposed fix

-    // Configure IIR filter coefficient 4 (gives coefficient ~15) for both pressure and temperature
+    // Configure IIR filter coefficient 15 for both pressure and temperature
+    // (register value 0x04 = coefficient 15, per BMP580 datasheet)

---

230-232: Debug-only variables are always assigned but never consumed.

bmp580_last_raw_temp and bmp580_last_raw_press are written on every sample (line 531-532) but never read anywhere. If intended for debugger inspection, wrap them in #ifdef DEBUG to avoid dead stores in release builds.

src/main/drivers/barometer/barometer_bmp580.c (3)

246-260: Stack buffer in SPI read helper is silently bounded — consider a guard.

buf is fixed at BMP580_DATA_FRAME_SIZE + 1 (7 bytes), but length is an unchecked uint8_t. Currently all call sites pass length ≤ 6, so this is safe today. A defensive bounds check (or a _Static_assert / comment) would prevent a future caller from overflowing the stack buffer.

Optional: add a bounds check

 static bool bmp580ReadRegisterBuffer(const extDevice_t *dev, uint8_t reg, uint8_t *data, uint8_t length)
 {
     if (dev->bus->busType == BUS_TYPE_SPI) {
+        if (length > BMP580_DATA_FRAME_SIZE) {
+            return false;
+        }
         // For SPI: first byte is dummy on BMP5xx read
         // Max read size: BMP580_DATA_FRAME_SIZE (6) + 1 dummy = 7 bytes
         uint8_t buf[BMP580_DATA_FRAME_SIZE + 1];

---

262-268: EXTI handler is a no-op, yet full interrupt infrastructure is configured.

bmp580_extiHandler does nothing, but bmp580Detect (lines 320-326, 372-382) still sets up the EXTI pin, configures the BMP580 INT_CONFIG register, and enables the DRDY interrupt source. This means the sensor will fire hardware interrupts at 50 Hz that enter an empty ISR — unnecessary context switches. Consider either:

• Removing the interrupt configuration entirely (lines 320-326 and 372-382) since polling is used, or
• Keeping the infrastructure but skipping the sensor-side INT register writes.

This is consistent with other drivers that wire up EOC for future use, so not blocking, but worth noting the wasted cycles.

---

531-558: Verify that Betaflight's barometer subsystem expects pressure in Pa (not Pa×256 or hPa).

The misleading comment on line 547 ("Betaflight expects Pa * 256 for some baros") may cause confusion. In barometer.c, pressureToAltitude() divides by 101325.0f (standard atmospheric pressure in Pa), confirming that Pa is expected. The calculation here (bmp580_up / 64) yields Pa, which is correct.

However, the integer division on line 550 truncates fractional Pa. With 128× OSR, the BMP580's LSB resolution is ~0.016 Pa (1/64). Truncation to integer Pa loses this precision. Other Betaflight baro drivers (e.g., BMP388) also return integer Pa, so this is consistent, but worth documenting that the sub-Pa precision from the high OSR is discarded here.

Also, the comment on line 547 is misleading — consider removing the "Pa * 256" mention to avoid confusing future maintainers:

Clean up misleading comment

-    // Pressure in Pa (Betaflight expects Pa * 256 for some baros, but using Pa directly)
-    // raw / 64 = pressure in Pa
-    // Betaflight barometer interface expects pressure in Pa
+    // Pressure in Pa (Betaflight barometer interface expects integer Pa)
+    // raw / 64 = pressure in Pa
     uint32_t press_pa = bmp580_up / 64;

src/main/sensors/barometer.c (1)

84-137: DEFAULT_BARO_BMP580 is recognized in the I2C bus config (line 149) but is missing from the default baro inference chain.

Line 149 correctly enables I2C bus config when a target pre-defines DEFAULT_BARO_BMP580. However, BMP580 is absent from both:

1. The outer exclusion guard (line 84–88) — if a target defines DEFAULT_BARO_BMP580, the inference block still runs and may additionally define a conflicting DEFAULT_BARO_* macro.
2. The inference chain (lines 90–136) — USE_BARO_BMP580 / USE_BARO_SPI_BMP580 are never inferred as defaults.

No target currently uses DEFAULT_BARO_BMP580, so this doesn't break anything today. But when a target does define it, the conflicting double-definition from (1) could cause unexpected bus config. Consider adding BMP580 to the outer guard and the inference chain for completeness.

Also applies to: 149-149