Conversation
signed off by Ayushman a-ayushman@ti.com
PR Summary by QodoAdd AM263Px input mirroring and AM243x PRU eQEP examples
AI Description
Diagram
High-Level Assessment
Files changed (60)
|
Code Review by Qodo
1. Input changes are not mirrored live
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| $(MAKE) -C firmware/am263px-lp/icss_m0_pru0_fw/ti-pru-cgt $(ARGUMENTS_PRU) | ||
| $(MAKE) -C firmware/am263px-lp/icss_m0_pru1_fw/ti-pru-cgt $(ARGUMENTS_PRU) | ||
| # am263px-cc | ||
| $(MAKE) -C firmware/am263px-cc/icss_m0_pru0_fw/ti-pru-cgt $(ARGUMENTS_PRU) |
There was a problem hiding this comment.
1. Broken mirror_input targets 🐞 Bug ≡ Correctness
examples/mirror_input/makefile invokes PRU firmware subdirectories (e.g., am263px-lp pru1 and am263px-cc) that are not provided by this example, so make for those targets will fail when it hits the missing -C directories.
Agent Prompt
### Issue description
`examples/mirror_input/makefile` defines build targets for multiple PRU cores/boards (e.g., am263px pru1, am263px-cc) that are not present in this example’s directory structure. This causes deterministic build failures when the `am263px` (and other listed) targets are selected.
### Issue Context
The example appears to only provide PRU0 firmware output (`pru0_load_bin.h`) and the MCU+ app only includes/loads PRU0 firmware.
### Fix Focus Areas
- examples/mirror_input/makefile[132-144]
- examples/mirror_input/firmware/am263px-lp/icss_m0_pru0_fw/ti-pru-cgt/makefile[4-10]
- examples/mirror_input/mcuplus/empty_example.c[40-41]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ; Handle buffer wrap-around before writing, so buffer_addr stays in bounds | ||
| qbgt write_in_bounds, buffer_addr, buffer_size | ||
| ldi buffer_addr, 0 | ||
| ldi READ_POS_BUFF, 0 |
There was a problem hiding this comment.
2. Wrong wrap-around branch 🐞 Bug ≡ Correctness
In examples/pru_eqep/firmware/main.asm, the pre-write wrap check branches to write_in_bounds only when buffer_addr > buffer_size, so in-bounds values reset to 0 and the ring buffer effectively never advances (overwriting the first entry repeatedly).
Agent Prompt
### Issue description
The ring-buffer bounds checks around `buffer_addr` are inverted. The code currently resets `buffer_addr`/`READ_POS_BUFF` when `buffer_addr <= buffer_size` and proceeds to write when `buffer_addr > buffer_size`, which defeats the intent of keeping writes in-bounds.
### Issue Context
This logic sits directly before the `sbbo` timestamp write and `WRITE_PTR_OFFSET` update.
### Fix Focus Areas
- examples/pru_eqep/firmware/main.asm[103-112]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| | Processor | Hardware | Software | | ||
| | --------- | ---------- | -------------------------- | | ||
| | AM263Px | FIXME | FIXME | | ||
|
|
There was a problem hiding this comment.
3. Placeholder readme content 🐞 Bug ⚙ Maintainability
examples/mirror_input/readme.md still uses the template title “Empty Project” and includes FIXME placeholders in the validated HW/SW table, making the new example documentation inaccurate/incomplete.
Agent Prompt
### Issue description
The mirror_input README appears to be copied from a template and still contains placeholder content (title and validation table).
### Issue Context
This is a new example; the README is the primary entry point for users.
### Fix Focus Areas
- examples/mirror_input/readme.md[1-5]
- examples/mirror_input/readme.md[17-20]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
0df4aed to
ff14d4c
Compare
| gPruIcss0Handle = PRUICSS_open(CONFIG_PRU_ICSS0); | ||
|
|
||
| status = PRUICSS_initMemory(gPruIcss0Handle, PRUICSS_DATARAM(PRUICSS_PRU0)); | ||
| DebugP_assert(status != 0); |
There was a problem hiding this comment.
1. Unchecked pruicss handle 🐞 Bug ☼ Reliability
examples/mirror_input/mcuplus/empty_example.c uses the result of PRUICSS_open() without checking for NULL, so an open failure can cause subsequent PRUICSS_* calls to crash or assert inside the driver. This new example should fail fast (assert/log/return) on a NULL handle to avoid hard-to-debug startup failures.
Agent Prompt
### Issue description
`examples/mirror_input/mcuplus/empty_example.c` calls `PRUICSS_open(CONFIG_PRU_ICSS0)` and immediately uses the returned handle without validating it. If the open fails (returns NULL), later calls like `PRUICSS_initMemory()` / `PRUICSS_loadFirmware()` will operate on an invalid handle and can crash or assert.
### Issue Context
Other code in this repo treats a NULL return from `PRUICSS_open()` as a failure case and aborts initialization.
### Fix Focus Areas
- examples/mirror_input/mcuplus/empty_example.c[69-75]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| PROJECT_NAME := empty | ||
| SUPPORTED_PROCESSORS := am243x am261x am263px am263x am62x am64x |
There was a problem hiding this comment.
2. Wrong project name string 🐞 Bug ⚙ Maintainability
examples/mirror_input/makefile sets PROJECT_NAME := empty, so build/clean messages and any project-name-based checks refer to the wrong project. This is inconsistent with the rest of the mirror_input project metadata and should be renamed to mirror_input.
Agent Prompt
### Issue description
The mirror_input top-level makefile declares `PROJECT_NAME := empty`, which is inconsistent with the example name and causes misleading build output.
### Issue Context
The PRU firmware makefile for this example uses `mirror_input_*` output naming, indicating the intended project name.
### Fix Focus Areas
- examples/mirror_input/makefile[7-8]
ⓘ 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 ff14d4c |
ff14d4c to
d93c48d
Compare
| <file path="../../../main.asm" openOnCreation="false" excludeFromBuild="false" action="copy"> | ||
| </file> | ||
| <file path="../../../pru0_load_bin.h" openOnCreation="false" excludeFromBuild="false" action="copy"> | ||
| </file> |
There was a problem hiding this comment.
1. Copied stale pru header 🐞 Bug ☼ Reliability
The PRU CCS projectspec copies a checked-in pru0_load_bin.h into the PRU project, so CCS builds that compile the R5F app without first rebuilding the PRU project can embed and load an outdated firmware image. This makes PRU firmware changes appear ineffective and is inconsistent with the other examples’ CCS workflow.
Agent Prompt
### Issue description
`example.projectspec` for the PRU firmware project copies a repository `pru0_load_bin.h` into the CCS project. This allows the downstream R5F project to compile against a fixed, checked-in firmware array when the PRU project (and its post-build header generation) hasn’t been run.
### Issue Context
Other examples (e.g. `examples/empty/...`) do **not** copy `*_load_bin.h` into the PRU project; they rely on the PRU build post-step to generate the header in the workspace.
### Fix Focus Areas
- examples/mirror_input/firmware/am263px-lp/icss_m0_pru0_fw/ti-pru-cgt/example.projectspec[74-79]
- examples/mirror_input/firmware/pru0_load_bin.h[1-7]
### Suggested fix
- Remove the `<file ... pru0_load_bin.h ... action="copy">` entry from the PRU firmware `example.projectspec`.
- Prefer not to check in `examples/mirror_input/firmware/pru0_load_bin.h` at all; instead, let it be generated by the PRU build (matching the pattern used by `examples/empty`). If a placeholder is required, ensure the R5F project has an explicit build dependency on the PRU firmware project so the generated header is always produced before compiling the R5F sources.
ⓘ 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 d93c48d |
d93c48d to
46898dd
Compare
| wbs r31, EPWM_SIGNAL | ||
| ; move CMP input to ICL output | ||
| qbbs bit_set0, r31,CMP_SIGNAL |
There was a problem hiding this comment.
1. Input changes are not mirrored live 🐞 Bug ≡ Correctness
main waits for each EPWM edge before reading CMP_SIGNAL and updating ICL_SIGNAL, then blocks waiting for the opposite edge. When the comparator input changes while EPWM remains at one level, the output retains its previous value until the next transition, contrary to the example's stated input-mirroring behavior.
Agent Prompt
## Issue description
The PRU firmware samples the comparator input only once per EPWM edge, so changes occurring between EPWM transitions are not propagated to the output.
## Fix Focus Areas
- examples/mirror_input/firmware/main.asm[41-62]
## Recommended Fix
Replace the EPWM-edge-blocking control flow with a continuous loop that reads `CMP_SIGNAL` and immediately sets or clears `ICL_SIGNAL`. If EPWM-gated sampling is actually required, rename and document the example as an edge-sampling example instead of describing it as live input mirroring.
ⓘ 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 46898dd |
46898dd to
854597b
Compare
| Refer to open-pru/examples/readme.md > Supported processors per-project | ||
| for the list of processors that support building this project, and information | ||
| about porting this project to other processors. |
There was a problem hiding this comment.
4. Users cannot find supported hardware 🐞 Bug ≡ Correctness
The mirror-input README directs users to the repository-wide supported-processors table, but that table has no entry for this project. This leaves the required AM263Px configuration undiscoverable through the documented route even though the checked-in host and firmware builds are AM263Px-specific.
Agent Prompt
## Issue description
The mirror-input README refers users to the repository-wide supported-processors table, but that table has no mirror-input row. Users therefore cannot determine from the referenced documentation that the supplied project supports AM263Px.
## Fix Focus Areas
- examples/mirror_input/readme.md[9-11]
- examples/readme.md[80-93]
## Recommended Fix
Add `mirror_input` to the repository-wide supported-processors table with support matching the configurations actually provided, and make the local README clearly identify the currently supported AM263Px board configuration.
ⓘ 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 854597b |
signed off by: Ayushman <a-ayushman@ti.com>
854597b to
2e08a95
Compare
| -I${MCU_PLUS_SDK_PATH}/source/kernel/freertos/portable/TI_ARM_CLANG/ARM_CR5F \ | ||
| -I${MCU_PLUS_SDK_PATH}/source/kernel/freertos/config/am263px/r5f \ | ||
| -I${MCU_PLUS_SDK_PATH}/source/pru_io/driver \ | ||
| -I${OPEN_PRU_PATH}/examples/mirror_input/firmware/am263px-lp \ |
There was a problem hiding this comment.
4. Host-only builds cannot compile 🐞 Bug ≡ Correctness
INCLUDES_common searches firmware/am263px-lp, but the only checked-in pru0_load_bin.h is in the parent firmware directory. When make host is invoked on a clean checkout without first generating the platform-specific header, compilation of empty_example.c stops at its firmware include.
Agent Prompt
## Issue description
The host-only build searches for `pru0_load_bin.h` under `firmware/am263px-lp`, while the seed header is checked in under `firmware`. Consequently, the documented `make host` target cannot compile from a clean checkout unless a PRU build has already generated another copy.
## Fix Focus Areas
- examples/mirror_input/mcuplus/am263px-lp/r5fss0-0_freertos/ti-arm-clang/makefile[72-72]
- examples/mirror_input/firmware/pru0_load_bin.h[1-14]
- examples/mirror_input/firmware/am263px-lp/icss_m0_pru0_fw/ti-pru-cgt/makefile[7-10]
## Recommended Fix
Place the checked-in seed firmware header in `examples/mirror_input/firmware/am263px-lp/pru0_load_bin.h`, matching both the host include path and `MCU_HEX_PATH`. Ensure subsequent PRU builds overwrite that same platform-specific header.
ⓘ 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 2e08a95 |
1 participant
No description provided.