Skip to content

Csm bringup#233

Open
Firefudge1 wants to merge 5 commits into
mainfrom
csm_bringup
Open

Csm bringup#233
Firefudge1 wants to merge 5 commits into
mainfrom
csm_bringup

Conversation

@Firefudge1
Copy link
Copy Markdown
Contributor

Implements firmware for the Corner Sensor Module (CSM) supporting all four corner boards (FL, FR, RL, RR).

Suspension potentiometer (ADC2) — reading
Strain gauge (ADC1 differential) — reading
IMU (LSM6DSV32XTR) — accel and gyro validated, calibration on boot
Ride height sensor (AFBR-S50) — initialized and reading distances
CAN (FDCAN2) — validated on oscilloscope in external loopback mode (before adding the gyro packet)
RGB LED — ride height distance visualized as blue→red gradient

Key Notes

AFBR-S50 requires SPI Mode 3 (CPOL=1, CPHA=1), not Mode 0
IMU and ride height share SPI1 — protected by FreeRTOS mutex in spi_mutex.c
Location-specific CAN IDs handled via BOARD_FL/FR/RL/RR defines from Bazel build system
Gyro packets added to can_packets.csv (0x508-0x50B) and can_ids.h regenerated

@Firefudge1 Firefudge1 requested a review from a team as a code owner May 23, 2026 03:08
@Firefudge1 Firefudge1 requested review from KaitlynWChang and zohaib642 and removed request for a team May 23, 2026 03:08
@vercel
Copy link
Copy Markdown

vercel Bot commented May 23, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
lhre-2026 Ready Ready Preview, Comment May 23, 2026 3:08am

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces the firmware for the Corner Sensor Module (CSM) targeting the STM32G491xx, incorporating drivers for the LSM6DSV32X IMU and AFBR-S50 ride height sensor alongside ADC-based readouts for strain gauges and suspension potentiometers. The system is integrated with FreeRTOS and includes a CAN communication interface. Feedback highlights several critical issues, such as unresolved merge conflict markers in the configuration files and a mismatch in the Bazel build defines. Technical concerns include incorrect voltage calculations for differential ADC inputs, potential race conditions in the ride height driver, and insufficient mutex protection for shared SPI resources. Additionally, the reviewer suggests optimizing IMU burst reads, reducing excessive task stack sizes, and replacing blocking delays with RTOS-aware delays to prevent system hangs.

@@ -1,4 +1,5 @@
{
<<<<<<< HEAD
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

Merge conflict markers detected in the file. Please resolve the conflict and remove the markers before committing.


// Read strain gauge
int32_t strain_raw = strainGaugeGetVal(&hadc1, ADC_CHANNEL_4);
float strain_voltage = ((float)strain_raw / 4095.0f) * 5000.0f;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The strain gauge ADC (ADC1) is configured in differential mode. In this mode on STM32G4, the result is a 12-bit signed value in 2's complement format (ranging from -2048 to 2047). The current calculation treats it as a 12-bit unsigned value (0-4095), which will lead to incorrect voltage readings when the differential input is negative. Additionally, ensure that the raw value is properly sign-extended if necessary.

return status;
}

status_t S2PI_TransferFrame(s2pi_slave_t slave, uint8_t const * txData, uint8_t * rxData, size_t frameSize, s2pi_callback_t callback, void * callbackData)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

The S2PI_TransferFrame implementation does not appear to use the spi1_mutex defined in spi_mutex.h. Since SPI1 is shared between the IMU and the Ride Height sensor, and the IMU driver uses spi1_mutex, the Ride Height driver must also use the same mutex to prevent bus contention. The current use of myS2PIHnd.SpiMutexBlocked is insufficient as it is independent of the IMU's locking mechanism.

Comment thread CSM/firmware/BUILD.bazel
linker_script = "STM32G491XX_FLASH.ld",
startup_script = "startup_stm32g491xx.s",
defines = [
"STM32G474xx",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The defines list still contains STM32G474xx, but the linker script and startup script have been updated to STM32G491xx. This mismatch might lead to incorrect peripheral configurations or missing features if the target hardware is indeed the G491.

Suggested change
"STM32G474xx",
"STM32G491xx",

const osThreadAttr_t accelRideHeightTask_attributes = {
.name = "accelRideHeightTask",
.priority = (osPriority_t) osPriorityNormal,
.stack_size = 4096 * 4
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The stack size for accelRideHeightTask is set to 16KB (4096 * 4 bytes). Given the STM32G491 has 112KB of RAM and the task's logic is relatively simple, this seems excessive. Consider reducing it to save memory for other tasks or heap allocation.

    .stack_size = 1024 * 4

Comment on lines +98 to +102
static void imu_read_all_axes(uint8_t reg, uint8_t *buf, uint8_t len) {
for (uint8_t i = 0; i < len; i++) {
buf[i] = imu_read(reg + i);
}
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The current implementation of imu_read_all_axes performs individual 1-byte SPI reads for each register. This is highly inefficient as it involves multiple mutex acquisitions and CS toggles. The LSM6DSV32X supports auto-incrementing burst reads. You should perform a single SPI transaction to read all 6 bytes of the axis data.

static void imu_read_all_axes(uint8_t reg, uint8_t *buf, uint8_t len) {
    osMutexAcquire(spi1_mutex, osWaitForever);
    uint8_t tx = reg | 0x80;
    IMU_PULL_CS_LOW();
    HAL_SPI_Transmit(_hspi, &tx, 1, IMU_TIMEOUT);
    HAL_SPI_Receive(_hspi, buf, len, IMU_TIMEOUT);
    IMU_PULL_CS_HIGH();
    osMutexRelease(spi1_mutex);
}

imu_write(LSM6DSV32X_CTRL2_G, CTRL2_G_CONFIG); // 0x5C: 208Hz ±4000dps
}

HAL_Delay(10);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using HAL_Delay inside a FreeRTOS task blocks the entire CPU for the duration of the delay, preventing other tasks from running. Use osDelay instead to allow the scheduler to switch to other tasks.

    osDelay(10);

xyz_t gyro_reading;

for (int i = 0; i < CALIBRATION_SAMPLES; i++) {
while (!IMU_GyroStatus());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Busy-waiting on a sensor status bit without a timeout can lead to a task hang if the sensor hardware fails or becomes unresponsive. Implement a timeout mechanism or use a non-blocking approach with a maximum retry count.

}

float ride_height_get_distance_mm() {
if (!myDataReadyEvents) { // shouldn't myDataReadyEvents be a boolean value? right now its an 8-bit uint value. also with the change we would also have to change the other code that references it
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The check if (!myDataReadyEvents) is performed outside of any synchronization mechanism, while the variable is modified in an ISR and decremented under an IRQ lock later. This is a race condition. Although myDataReadyEvents is volatile, the check and subsequent logic should be handled more robustly to ensure atomicity.


HAL_ADC_ConfigChannel(hadc, &sConfig);
HAL_ADC_Start(hadc);
HAL_ADC_PollForConversion(hadc, HAL_MAX_DELAY);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using HAL_MAX_DELAY in HAL_ADC_PollForConversion is risky. If the ADC hardware fails to complete a conversion, the task will hang indefinitely. It is safer to use a reasonable timeout and handle the error case.

Suggested change
HAL_ADC_PollForConversion(hadc, HAL_MAX_DELAY);
if (HAL_ADC_PollForConversion(hadc, 10) != HAL_OK) return 0;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant