am243x : add configuration of pulse per revolution - #162
Open
a1248924 wants to merge 1 commit into
Open
Conversation
PR Summary by Qodoam243x pru_eqep: publish/configure QPOSMAX (PPR×4−1) and use modulus-aware direction
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. Unsynced QPOSMAX read
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Collaborator
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit f8618ff |
a1248924
force-pushed
the
ADD_PRU_EQEP_PPR_CONFIG
branch
from
August 21, 2026 06:06
f8618ff to
2ba57e6
Compare
Collaborator
Author
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 2ba57e6 |
signed-off by Ayushman <a-ayushman@ti.com>
a1248924
force-pushed
the
ADD_PRU_EQEP_PPR_CONFIG
branch
from
August 21, 2026 06:27
2ba57e6 to
90136a2
Compare
Comment on lines
+396
to
+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; |
There was a problem hiding this comment.
2. Direction flips on long stalls 🐞 Bug ≡ Correctness
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.
Agent Prompt
### 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
Comment on lines
+78
to
+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 |
There was a problem hiding this comment.
3. Unused qposmax offset defines 🐞 Bug ⚙ Maintainability
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.
Agent Prompt
### 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
|
Code review by qodo was updated up to the latest commit 90136a2 |
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.
signed-off by Ayushman a-ayushman@ti.com