Fix static analysis issues - #1
Open
stephenkingston wants to merge 1 commit into
Open
Conversation
- main.h: Change bare definitions of newCommandAvailable and motor[] to extern declarations to fix ODR (multiple-definition) violations when the header is included by more than one translation unit. - main.c: Add authoritative definitions of newCommandAvailable and motor[3] in the USER CODE PV section. - main.c: Move programInit() call from USER CODE BEGIN 1 (before HAL_Init()) to USER CODE BEGIN 2 (after all peripheral inits) so HAL and peripherals are ready before application state is set up. - main.c: Replace empty microsecondDelay() loop with 72 __NOP() calls so the delay is not eliminated by the optimiser at -O2/-O3. - main.c: Clamp rampDownCount to [0, NO_OF_RAMP_STEPS-1] before passing it to ramp() to prevent out-of-bounds access into cosine[]. - main.c: Guard getDurationOfUninterruptedMovement() against zero or negative numOfSteps (negative array index), guard the else-branch index, and replace the magic number 64 with NO_OF_RAMP_STEPS. - main.c: Fix performDataValidation() else-if branch using motor-> (which pointed at motor[0] via the shadowed global) to motor[i]. - main.c: Rename max() to maxIndex() to avoid shadowing the stdlib max() macro; update the single call site accordingly. - stm32f1xx_it.c: Fix TIM4 ISR pulse condition from != to < to match TIM2/TIM3 and prevent spurious pulses when currentCount overshoots. - usbd_cdc_if.c: Add bounds check before copying USB receive data into receivedData[12] to prevent buffer overrun when *Len > 12.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR fixes a set of static analysis issues identified across four source files. Each change is described below.
Core/Inc/main.h— ODR / multiple-definition violationnewCommandAvailableandmotor[3]were defined (not just declared) in a header. Every translation unit that includedmain.hgot its own copy, causing undefined behaviour (multiple definitions of the same external-linkage object). Changed both toexterndeclarations; the single authoritative definitions now live inmain.c.Core/Src/main.cGlobals definition — Added the definitions of
newCommandAvailableandmotor[3]in theUSER CODE PVsection to match theexterndeclarations now in the header.programInit()called beforeHAL_Init()—programInit()was placed inUSER CODE BEGIN 1, which runs beforeHAL_Init()and peripheral initialisation. It sets up timer handles and function pointers that depend on the HAL being ready. Moved the call toUSER CODE BEGIN 2, afterMX_USB_DEVICE_Init().microsecondDelay()optimised away — The emptyforloop body was silently deleted by the compiler at-O2/-O3, making the pulse width zero. Replaced with 72__NOP()intrinsics (~1 µs at 72 MHz) which the compiler cannot remove.rampDownCountout-of-bounds index intocosine[]—rampDownCountis auint8_tpassed directly as an index intocosine[NO_OF_RAMP_STEPS](64 entries). In both the long-move and short-move branches the value was computed fromabs(targetCount - currentCount)without clamping, so values >= 64 caused an out-of-bounds read. Added an explicit clamp toNO_OF_RAMP_STEPS - 1after the cast.getDurationOfUninterruptedMovement()— negative array index — WhennumOfSteps == 0or1, the expression(numOfSteps/2) - 1evaluates to-1, producing an out-of-bounds array access. Added an early return fornumOfSteps <= 0, a safeidxclamp for theelsebranch, and replaced the magic number64with the existingNO_OF_RAMP_STEPSmacro.performDataValidation()wrong motor pointer — Theelse ifbranch usedmotor->absolutePositionandmotor->newAbsoluteTarget(the parameter namemotorshadowed the global array and always pointed atmotor[0]). Changed tomotor[i].absolutePosition/motor[i].newAbsoluteTargetso all three motors are validated correctly.max()shadows stdlib macro — The local functionmax()conflicts with themax()macro defined in some standard library headers, causing silent macro expansion or redefinition warnings. Renamed tomaxIndex()and updated the single call site.Core/Src/stm32f1xx_it.c— TIM4 inconsistent pulse conditionThe TIM2 and TIM3 ISRs use
currentCount < targetCountto decide whether to setpulseFlag. The TIM4 ISR usedcurrentCount != targetCount, which would also fire whencurrentCount > targetCount(overshoot), generating a spurious extra pulse. Changed to<for consistency.USB_DEVICE/App/usbd_cdc_if.c— USB receive buffer overrunreceivedDatais declared asuint8_t receivedData[12]. The copy loop iterated up to*Lenbytes without checking whether*Lenexceeded 12, allowing a host to overwrite memory beyond the buffer. Added a bounds check:copyLen = min(*Len, sizeof(receivedData))before the loop.Build note
The
arm-none-eabi-gcctoolchain is not present in the CI VM, so a full build could not be verified automatically. The changes are syntactically correct C and have been reviewed against the HAL headers present in the repository.