am243x : add configuration of pulse per revolution by a1248924 · Pull Request #162 · TexasInstruments/open-pru

a1248924 · 2026-08-19T08:30:34Z

signed-off by Ayushman a-ayushman@ti.com

qodo-code-review · 2026-08-19T08:31:48Z

PR Summary by Qodo

Add configurable PPR-based QPOS wraparound for AM243x PRU eQEP

✨ Enhancement 🐞 Bug fix 📝 Documentation ⚙️ Configuration changes 🕐 20-40 Minutes

AI Description

• Adds build-time PPR-based QPOSMAX wrapping across all six PRU eQEP channels.
• Publishes counter modulus and last-edge direction through shared memory for reliable R5F reads.
• Documents encoder configuration and refreshes AM243x UART logging settings.

Diagram

graph TD
  A["Encoder edges"] -->|quadrature input| B["PRU firmware"] -->|updates| C["Bounded QPOS"] -->|publishes values| D[("Shared DMEM")] -->|reads state| E["R5F polling"] -->|reports| F["eQEP diagnostics"]
  B -->|last direction| D

High-Level Assessment

The following are alternative approaches to this PR:

1. Runtime DMEM configuration

➕ Changes PPR without rebuilding firmware
➕ Could support a different modulus per channel
➖ Requires ownership, validation, and startup synchronization rules
➖ Adds shared-memory reads or additional register initialization to firmware

Recommendation: Keep the compile-time QPOSMAX approach for the current fixed-encoder example because it is deterministic and avoids hot-path overhead; publishing the resolved value and edge direction to R5F prevents duplicated constants and ambiguous host-side inference. Consider runtime, per-channel DMEM configuration only if deployments must switch encoders without reflashing.

Files changed (7) +132 / -35

Enhancement (4) +53 / -0

macros.incDefine the encoder-derived QPOSMAX constant +3/-0

Define the encoder-derived QPOSMAX constant
• Adds the firmware build-time position maximum, defaulting to 3999 for a 1000-PPR encoder with x4 quadrature decoding.
examples/pru_eqep/firmware/include/macros.inc

memory.incAllocate QPOSMAX state and per-channel DMEM offsets +13/-0

Allocate QPOSMAX state and per-channel DMEM offsets
• Reserves a PRU register for the cached counter maximum. Defines six-channel shared-memory locations for the published QPOSMAX value and last processed direction.
examples/pru_eqep/firmware/include/memory.inc

main.asmBound QPOS and publish modulus and direction +27/-0

Bound QPOS and publish modulus and direction

• Loads and publishes QPOSMAX during firmware startup, then wraps increments and decrements within the configured range. Records each processed edge's direction in per-channel shared memory for direct R5F consumption.

examples/pru_eqep/firmware/main.asm

eqep_diagnostic.hTrack firmware-published QPOSMAX and direction +10/-0

Track firmware-published QPOSMAX and direction
• Extends each channel configuration with shared-memory pointers for QPOSMAX and last direction, plus a cached modulus value.
examples/pru_eqep/mcuplus/eqep_diagnostic.h

Bug fix (1) +53 / -26

pru_eqep_example.cConsume per-channel modulus and firmware direction +53/-26

Consume per-channel modulus and firmware direction

• Maps the new DMEM fields, caches QPOSMAX after firmware startup, and reads last-edge direction during polling and position capture. This replaces position-difference inference that can become ambiguous after long R5F stalls and counter rewraps.

examples/pru_eqep/mcuplus/pru_eqep_example.c

Documentation (1) +20 / -2

readme.mdDocument PPR configuration and shared-memory diagnostics +20/-2

Document PPR configuration and shared-memory diagnostics

• Explains QPOSMAX wraparound, the PPR-to-modulus formula, rebuild requirements, and current lack of runtime configuration. Updates direction troubleshooting and the DMEM memory map for the newly published fields.

examples/pru_eqep/readme.md

Other (1) +6 / -7

example.syscfgRefresh UART debug logging configuration +6/-7

Refresh UART debug logging configuration
• Replaces deprecated shared-memory and memory-log options with CSS logging control and explicitly configures the console baud rate to 921600.
examples/pru_eqep/mcuplus/am243x-lp/r5fss0-0_freertos/example.syscfg

qodo-code-review · 2026-08-19T08:34:21Z

Code Review by Qodo

🐞 Bugs (5) 📘 Rule violations (0) 📜 Skill insights (0)

1. ~~Idle direction stays nonzero~~ ✗ Dismissed 🐞 Bug ≡ Correctness

Description

The polling loop unconditionally converts the firmware's sticky last-edge byte into ±1, overriding
the zero assigned for “no change” on every idle pass. After the first encoder edge, status and
position APIs therefore continue reporting motion direction indefinitely while the shaft is
stationary.

Code

examples/pru_eqep/mcuplus/pru_eqep_example.c[R404-406]

+            uint8_t last_dir = HW_RD_REG8((uint32_t)ABZHandle[ch]->last_dir_base);
+            if      (last_dir == 1) ABZHandle[ch]->direction =  1;
+            else if (last_dir == 2) ABZHandle[ch]->direction = -1;

