Skip to content

fix(HardwareTimer): use ARR+1 in getCaptureCompare percent/resolution#3019

Open
94xhn wants to merge 1 commit into
stm32duino:mainfrom
94xhn:fix/hardwaretimer-getcapturecompare-percent
Open

fix(HardwareTimer): use ARR+1 in getCaptureCompare percent/resolution#3019
94xhn wants to merge 1 commit into
stm32duino:mainfrom
94xhn:fix/hardwaretimer-getcapturecompare-percent

Conversation

@94xhn

@94xhn 94xhn commented Jul 11, 2026

Copy link
Copy Markdown

Pull Request template

Summary

HardwareTimer::getCaptureCompare() and HardwareTimer::setCaptureCompare() are meant to be inverse operations: one converts a percent/N-bit duty-cycle value to a CCR register tick value, the other converts it back. They currently use inconsistent formulas, so a round trip does not return the original value, and a divide-by-zero is reachable.

This PR fixes/implements the following bugs/features

  • Bug: getCaptureCompare() uses ARR as the "full scale" divisor for PERCENT_COMPARE_FORMAT and every RESOLUTION_xB_COMPARE_FORMAT case, while setCaptureCompare() uses ARR + 1 for the same conversions (per the code's own comment). This makes get/set round-trips wrong and, when ARR == 0, divides by zero.

Motivation

In libraries/SrcWrapper/src/HardwareTimer.cpp:

setCaptureCompare() (correct, and the source of truth for the intended formula):

// As per Reference Manual PWM reach 100% with CCRx value strictly greater than ARR (So ARR+1 in our case)
case PERCENT_COMPARE_FORMAT:
  CCR_RegisterValue = ((LL_TIM_GetAutoReload(_timerObj.instance) + 1) * compare) / 100;
  break;
...
case RESOLUTION_1B_COMPARE_FORMAT: ... case RESOLUTION_16B_COMPARE_FORMAT:
  CCR_RegisterValue = ((LL_TIM_GetAutoReload(_timerObj.instance) + 1) * compare) / ((1 << format) - 1);
  break;

getCaptureCompare() (before this PR, missing the + 1):

case PERCENT_COMPARE_FORMAT:
  return_value = (CCR_RegisterValue * 100) / LL_TIM_GetAutoReload(_timerObj.instance);
  break;
...
case RESOLUTION_1B_COMPARE_FORMAT: ... case RESOLUTION_16B_COMPARE_FORMAT:
  return_value = (CCR_RegisterValue * ((1 << format) - 1)) / LL_TIM_GetAutoReload(_timerObj.instance);
  break;

Because the getter divides by ARR instead of ARR + 1:

  1. Round-trip is wrong, worse for lower-resolution timers. Example with PERCENT_COMPARE_FORMAT: calling setCaptureCompare(ch, 100, PERCENT_COMPARE_FORMAT) and then immediately getCaptureCompare(ch, PERCENT_COMPARE_FORMAT) should read back 100, but reads back 111 when ARR == 9, and 200 when ARR == 1. The same discrepancy exists for every RESOLUTION_xB_COMPARE_FORMAT value.
  2. Divide by zero when ARR == 0 (a legal, reachable timer state, e.g. right after setOverflow(1, TICK_FORMAT)). LL_TIM_GetAutoReload() returns 0 and getCaptureCompare(ch, PERCENT_COMPARE_FORMAT) (or any resolution format) computes x / 0.

The fix simply adds the missing + 1 in the getter, matching the setter and the setter's own documented rationale. No public API/signature changes.

Validation

  • Ensure CI build is passed.

  • Since this is pure integer-arithmetic logic (no MCU register access besides reading ARR/CCR through already-abstracted LL calls), I extracted both formulas verbatim into a standalone host-side C program and ran it on a desktop toolchain (independent of any STM32 board) to confirm the discrepancy before the fix and the correct behavior after:

    === set(100%) -> get() round trip, before fix ===
    ARR      CCR    expected%  got(before fix)   got(after fix)
    1        2      100        200               100   <- WRONG
    9        10     100        111               100   <- WRONG
    99       100    100        101               100   <- WRONG
    255      256    100        100               100
    999      1000   100        100               100
    65535    65536  100        100               100
    
    === ARR == 0 -> integer division by zero (reachable via setOverflow(1, TICK_FORMAT)) ===
    setCaptureCompare(ch, 100, PERCENT_COMPARE_FORMAT) with ARR=0 writes CCR=1
    get_capture_compare_percent_BUGGY(ARR=0, CCR=1)  -> crashes with SIGFPE on the host
                                                         (silently returns 0 on Cortex-M
                                                         without DIV_0_TRP, i.e. reports
                                                         0% instead of the real ~100%)
    

    The same overshoot pattern reproduces for the RESOLUTION_8B_COMPARE_FORMAT branch (full_scale = 255): ARR=9 reads back 283 instead of 255, ARR=1 reads back 510 instead of 255.

  • I do not own STM32 hardware set up for this project's test rig, so I have not run this on-target; the change only touches a pure integer formula that is otherwise identical in structure/behaviour to the already-existing, already-shipped setter, so the on-target risk is limited to this exact expression change.

Code formatting

  • Ensure AStyle check is passed thanks CI

Closing issues

N/A – found while reading the timer duty-cycle conversion code, not tied to an existing issue.


Disclosure: I used an AI coding assistant (Claude) to help read through the codebase, extract the reproduction, and draft this PR. All findings were verified by me by compiling and running the standalone reproduction shown above.

getCaptureCompare() divided by LL_TIM_GetAutoReload() (ARR) for
PERCENT_COMPARE_FORMAT and all RESOLUTION_xB_COMPARE_FORMAT cases,
while its counterpart setCaptureCompare() multiplies by ARR+1, per
the code's own comment: "PWM reach 100% with CCRx value strictly
greater than ARR (So ARR+1 in our case)".

This makes get/set inconsistent: reading back a duty cycle written
via setCaptureCompare(ch, 100, PERCENT_COMPARE_FORMAT) does not
return 100, and overshoots more as ARR gets smaller (e.g. ARR=9
reads back 111, ARR=1 reads back 200). When ARR is 0 (a reachable
state, e.g. after setOverflow(1, TICK_FORMAT)), the getter performs
a division by zero.

Use ARR+1 in the getter too, matching the setter and its own
documented rationale.

Signed-off-by: 94xhn <87560781+94xhn@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant