am243x: add pru-eqep example - #149
Conversation
Code Review by Qodo
1. sbbo before buffer bounds
|
PR Summary by Qodoam243x: add PRU-based eQEP (ABZ) example
AI Description
Diagram
High-Level Assessment
Files changed (38)
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| sbbo &time_stamp, DMEM0, buffer_addr, 4 | ||
| sbco &buffer_addr, DMEM1, WRITE_PTR_OFFSET, 2 | ||
| sbco &QPOS, DMEM1, CH_POS_OFFSET, 4 | ||
|
|
||
| ; Handle buffer wrap-around | ||
| qbgt no_buffer_wrap, buffer_addr, buffer_size | ||
| ldi buffer_addr, 0 | ||
| ldi READ_POS_BUFF, 0 |
There was a problem hiding this comment.
1. sbbo before buffer bounds 📘 Rule violation ⛨ Security
sbbo &time_stamp, DMEM0, buffer_addr, 4 stores to DMEM using buffer_addr before verifying that buffer_addr + 4 stays within buffer_size, and the wrap check only triggers on `buffer_addr > buffer_size (allowing buffer_addr == buffer_size`). This can write past the intended buffer end and corrupt adjacent PRU memory.
Agent Prompt
## Issue description
The PRU firmware writes a 4-byte timestamp to `DMEM0 + buffer_addr` before ensuring `buffer_addr` is within the valid buffer range, and the wrap condition allows `buffer_addr == buffer_size`.
## Issue Context
Compliance requires bounds checks for memory accesses, including validating `offset + size` against named limits.
## Fix Focus Areas
- examples/pru_eqep/firmware/main.asm[122-129]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| .asg r0, scratch | ||
| .asg r1, scratch2 | ||
| .asg r17.b0, curr_gpi_sample ; Current GPI state after masking | ||
| .asg r17.b1, A_B_Z_edge ; Edge detection for A,B,Z signals | ||
| .asg r18, curr_ts ; Timestamp counter | ||
| .asg r19.b0, edge ; Edge detection | ||
| .asg r20.b0, prev_sample ; Previous state of A,B,Z signals | ||
| .asg r21, time_stamp ; Time stamp counter | ||
| .asg r22.w0, buffer_addr ; Buffer address pointer | ||
| .asg r22.w2, buffer_size ; Buffer size | ||
| .asg r23, wr_ptr ; Write pointer | ||
| .asg r24.w0, READ_POS_BUFF ; Read position buffer | ||
| .asg r24.w2, read_pos_speed_intr ; Read position speed interrupt | ||
| .asg r26.b0, a_b_transition ; A/B transition state | ||
| .asg r26.b1, qpos_update ; 2-> QPOS--, 1-> QPOS++, 0-> no change | ||
| .asg r27, QPOS ; Position counter | ||
| .asg r28, DMEM0 ; Data memory pointer |
There was a problem hiding this comment.
2. Direct .asg register aliases 📘 Rule violation ⚙ Maintainability
The new PRU assembly include uses extensive direct register aliasing via .asg instead of .struct/.sassign. This increases risk of register overlap/corruption and makes calling/register conventions harder to audit.
Agent Prompt
## Issue description
The PRU assembly register map is implemented with many `.asg` aliases, contrary to the preferred `.struct`/`.sassign` pattern.
## Issue Context
The compliance rule requires structured register allocation to reduce overlap/corruption risk and improve maintainability.
## Fix Focus Areas
- examples/pru_eqep/firmware/include/memory.inc[33-49]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| postBuildStep=" | ||
| $(CG_TOOL_ROOT)/bin/hexpru --diag_wrap=off --array --array:name_prefix=PRU0Firmware -o ../pru0_load_bin.h ${BuildArtifactFileBaseName}.out; | ||
| " |
There was a problem hiding this comment.
3. Long postbuildstep command line 📘 Rule violation ⚙ Maintainability
New .projectspec files embed long command strings in a single line (for example, postBuildStep=...), which harms readability and violates the repository’s configuration line-length expectations. This makes future edits and reviews error-prone.
Agent Prompt
## Issue description
Project configuration includes very long single-line command attributes (notably `postBuildStep`), reducing maintainability and violating line-length guidance.
## Issue Context
The compliance rule requests wrapping/reflowing long command-like attributes where feasible.
## Fix Focus Areas
- examples/pru_eqep/firmware/am243x-lp/icss_g0_pru0_fw/ti-pru-cgt/example.projectspec[55-57]
- examples/pru_eqep/mcuplus/am243x-lp/r5fss0-0_freertos/ti-arm-clang/example.projectspec[70-70]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| - **When using CCS projects to build**, import the CCS project from the above mentioned Example folder path for R5F and PRU, After this `main.asm`, `linker.cmd` files gets copied to ccs workspace of PRU project. The `main.asm` contains sample code to halt PRU program | ||
|
|
||
| - Build the PRU project using the CCS project menu (see [for AM64x](https://software-dl.ti.com/mcu-plus-sdk/esd/AM64X/latest/exports/docs/api_guide_am64x/CCS_PROJECTS_PAGE.html), [for AM243x](https://software-dl.ti.com/mcu-plus-sdk/esd/AM243X/latest/exports/docs/api_guide_am243x/CCS_PROJECTS_PAGE.html), [for AM261x](https://software-dl.ti.com/mcu-plus-sdk/esd/AM261X/latest/exports/docs/api_guide_am261x/CCS_PROJECTS_PAGE.html), [for AM263x](https://software-dl.ti.com/mcu-plus-sdk/esd/AM263X/latest/exports/docs/api_guide_am263x/CCS_PROJECTS_PAGE.html), [for AM263Px](https://software-dl.ti.com/mcu-plus-sdk/esd/AM263PX/latest/exports/docs/api_guide_am263px/CCS_PROJECTS_PAGE.html)). | ||
| - Build Flow: Once you click on build in PRU project, firmware header file which is generated in release or debug folder of ccs workspace, is moved to `<open-pru/examples/empty/firmware/device/>` |
There was a problem hiding this comment.
4. readme.md wrong empty path 📘 Rule violation ≡ Correctness
The README references <open-pru/examples/empty/firmware/device/>, which does not match this example’s actual location and can mislead users. The same section also contains extremely long markdown lines, reducing portability/readability in rendered views and diffs.
Agent Prompt
## Issue description
The new README contains an incorrect repository path reference (points to `examples/empty` instead of `examples/pru_eqep`) and includes very long single-line markdown content.
## Issue Context
Incorrect path references break portability/usability of docs, and overly long markdown lines violate documentation formatting expectations.
## Fix Focus Areas
- examples/pru_eqep/readme.md[20-23]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| # pru_rules.mak has shared settings for all PRU/RTU/TX_PRU core makefiles | ||
| include $(OPEN_PRU_PATH)/pru_rules.mak | ||
| include $(OPEN_PRU_PATH)/examples/pru_eqep/firmware/include/memory.inc | ||
| # Optional: |
There was a problem hiding this comment.
5. Asm file included by make 🐞 Bug ≡ Correctness
The PRU0 firmware makefile uses GNU make include to include memory.inc, but that file contains assembly syntax (e.g., ; comments and .asg) and will be parsed as make syntax, causing the build to fail.
Agent Prompt
### Issue description
`examples/pru_eqep/firmware/am243x-lp/icss_g0_pru0_fw/ti-pru-cgt/makefile` includes an assembly `.inc` file via make's `include`, which will break parsing and stop the build.
### Issue Context
`memory.inc` is meant to be included by the PRU assembler (via `.include` in `main.asm` or via compiler include paths), not by `make`.
### Fix Focus Areas
- Remove the `include $(OPEN_PRU_PATH)/examples/pru_eqep/firmware/include/memory.inc` line.
- Ensure the assembler can still find `memory.inc` via `INCLUDE += --include_path=...`.
### Fix Focus Areas (code references)
- examples/pru_eqep/firmware/am243x-lp/icss_g0_pru0_fw/ti-pru-cgt/makefile[31-38]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| void ABZ_enable_load_share_mode(void *pruCfg, uint32_t pruSlice) | ||
| { | ||
| uint32_t regVal; | ||
| if(pruSlice == 1) | ||
| { | ||
| regVal = HW_RD_REG32((uint8_t *)pruCfg + CSL_ICSSCFG_EDPRU1TXCFGREGISTER_PRU1_ED_TX_CLK_SEL_MASK); | ||
| regVal |= CSL_ICSSCFG_EDPRU1TXCFGREGISTER_PRU1_ENDAT_SHARE_EN_MASK; | ||
| HW_WR_REG32((uint8_t *)pruCfg + CSL_ICSSCFG_EDPRU1TXCFGREGISTER, regVal); | ||
| } | ||
| else | ||
| { | ||
| regVal = HW_RD_REG32((uint8_t *)pruCfg + CSL_ICSSCFG_EDPRU0TXCFGREGISTER_PRU0_ED_TX_CLK_SEL_MASK); | ||
| regVal |= CSL_ICSSCFG_EDPRU0TXCFGREGISTER_PRU0_ENDAT_SHARE_EN_MASK; | ||
| HW_WR_REG32((uint8_t *)pruCfg + CSL_ICSSCFG_EDPRU0TXCFGREGISTER, regVal); | ||
| } |
There was a problem hiding this comment.
8. Mask used as register offset 🐞 Bug ≡ Correctness
ABZ_enable_load_share_mode() adds *_MASK constants to the PRU CFG base pointer when reading the EDPRU TX CFG register, but those constants are bitmasks (e.g., 0x10) rather than register offsets (e.g., 0xE4/0x104), so it reads the wrong address and may misconfigure load-share mode.
Agent Prompt
### Issue description
The code reads a PRU CFG register using a bitmask macro as if it were a register offset, so the value read is from the wrong location.
### Issue Context
`CSL_ICSSCFG_EDPRU1TXCFGREGISTER_PRU1_ED_TX_CLK_SEL_MASK` is 0x10 (mask), while `CSL_ICSSCFG_EDPRU1TXCFGREGISTER` is 0x104 (offset). The same pattern exists for PRU0.
### Fix Focus Areas
- Replace the `HW_RD_REG32(base + *_MASK)` reads with `HW_RD_REG32(base + CSL_ICSSCFG_EDPRU{0,1}TXCFGREGISTER)`.
- Keep using the `*_ENDAT_SHARE_EN_MASK` bitmask when setting/clearing bits in the read value.
### Fix Focus Areas (code references)
- examples/pru_eqep/mcuplus/pru_eqep_example.c[569-583]
- source/include/am243x/cslr_icss_g.h[13069-13071]
- source/include/am243x/cslr_icss_g.h[14785-14789]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ABZHandle[i]->QPOSCOUNT = 0; | ||
| ABZHandle[i]->read_ptr = ABZHandle[i]->baseMemAddr0; | ||
| ABZHandle[i]->write_ptr = ABZHandle[i]->baseMemAddr1; | ||
| ABZHandle[i]->mem_limit=(DMEM_CHANNEL_SIZE)*(i) + DMEM_CHANNEL_SIZE; | ||
| if(i<3) ABZHandle[i]->pru_slice=0; | ||
| else ABZHandle[i]->pru_slice=1; |
There was a problem hiding this comment.
10. Mem_limit scaled by channel 🐞 Bug ≡ Correctness
mem_limit is computed as DMEM_CHANNEL_SIZE*i + DMEM_CHANNEL_SIZE, but the PRU firmware write pointer (buffer_addr) is a per-channel offset that wraps to 0; for channels >0 this inflates wrap-around edge counts and produces incorrect speed.
Agent Prompt
### Issue description
Speed calculation uses `mem_limit` as the buffer end when the write pointer wraps, but `mem_limit` is incorrectly made larger for higher channel indices.
### Issue Context
`EQEP_Get_Speed_ABZ()` uses `mem_limit` only as an upper bound for `(mem_limit - read_ptr_offset + write_ptr_offset)` when `read_ptr_offset > write_ptr_offset` (wrap case). Since firmware resets `buffer_addr` back to 0 on wrap, `mem_limit` must be the fixed per-channel buffer size.
### Fix Focus Areas
- Change `ABZHandle[i]->mem_limit` to `DMEM_CHANNEL_SIZE` (or the correct per-channel buffer size constant), not scaled by `i`.
- Ensure the host-side buffer size constant matches the firmware’s `MAX_BUFFER`/wrap behavior.
### Fix Focus Areas (code references)
- examples/pru_eqep/mcuplus/pru_eqep_example.c[294-316]
- examples/pru_eqep/mcuplus/pru_eqep_example.c[485-510]
- examples/pru_eqep/firmware/main.asm[126-130]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (!(ABZHandle[0]->write_ptr_offset == (HW_RD_REG32(ABZHandle[0]->baseMemAddr1) & WRITE_PTR_OFFSET_MASK))) | ||
| { | ||
| EQEP_Get_Speed_ABZ(0); | ||
| } | ||
| else | ||
| { | ||
| while (ABZHandle[0]->write_ptr_offset == (HW_RD_REG32(ABZHandle[0]->baseMemAddr1) & WRITE_PTR_OFFSET_MASK)); | ||
| EQEP_Get_Speed_ABZ(0); |
There was a problem hiding this comment.
11. Spin loop inside isr 🐞 Bug ☼ Reliability
Each PRU host interrupt ISR busy-waits in a tight while (...) ; loop until the PRU updates the write pointer; if the PRU stalls or the pointer never changes, the ISR can run indefinitely and block other interrupts/tasks.
Agent Prompt
### Issue description
The interrupt handlers contain an unbounded spin-wait, which can lock the CPU in interrupt context.
### Issue Context
The ISR currently retries until `write_ptr_offset` changes. In failure modes (PRU halted, missed update, misconfigured mapping) this never terminates.
### Fix Focus Areas
- Avoid waiting inside the ISR; instead, read once and compute if data progressed, or defer work to a task.
- If waiting is necessary, add a bounded retry/timeout and record an error condition.
### Fix Focus Areas (code references)
- examples/pru_eqep/mcuplus/pru_eqep_example.c[389-420]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Took a detailed pass through this PR, verifying each item against the code, the CSL headers, and CI. Grouping the Qodo findings by whether they hold up, plus a couple of independent items. Confirmed — worth fixing (+1)
Disagree — please do not apply this one
Low priority / subjective
Independent items (not flagged by the bot)
Happy to re-verify once these are addressed — particularly a green |
|
thanks @pratheesh for grouping the comments, will review it carefully |
82a3d5f to
6eb4e38
Compare
| if (ABZHandle[channel]->prev_ts > ABZHandle[channel]->cur_ts) | ||
| ABZHandle[channel]->delta_t = TIMESTAMP_MASK - ABZHandle[channel]->prev_ts + ABZHandle[channel]->cur_ts; | ||
| else | ||
| ABZHandle[channel]->delta_t = ABZHandle[channel]->cur_ts - ABZHandle[channel]->prev_ts; |
There was a problem hiding this comment.
construct a case where delta_t becomes zero leading to division by zero, for timing calculations, only 24 bits are being considered currently
|
/agentic_describe |
| DebugP_log("EQEP Position Speed Test Started ...\r\n"); | ||
|
|
||
| // Main loop | ||
| uint64_t last_print_us = ClockP_getTimeUsec(); |
There was a problem hiding this comment.
can you explain this by adding comment ?
|
|
||
| /* Print status every 1 second */ | ||
| uint64_t now_us = ClockP_getTimeUsec(); | ||
| if ((now_us - last_print_us) >= 1000000U) |
There was a problem hiding this comment.
use #defines for hardcoded values 1000000
There was a problem hiding this comment.
also explain the flow in each branch what exactly is happening
| ABZHandle[ch]->prev_QPOS = ABZHandle[ch]->QPOSCOUNT; | ||
| ABZHandle[ch]->QPOSCOUNT = HW_RD_REG32((uint32_t)ABZHandle[ch]->position_base); | ||
| if (ABZHandle[ch]->QPOSCOUNT > ABZHandle[ch]->prev_QPOS) ABZHandle[ch]->direction = 1; | ||
| else if (ABZHandle[ch]->QPOSCOUNT < ABZHandle[ch]->prev_QPOS) ABZHandle[ch]->direction = -1; |
There was a problem hiding this comment.
if there are no transitions, aren't we supposed to bring direction to 0?
| ABZHandle[ch]->write_ptr_offset = curr_write_ptr_offset; | ||
| EQEP_Get_Speed_ABZ(ch); | ||
|
|
||
| PRUICSS_clearEvent(gPruIcssXHandle, 16U + ch); |
There was a problem hiding this comment.
use macros for hardcoded values 16U
4 participants
Signed-off-by: Ayushman a-ayushman@ti.com