Evidence
The host establishes zero as the no-change result, then always overwrites it when the persistent
byte is 1 or 2. Firmware writes LAST_DIR only in processed transition paths and has no idle-loop
clear, while the second host reader likewise maps the retained byte to ±1.
examples/pru_eqep/mcuplus/pru_eqep_example.c[370-371]
examples/pru_eqep/mcuplus/pru_eqep_example.c[397-406]
examples/pru_eqep/firmware/main.asm[140-158]
examples/pru_eqep/firmware/main.asm[183-201]
examples/pru_eqep/mcuplus/pru_eqep_example.c[569-575]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`LAST_DIR` records the last processed edge and is not cleared when no edge occurs, so reading it unconditionally makes an idle channel retain ±1 instead of reporting zero.

## Issue Context
The main loop explicitly resets direction to zero for no change, but the sticky firmware byte immediately overwrites it. `EQEP_Get_position_ABZ` has the same behavior. Add a firmware-owned edge sequence/change indicator so R5F applies `LAST_DIR` only when a new edge has occurred since its previous sample; avoid having R5F clear PRU-owned memory.

## Fix Focus Areas
- examples/pru_eqep/firmware/main.asm[147-158]
- examples/pru_eqep/firmware/main.asm[190-201]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[397-406]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[569-575]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Unsynced QPOSMAX read 🐞 Bug ☼ Reliability

Description

R5F reads qposmax immediately after starting the PRU cores, but there is no handshake/ready flag
ensuring the PRU has executed its boot-time sbco publish yet. If the read happens first (DMEM is
zeroed at init), modulus becomes 1 and the new direction wrap logic produces incorrect direction
results.

Code

examples/pru_eqep/mcuplus/pru_eqep_example.c[R318-325]

+    /* Read QPOSMAX once, after firmware start: PRU publishes it to DMEM
+     * during its own init block (before the capture loop begins), so it
+     * is guaranteed valid here. Read once, not per-poll, since firmware
+     * never rewrites it again after boot. */
+    for (int i = 0; i < 6; i++)
+    {
+        ABZHandle[i]->qposmax = HW_RD_REG32((uint32_t)ABZHandle[i]->qposmax_base);
+    }

Evidence
PRU publishes QPOSMAX at boot, but R5F reads it immediately after core enable without any
synchronization; since ABZ_PRU_ICSS_Init clears PRU data RAM to 0, an early read can return 0 and
invalidate the modulus-based direction calculation.
examples/pru_eqep/firmware/main.asm[64-68]
examples/pru_eqep/mcuplus/pru_eqep_example.c[141-208]
examples/pru_eqep/mcuplus/pru_eqep_example.c[318-325]
examples/pru_eqep/mcuplus/pru_eqep_example.c[210-218]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
R5F reads `ABZHandle[i]->qposmax` right after `EQEP_pruss_load_run_fw()`, assuming PRU has already published `QPOSMAX` into DMEM1. However, core enable is asynchronous and DMEM is explicitly zeroed during init, so this read can return `0` transiently. That makes `modulus = qposmax + 1` equal `1` and breaks the new modulus-based direction computation.

### Issue Context
- PRU publishes QPOSMAX once at boot via `sbco`.
- R5F enables PRU cores and immediately reads QPOSMAX.
- PRU data RAM is cleared to 0 before firmware start.

### Fix Focus Areas
- examples/pru_eqep/mcuplus/pru_eqep_example.c[141-208]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[318-325]
- examples/pru_eqep/firmware/main.asm[64-68]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[210-218]

### Suggested fix
Implement a simple readiness handshake, e.g.:
1) Reserve a DMEM1 `READY` word per channel (or reuse an existing safe location).
2) PRU writes `QPOSMAX` and then writes `READY=0xA5A5A5A5`.
3) R5F loops with a timeout waiting for `READY` before reading `QPOSMAX` (or loops until `qposmax != 0`).
4) If timeout, log an error and fall back to a safe default (or abort).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Rollover reverses reported direction 🐞 Bug ≡ Correctness

Description

The firmware now wraps QPOS from 3999 to 0 and from 0 to 3999, while both R5F readers determine
direction using subtraction modulo 2^32. A forward boundary crossing therefore produces a negative
difference and reports reverse, while a reverse crossing reports forward.

Code

examples/pru_eqep/firmware/main.asm[R135-136]

+    qbge    no_qpos_overflow0, QPOS, scratch2   ; jump if scratch2 >= QPOS (i.e. QPOS <= QPOSMAX)
+    ldi     QPOS, 0

Evidence
The added firmware code reloads QPOS to zero after its configured maximum and reloads QPOSMAX after
decrementing below zero. However, the polling reader explicitly documents and implements only 32-bit
rollover handling at pru_eqep_example.c lines 359-375, and EQEP_Get_position_ABZ repeats that
subtraction at lines 538-544; with QPOSMAX=3999, 3999→0 yields -3999 and 0→3999 yields +3999.
examples/pru_eqep/firmware/main.asm[131-146]
examples/pru_eqep/firmware/main.asm[173-188]
examples/pru_eqep/firmware/include/macros.inc[56-57]
examples/pru_eqep/mcuplus/pru_eqep_example.c[348-375]
examples/pru_eqep/mcuplus/pru_eqep_example.c[526-545]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Firmware now bounds QPOS using `QPOSMAX`, but R5F direction calculations still assume modulo-2^32 rollover. Update direction detection to use the configured counter modulus so crossings between zero and QPOSMAX preserve the actual direction.

## Issue Context
Both polling and `EQEP_Get_position_ABZ()` use signed subtraction intended for `0xFFFFFFFF` rollover. Ensure the host and firmware obtain QPOSMAX from a synchronized definition or shared configuration rather than duplicating an independently editable value.

## Fix Focus Areas
- examples/pru_eqep/firmware/main.asm[131-146]
- examples/pru_eqep/firmware/main.asm[173-188]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[348-375]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[526-545]
- examples/pru_eqep/firmware/include/macros.inc[56-57]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

4. ~~Z edges erase direction~~ ✗ Dismissed 🐞 Bug ≡ Correctness ⭐ New

Description

The decrement fall-through unconditionally publishes qpos_update, so a Z-only transition whose A/B
LUT result is 0 overwrites LAST_DIR with 0. If this occurs after an A/B edge but before R5F polls,
both host readers ignore the 0 and retain an older/default direction instead of reporting the most
recent directional edge.

Code

examples/pru_eqep/firmware/main.asm[158]
+    sbco    &qpos_update, DMEM1, LAST_DIR_OFFSET, 1

Evidence
The firmware wakes for any masked A/B/Z change, while unchanged A/B states map to LUT value 0. The
new unconditional stores at the decrement fall-through therefore publish 0 for Z-only changes, and
the host only acts on values 1 and 2.
examples/pru_eqep/firmware/include/memory.inc[79-92]
examples/pru_eqep/firmware/main.asm[86-92]
examples/pru_eqep/mcuplus/pru_eqep_example.c[500-535]
examples/pru_eqep/firmware/main.asm[149-158]
examples/pru_eqep/mcuplus/pru_eqep_example.c[401-403]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A no-change A/B transition can overwrite `LAST_DIR` with zero because the store after `no_decrement0`/`no_decrement` executes even when `qpos_update` is neither increment nor decrement.

## Issue Context
The capture mask includes Z, so a Z-only edge enters the update path while A/B remains unchanged; LUT states 0x0 and 0xF return zero. `LAST_DIR` should retain the last directional A/B edge.

## Fix Focus Areas
- examples/pru_eqep/firmware/main.asm[147-158]
- examples/pru_eqep/firmware/main.asm[190-201]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

5. Direction flips on long stalls 🐞 Bug ≡ Correctness

Description

The new modulus-based rewrap forces qpos_diff into [-modulus/2, modulus/2), which will report the
opposite direction if the polling loop is delayed long enough for QPOS to advance by more than half
the modulus between reads. This can happen during UART logging or task preemption, producing
deterministic wrong direction even without any counter wraparound.

Code

examples/pru_eqep/mcuplus/pru_eqep_example.c[R396-399]

+            int32_t modulus = (int32_t)ABZHandle[ch]->qposmax + 1;
            int32_t qpos_diff = (int32_t)(ABZHandle[ch]->QPOSCOUNT - ABZHandle[ch]->prev_QPOS);
+            if      (qpos_diff >  modulus / 2) qpos_diff -= modulus;
+            else if (qpos_diff < -modulus / 2) qpos_diff += modulus;

Evidence
The newly added modulus-rewrap code explicitly flips large deltas into the opposite sign, and the
same task contains potentially blocking UART logging—making it realistic for deltas to exceed
modulus/2 between reads and thus invert direction.
examples/pru_eqep/mcuplus/pru_eqep_example.c[390-402]
examples/pru_eqep/mcuplus/pru_eqep_example.c[410-427]
examples/pru_eqep/mcuplus/pru_eqep_example.c[561-573]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Direction is derived from a modulus-wrapped delta:
- Compute `qpos_diff = curr - prev`
- If `qpos_diff > modulus/2`, subtract modulus; if `< -modulus/2`, add modulus

If the task stalls (UART prints, preemption, interrupts), `curr-prev` can legitimately exceed `modulus/2` without any wrap, and the current logic will *flip the sign* and report the wrong direction.

### Issue Context
The main loop includes periodic `DebugP_log(...)` calls which can block long enough for many encoder edges to accumulate.

### Fix Focus Areas
- examples/pru_eqep/mcuplus/pru_eqep_example.c[390-402]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[410-427]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[564-573]

### What to change
- Add an explicit ambiguity guard:
 - If `abs(qpos_diff) > modulus/2` (or `>=` depending on your convention), do **not** infer direction from the wrapped delta.
 - Instead set direction to 0/unknown, or keep the previous direction, or (best) fetch direction from a PRU-provided direction signal/metadata.
- Apply the same fix in both places where the modulus rewrap logic exists (main polling loop and `EQEP_Get_position_ABZ`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

6. ~~Unknown SysCfg property~~ ✗ Dismissed 🐞 Bug ≡ Correctness

Description

The SysCfg file sets debug_log.enableCssLog, but this property is not used anywhere else in the
repo’s SysCfg examples, so SysCfg generation may fail or silently ignore the setting. If
ignored/failing, debug logging configuration will not match what the example expects.

Code

examples/pru_eqep/mcuplus/am243x-lp/r5fss0-0_freertos/example.syscfg[R100-101]
+debug_log.enableUartLog        = true;
+debug_log.enableCssLog         = false;

Evidence
The PR introduces debug_log.enableCssLog, but this key is unique in the repo, while other examples
configure debug_log without it—suggesting it may be unsupported and could break SysCfg processing.
examples/pru_eqep/mcuplus/am243x-lp/r5fss0-0_freertos/example.syscfg[100-110]
examples/spi_loopback/spi_loopback_app/am243x-lp/r5fss0-0_freertos/example.syscfg[62-69]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`example.syscfg` sets `debug_log.enableCssLog`, which appears to be an invalid/unknown configuration knob in this repo context (no other SysCfg uses it). This can break SysCfg generation or lead to the setting being ignored.

### Issue Context
Other example `.syscfg` files configure `debug_log` without any `enableCssLog` field.

### Fix Focus Areas
- examples/pru_eqep/mcuplus/am243x-lp/r5fss0-0_freertos/example.syscfg[100-105]

### What to change
- Confirm the correct SysCfg property name(s) for disabling CCS/shared-mem logging in the `debug_log` module used by this SDK version.
- Replace/remove `debug_log.enableCssLog` accordingly.
- If the intention is “UART-only logging”, align this file with patterns used in other examples (e.g., only `enableUartLog` + uart instance config), or explicitly set the actually-supported shared-mem log toggles.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

View medium (1)

7. QPOSMAX reloaded per edge 🐞 Bug ➹ Performance

Description

The PRU hot path reloads the constant QPOSMAX via ldi32 on every increment path in both duplicated
LUT blocks, adding extra instructions to the edge-processing critical path. This reduces the maximum
sustainable encoder edge rate compared to loading QPOSMAX once at boot into a dedicated register.

Code

examples/pru_eqep/firmware/main.asm[R139-142]

+    ldi32   scratch2, QPOSMAX
+    qbge    no_qpos_overflow0, QPOS, scratch2   ; jump if scratch2 >= QPOS (i.e. QPOS <= QPOSMAX)
+    ldi     QPOS, 0
+no_qpos_overflow0:

Evidence
The increment paths in both duplicated update blocks perform ldi32 scratch2, QPOSMAX just to do
the wrap compare, despite QPOSMAX being constant and already available to load once in the init
block.
examples/pru_eqep/firmware/main.asm[138-142]
examples/pru_eqep/firmware/main.asm[179-184]
examples/pru_eqep/firmware/main.asm[64-68]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ldi32 scratch2, QPOSMAX` is executed on every qualifying edge before the wrap compare, even though QPOSMAX is a compile-time constant.

### Issue Context
This firmware claims high-speed capture; adding avoidable instructions inside the per-edge loop reduces the headroom.

### Fix Focus Areas
- examples/pru_eqep/firmware/main.asm[64-68]
- examples/pru_eqep/firmware/main.asm[138-142]
- examples/pru_eqep/firmware/main.asm[179-184]

### Suggested fix
- Load QPOSMAX once during init into an unused register (e.g., `r29`/`r30` depending on conventions), and use that register for both wrap compares.
- Ensure both duplicated LUT-handling blocks use the same cached register so the hot path does not perform constant reloads.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

8. Unused QPOSMAX offset defines 🐞 Bug ⚙ Maintainability

Description

The PR adds per-channel CH1_QPOSMAX_OFFSET..CH5_QPOSMAX_OFFSET defines, but the code initializes
all qposmax_base pointers using only CH0_QPOSMAX_OFFSET, leaving the per-channel constants
unused and likely to drift out of sync with firmware offsets. This increases maintenance risk for
future layout changes.

Code

examples/pru_eqep/mcuplus/pru_eqep_example.c[R78-82]

+// Define QPOSMAX offsets for each channel (PRU-write-once at boot, R5F-read-once at init)
+#define CH0_QPOSMAX_OFFSET 0x4C
+#define CH1_QPOSMAX_OFFSET 0x50
+#define CH2_QPOSMAX_OFFSET 0x54
+#define CH3_QPOSMAX_OFFSET 0x58

Evidence
The new per-channel QPOSMAX offsets are defined, but the subsequent initialization uses only
CH0_QPOSMAX_OFFSET for every channel’s qposmax_base, making the other new defines dead and
potentially misleading.
examples/pru_eqep/mcuplus/pru_eqep_example.c[78-85]
examples/pru_eqep/mcuplus/pru_eqep_example.c[282-287]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Per-channel QPOSMAX offset macros were added, but the initialization uses only `CH0_QPOSMAX_OFFSET` for all channels. This leaves the new macros unused and creates duplicated constants that can silently drift.

### Issue Context
Today this likely “works by construction” because `baseMemAddr1` is offset by `i*4`, but that coupling is non-obvious and fragile.

### Fix Focus Areas
- examples/pru_eqep/mcuplus/pru_eqep_example.c[78-85]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[282-287]

### What to change
Pick one:
1) Remove `CH1_QPOSMAX_OFFSET..CH5_QPOSMAX_OFFSET` and document that per-channel spacing is provided by the `baseMemAddr1` stride.
2) Use the per-channel defines when building `qposmax_base` (e.g., an offset array indexed by channel), so the code matches the comments and the constants serve a purpose.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Context sources

Review mode: ⚖️ Balanced

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous reviews

Review updated until commit 496e405

Results up to commit f8618ff ⚖️ Balanced

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Rollover reverses reported direction 🐞 Bug ≡ Correctness

Description

The firmware now wraps QPOS from 3999 to 0 and from 0 to 3999, while both R5F readers determine
direction using subtraction modulo 2^32. A forward boundary crossing therefore produces a negative
difference and reports reverse, while a reverse crossing reports forward.

Code

examples/pru_eqep/firmware/main.asm[R135-136]

+    qbge    no_qpos_overflow0, QPOS, scratch2   ; jump if scratch2 >= QPOS (i.e. QPOS <= QPOSMAX)
+    ldi     QPOS, 0

Evidence
The added firmware code reloads QPOS to zero after its configured maximum and reloads QPOSMAX after
decrementing below zero. However, the polling reader explicitly documents and implements only 32-bit
rollover handling at pru_eqep_example.c lines 359-375, and EQEP_Get_position_ABZ repeats that
subtraction at lines 538-544; with QPOSMAX=3999, 3999→0 yields -3999 and 0→3999 yields +3999.
examples/pru_eqep/firmware/main.asm[131-146]
examples/pru_eqep/firmware/main.asm[173-188]
examples/pru_eqep/firmware/include/macros.inc[56-57]
examples/pru_eqep/mcuplus/pru_eqep_example.c[348-375]
examples/pru_eqep/mcuplus/pru_eqep_example.c[526-545]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Firmware now bounds QPOS using `QPOSMAX`, but R5F direction calculations still assume modulo-2^32 rollover. Update direction detection to use the configured counter modulus so crossings between zero and QPOSMAX preserve the actual direction.

## Issue Context
Both polling and `EQEP_Get_position_ABZ()` use signed subtraction intended for `0xFFFFFFFF` rollover. Ensure the host and firmware obtain QPOSMAX from a synchronized definition or shared configuration rather than duplicating an independently editable value.

## Fix Focus Areas
- examples/pru_eqep/firmware/main.asm[131-146]
- examples/pru_eqep/firmware/main.asm[173-188]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[348-375]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[526-545]
- examples/pru_eqep/firmware/include/macros.inc[56-57]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Results up to commit 2ba57e6 ⚖️ Balanced

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Unsynced QPOSMAX read 🐞 Bug ☼ Reliability

Description

R5F reads qposmax immediately after starting the PRU cores, but there is no handshake/ready flag
ensuring the PRU has executed its boot-time sbco publish yet. If the read happens first (DMEM is
zeroed at init), modulus becomes 1 and the new direction wrap logic produces incorrect direction
results.

Code

examples/pru_eqep/mcuplus/pru_eqep_example.c[R318-325]

+    /* Read QPOSMAX once, after firmware start: PRU publishes it to DMEM
+     * during its own init block (before the capture loop begins), so it
+     * is guaranteed valid here. Read once, not per-poll, since firmware
+     * never rewrites it again after boot. */
+    for (int i = 0; i < 6; i++)
+    {
+        ABZHandle[i]->qposmax = HW_RD_REG32((uint32_t)ABZHandle[i]->qposmax_base);
+    }

Evidence
PRU publishes QPOSMAX at boot, but R5F reads it immediately after core enable without any
synchronization; since ABZ_PRU_ICSS_Init clears PRU data RAM to 0, an early read can return 0 and
invalidate the modulus-based direction calculation.
examples/pru_eqep/firmware/main.asm[64-68]
examples/pru_eqep/mcuplus/pru_eqep_example.c[141-208]
examples/pru_eqep/mcuplus/pru_eqep_example.c[318-325]
examples/pru_eqep/mcuplus/pru_eqep_example.c[210-218]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
R5F reads `ABZHandle[i]->qposmax` right after `EQEP_pruss_load_run_fw()`, assuming PRU has already published `QPOSMAX` into DMEM1. However, core enable is asynchronous and DMEM is explicitly zeroed during init, so this read can return `0` transiently. That makes `modulus = qposmax + 1` equal `1` and breaks the new modulus-based direction computation.

### Issue Context
- PRU publishes QPOSMAX once at boot via `sbco`.
- R5F enables PRU cores and immediately reads QPOSMAX.
- PRU data RAM is cleared to 0 before firmware start.

### Fix Focus Areas
- examples/pru_eqep/mcuplus/pru_eqep_example.c[141-208]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[318-325]
- examples/pru_eqep/firmware/main.asm[64-68]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[210-218]

### Suggested fix
Implement a simple readiness handshake, e.g.:
1) Reserve a DMEM1 `READY` word per channel (or reuse an existing safe location).
2) PRU writes `QPOSMAX` and then writes `READY=0xA5A5A5A5`.
3) R5F loops with a timeout waiting for `READY` before reading `QPOSMAX` (or loops until `qposmax != 0`).
4) If timeout, log an error and fall back to a safe default (or abort).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. QPOSMAX reloaded per edge 🐞 Bug ➹ Performance

Description

The PRU hot path reloads the constant QPOSMAX via ldi32 on every increment path in both duplicated
LUT blocks, adding extra instructions to the edge-processing critical path. This reduces the maximum
sustainable encoder edge rate compared to loading QPOSMAX once at boot into a dedicated register.

Code

examples/pru_eqep/firmware/main.asm[R139-142]

+    ldi32   scratch2, QPOSMAX
+    qbge    no_qpos_overflow0, QPOS, scratch2   ; jump if scratch2 >= QPOS (i.e. QPOS <= QPOSMAX)
+    ldi     QPOS, 0
+no_qpos_overflow0:

Evidence
The increment paths in both duplicated update blocks perform ldi32 scratch2, QPOSMAX just to do
the wrap compare, despite QPOSMAX being constant and already available to load once in the init
block.
examples/pru_eqep/firmware/main.asm[138-142]
examples/pru_eqep/firmware/main.asm[179-184]
examples/pru_eqep/firmware/main.asm[64-68]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`ldi32 scratch2, QPOSMAX` is executed on every qualifying edge before the wrap compare, even though QPOSMAX is a compile-time constant.

### Issue Context
This firmware claims high-speed capture; adding avoidable instructions inside the per-edge loop reduces the headroom.

### Fix Focus Areas
- examples/pru_eqep/firmware/main.asm[64-68]
- examples/pru_eqep/firmware/main.asm[138-142]
- examples/pru_eqep/firmware/main.asm[179-184]

### Suggested fix
- Load QPOSMAX once during init into an unused register (e.g., `r29`/`r30` depending on conventions), and use that register for both wrap compares.
- Ensure both duplicated LUT-handling blocks use the same cached register so the hot path does not perform constant reloads.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Results up to commit 90136a2 ⚖️ Balanced

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Direction flips on long stalls 🐞 Bug ≡ Correctness

Description

The new modulus-based rewrap forces qpos_diff into [-modulus/2, modulus/2), which will report the
opposite direction if the polling loop is delayed long enough for QPOS to advance by more than half
the modulus between reads. This can happen during UART logging or task preemption, producing
deterministic wrong direction even without any counter wraparound.

Code

examples/pru_eqep/mcuplus/pru_eqep_example.c[R396-399]

+            int32_t modulus = (int32_t)ABZHandle[ch]->qposmax + 1;
            int32_t qpos_diff = (int32_t)(ABZHandle[ch]->QPOSCOUNT - ABZHandle[ch]->prev_QPOS);
+            if      (qpos_diff >  modulus / 2) qpos_diff -= modulus;
+            else if (qpos_diff < -modulus / 2) qpos_diff += modulus;

Evidence
The newly added modulus-rewrap code explicitly flips large deltas into the opposite sign, and the
same task contains potentially blocking UART logging—making it realistic for deltas to exceed
modulus/2 between reads and thus invert direction.
examples/pru_eqep/mcuplus/pru_eqep_example.c[390-402]
examples/pru_eqep/mcuplus/pru_eqep_example.c[410-427]
examples/pru_eqep/mcuplus/pru_eqep_example.c[561-573]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Direction is derived from a modulus-wrapped delta:
- Compute `qpos_diff = curr - prev`
- If `qpos_diff > modulus/2`, subtract modulus; if `< -modulus/2`, add modulus

If the task stalls (UART prints, preemption, interrupts), `curr-prev` can legitimately exceed `modulus/2` without any wrap, and the current logic will *flip the sign* and report the wrong direction.

### Issue Context
The main loop includes periodic `DebugP_log(...)` calls which can block long enough for many encoder edges to accumulate.

### Fix Focus Areas
- examples/pru_eqep/mcuplus/pru_eqep_example.c[390-402]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[410-427]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[564-573]

### What to change
- Add an explicit ambiguity guard:
 - If `abs(qpos_diff) > modulus/2` (or `>=` depending on your convention), do **not** infer direction from the wrapped delta.
 - Instead set direction to 0/unknown, or keep the previous direction, or (best) fetch direction from a PRU-provided direction signal/metadata.
- Apply the same fix in both places where the modulus rewrap logic exists (main polling loop and `EQEP_Get_position_ABZ`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. ~~Unknown SysCfg property~~ ✗ Dismissed 🐞 Bug ≡ Correctness

Description

The SysCfg file sets debug_log.enableCssLog, but this property is not used anywhere else in the
repo’s SysCfg examples, so SysCfg generation may fail or silently ignore the setting. If
ignored/failing, debug logging configuration will not match what the example expects.

Code

examples/pru_eqep/mcuplus/am243x-lp/r5fss0-0_freertos/example.syscfg[R100-101]
+debug_log.enableUartLog        = true;
+debug_log.enableCssLog         = false;

Evidence
The PR introduces debug_log.enableCssLog, but this key is unique in the repo, while other examples
configure debug_log without it—suggesting it may be unsupported and could break SysCfg processing.
examples/pru_eqep/mcuplus/am243x-lp/r5fss0-0_freertos/example.syscfg[100-110]
examples/spi_loopback/spi_loopback_app/am243x-lp/r5fss0-0_freertos/example.syscfg[62-69]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`example.syscfg` sets `debug_log.enableCssLog`, which appears to be an invalid/unknown configuration knob in this repo context (no other SysCfg uses it). This can break SysCfg generation or lead to the setting being ignored.

### Issue Context
Other example `.syscfg` files configure `debug_log` without any `enableCssLog` field.

### Fix Focus Areas
- examples/pru_eqep/mcuplus/am243x-lp/r5fss0-0_freertos/example.syscfg[100-105]

### What to change
- Confirm the correct SysCfg property name(s) for disabling CCS/shared-mem logging in the `debug_log` module used by this SDK version.
- Replace/remove `debug_log.enableCssLog` accordingly.
- If the intention is “UART-only logging”, align this file with patterns used in other examples (e.g., only `enableUartLog` + uart instance config), or explicitly set the actually-supported shared-mem log toggles.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Unused QPOSMAX offset defines 🐞 Bug ⚙ Maintainability

Description

The PR adds per-channel CH1_QPOSMAX_OFFSET..CH5_QPOSMAX_OFFSET defines, but the code initializes
all qposmax_base pointers using only CH0_QPOSMAX_OFFSET, leaving the per-channel constants
unused and likely to drift out of sync with firmware offsets. This increases maintenance risk for
future layout changes.

Code

examples/pru_eqep/mcuplus/pru_eqep_example.c[R78-82]

+// Define QPOSMAX offsets for each channel (PRU-write-once at boot, R5F-read-once at init)
+#define CH0_QPOSMAX_OFFSET 0x4C
+#define CH1_QPOSMAX_OFFSET 0x50
+#define CH2_QPOSMAX_OFFSET 0x54
+#define CH3_QPOSMAX_OFFSET 0x58

Evidence
The new per-channel QPOSMAX offsets are defined, but the subsequent initialization uses only
CH0_QPOSMAX_OFFSET for every channel’s qposmax_base, making the other new defines dead and
potentially misleading.
examples/pru_eqep/mcuplus/pru_eqep_example.c[78-85]
examples/pru_eqep/mcuplus/pru_eqep_example.c[282-287]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Per-channel QPOSMAX offset macros were added, but the initialization uses only `CH0_QPOSMAX_OFFSET` for all channels. This leaves the new macros unused and creates duplicated constants that can silently drift.

### Issue Context
Today this likely “works by construction” because `baseMemAddr1` is offset by `i*4`, but that coupling is non-obvious and fragile.

### Fix Focus Areas
- examples/pru_eqep/mcuplus/pru_eqep_example.c[78-85]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[282-287]

### What to change
Pick one:
1) Remove `CH1_QPOSMAX_OFFSET..CH5_QPOSMAX_OFFSET` and document that per-channel spacing is provided by the `baseMemAddr1` stride.
2) Use the per-channel defines when building `qposmax_base` (e.g., an offset array indexed by channel), so the code matches the comments and the constants serve a purpose.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Results up to commit a3ee65e ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. ~~Idle direction stays nonzero~~ ✗ Dismissed 🐞 Bug ≡ Correctness

Description

The polling loop unconditionally converts the firmware's sticky last-edge byte into ±1, overriding
the zero assigned for “no change” on every idle pass. After the first encoder edge, status and
position APIs therefore continue reporting motion direction indefinitely while the shaft is
stationary.

Code

examples/pru_eqep/mcuplus/pru_eqep_example.c[R404-406]

+            uint8_t last_dir = HW_RD_REG8((uint32_t)ABZHandle[ch]->last_dir_base);
+            if      (last_dir == 1) ABZHandle[ch]->direction =  1;
+            else if (last_dir == 2) ABZHandle[ch]->direction = -1;

Evidence
The host establishes zero as the no-change result, then always overwrites it when the persistent
byte is 1 or 2. Firmware writes LAST_DIR only in processed transition paths and has no idle-loop
clear, while the second host reader likewise maps the retained byte to ±1.
examples/pru_eqep/mcuplus/pru_eqep_example.c[370-371]
examples/pru_eqep/mcuplus/pru_eqep_example.c[397-406]
examples/pru_eqep/firmware/main.asm[140-158]
examples/pru_eqep/firmware/main.asm[183-201]
examples/pru_eqep/mcuplus/pru_eqep_example.c[569-575]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`LAST_DIR` records the last processed edge and is not cleared when no edge occurs, so reading it unconditionally makes an idle channel retain ±1 instead of reporting zero.

## Issue Context
The main loop explicitly resets direction to zero for no change, but the sticky firmware byte immediately overwrites it. `EQEP_Get_position_ABZ` has the same behavior. Add a firmware-owned edge sequence/change indicator so R5F applies `LAST_DIR` only when a new edge has occurred since its previous sample; avoid having R5F clear PRU-owned memory.

## Fix Focus Areas
- examples/pru_eqep/firmware/main.asm[147-158]
- examples/pru_eqep/firmware/main.asm[190-201]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[397-406]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[569-575]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

a1248924 · 2026-08-19T08:40:59Z

/agentic_review

qodo-code-review · 2026-08-19T08:44:50Z

Code review by qodo was updated up to the latest commit f8618ff

a1248924 · 2026-08-21T06:07:20Z

/agentic_review

qodo-code-review · 2026-08-21T06:12:33Z

Code review by qodo was updated up to the latest commit 2ba57e6

qodo-code-review · 2026-08-21T06:36:25Z

Code review by qodo was updated up to the latest commit 90136a2

a1248924 · 2026-08-31T08:01:03Z

/agentic_review

qodo-code-review · 2026-08-31T08:06:09Z

Code review by qodo was updated up to the latest commit a3ee65e

signed-off by Ayushman <a-ayushman@ti.com>

qodo-code-review · 2026-08-31T08:32:31Z

Code review by qodo was updated up to the latest commit 496e405

pratheesh-ti · 2026-09-04T07:52:39Z

 no_increment0:
    qbbc    no_decrement0, qpos_update, 1
    sub     QPOS, QPOS, 1
+    ldi32   scratch2, 0xFFFFFFFF


Use fill &scratch2, 4 to save 1 cycle

pratheesh-ti · 2026-09-04T07:53:27Z

+
+    ; Cache QPOSMAX in a dedicated register once at boot, so the hot-path
+    ; overflow compares below don't pay for an ldi32 reload on every edge.
+    ldi32   qposmax_reg, QPOSMAX


ldi is sufficient since QPOSMAX < 16-bit

pratheesh-ti · 2026-09-04T07:53:38Z

    sub     QPOS, QPOS, 1
+    ldi32   scratch2, 0xFFFFFFFF
+    qbne    no_qpos_underflow0, QPOS, scratch2  ; only reload if result is exactly 0xFFFFFFFF (true underflow)
+    ldi32   QPOS, QPOSMAX


ldi is sufficient since QPOSMAX < 16-bit

pratheesh-ti · 2026-09-04T07:54:38Z

 no_increment:
    qbbc    no_decrement, qpos_update, 1
    sub     QPOS, QPOS, 1
+    ldi32   scratch2, 0xFFFFFFFF


pratheesh-ti · 2026-09-04T07:54:53Z

    sub     QPOS, QPOS, 1
+    ldi32   scratch2, 0xFFFFFFFF
+    qbne    no_qpos_underflow, QPOS, scratch2   ; only reload if result is exactly 0xFFFFFFFF (true underflow)
+    ldi32   QPOS, QPOSMAX


ldi is sufficient

pratheesh-ti · 2026-09-04T08:14:42Z

    uint32_t phase_err_count_last_seen;
    uint32_t phase_error_flag;
+
+    // Counter modulus published once by firmware at boot (PRU-write-once, R5F-read-once)


Use C style comments for consistency

pratheesh-ti · 2026-09-04T08:14:57Z

+    uint32_t *qposmax_base;
+    uint32_t qposmax;
+
+    // Direction of the last processed edge, published by firmware every edge


Use C style comments

pratheesh-ti · 2026-09-04T08:15:18Z

+#define CH4_QPOSMAX_OFFSET 0x5C
+#define CH5_QPOSMAX_OFFSET 0x60
+
+// Define last-direction offsets for each channel (PRU-write-every-edge, R5F-read-only)


Use C style comments

pratheesh-ti · 2026-09-04T08:15:28Z

+        ABZHandle[i]->qposmax = HW_RD_REG32((uint32_t)ABZHandle[i]->qposmax_base);
+    }

    // Log messages


Use C style comments

qodo-code-review Bot reviewed Aug 19, 2026

View reviewed changes

Comment thread examples/pru_eqep/firmware/main.asm Outdated

a1248924 force-pushed the ADD_PRU_EQEP_PPR_CONFIG branch from f8618ff to 2ba57e6 Compare August 21, 2026 06:06

qodo-code-review Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread examples/pru_eqep/mcuplus/pru_eqep_example.c

Comment thread examples/pru_eqep/firmware/main.asm Outdated

a1248924 force-pushed the ADD_PRU_EQEP_PPR_CONFIG branch from 2ba57e6 to 90136a2 Compare August 21, 2026 06:27

qodo-code-review Bot reviewed Aug 21, 2026

View reviewed changes

Comment thread examples/pru_eqep/mcuplus/am243x-lp/r5fss0-0_freertos/example.syscfg

Comment thread examples/pru_eqep/mcuplus/pru_eqep_example.c Outdated

Comment thread examples/pru_eqep/mcuplus/pru_eqep_example.c

a1248924 force-pushed the ADD_PRU_EQEP_PPR_CONFIG branch from 90136a2 to a3ee65e Compare August 31, 2026 08:00

qodo-code-review Bot reviewed Aug 31, 2026

View reviewed changes

Comment thread examples/pru_eqep/mcuplus/pru_eqep_example.c

am243x : add configuration of pulse per revolution

496e405

signed-off by Ayushman <a-ayushman@ti.com>

a1248924 force-pushed the ADD_PRU_EQEP_PPR_CONFIG branch from a3ee65e to 496e405 Compare August 31, 2026 08:26

qodo-code-review Bot reviewed Aug 31, 2026

View reviewed changes

Comment thread examples/pru_eqep/firmware/main.asm

a1248924 requested review from dhavaljk, manojKoppolu, pratheesh-ti and rajul-bhambay August 31, 2026 08:51

pratheesh-ti requested changes Sep 4, 2026

View reviewed changes

Uh oh!

Conversation

a1248924 commented Aug 19, 2026

Uh oh!

qodo-code-review Bot commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary by Qodo

Uh oh!

qodo-code-review Bot commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

a1248924 commented Aug 19, 2026

Uh oh!

qodo-code-review Bot commented Aug 19, 2026

Uh oh!

a1248924 commented Aug 21, 2026

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 21, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 21, 2026

Uh oh!

a1248924 commented Aug 31, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 31, 2026

Uh oh!

Uh oh!

qodo-code-review Bot commented Aug 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

qodo-code-review Bot commented Aug 19, 2026 •

edited

Loading

qodo-code-review Bot commented Aug 19, 2026 •

edited

Loading