Spi driver demo - #161
Open
a1248924 wants to merge 1 commit into
Open
Spi driver demo#161a1248924 wants to merge 1 commit into
a1248924 wants to merge 1 commit into
Conversation
PR Summary by QodoAdd AM261x PRU SPI driver demonstration
AI Description
Diagram
High-Level Assessment
Files changed (24)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
a1248924
force-pushed
the
SPI_driver_demo
branch
from
August 18, 2026 06:46
45b14ca to
c7b425c
Compare
Comment on lines
+169
to
+170
| DebugP_log("\r\nEnter command (0-3): "); | ||
| DebugP_scanf("%d", &input); |
There was a problem hiding this comment.
2. Failed input reuses command 🐞 Bug ≡ Correctness
empty_example_main ignores whether DebugP_scanf successfully parsed a value and then validates and dispatches the uninitialized or previously retained input. Malformed or unavailable console input can therefore trigger an unintended SPI transaction, and an unconsumed invalid token can repeat this behavior.
Agent Prompt
## Issue description
The command loop uses `input` even when console parsing fails, allowing an indeterminate or stale command to be sent.
## Issue Context
Initialize the input variable, check the `DebugP_scanf` result before range validation, and consume or otherwise handle invalid input so the same malformed token does not repeatedly fail.
## Fix Focus Areas
- examples/spi_driver/mcuplus/empty_example.c[131-133]
- examples/spi_driver/mcuplus/empty_example.c[167-180]
- examples/spi_driver/mcuplus/empty_example.c[182-189]
ⓘ 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 c7b425c |
a1248924
force-pushed
the
SPI_driver_demo
branch
from
August 19, 2026 07:14
c7b425c to
a278e17
Compare
Comment on lines
+161
to
+164
| if (elapsed >= TRIGGER_POLL_TIMEOUT_US) | ||
| { | ||
| return 0; | ||
| } |
There was a problem hiding this comment.
1. Timeout desynchronizes transactions 🐞 Bug ☼ Reliability
PRU_SPI_runTransaction5 returns on timeout while leaving the outstanding trigger active, and the caller immediately permits another transaction. If the timed-out PRU operation later clears that shared trigger after the retry sets it, the retry reports success and reads responses from the previous transaction.
Agent Prompt
## Issue description
A timed-out SPI transaction remains active because the shared trigger is left set. A late PRU completion can clear the trigger belonging to a subsequent retry, causing stale responses to be accepted as current.
## Issue Context
The PRU clears the same trigger only after publishing its responses, while the application continues its input loop after a timeout. Recovery must not simply clear the trigger while the PRU may still be processing; either stop/reinitialize the PRU transaction state or terminate further transactions, or introduce a sequence-based handshake that distinguishes completions.
## Fix Focus Areas
- examples/spi_driver/mcuplus/empty_example.c[156-172]
- examples/spi_driver/mcuplus/empty_example.c[232-235]
- examples/spi_driver/firmware/am261x-lp/icss_m1_pru0_fw/main.asm[365-375]
ⓘ 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 a278e17 |
a1248924
force-pushed
the
SPI_driver_demo
branch
from
August 19, 2026 07:26
a278e17 to
dc18a4d
Compare
| while (i < 5U) | ||
| { | ||
| DebugP_log(" cmd[%u]: ", i); | ||
| DebugP_scanf("%x", &input); |
There was a problem hiding this comment.
1. Decimal input parsed hexadecimal 🐞 Bug ≡ Correctness
The prompt requests decimal command words, but DebugP_scanf("%x", &input) parses them as
hexadecimal. For example, entering decimal 10 sends 0x10 (16), causing the SPI transaction to
use a different command than the user requested.
Agent Prompt
## Issue description
The console requests decimal command words but parses input using the hexadecimal `%x` conversion, so digit-only values can be transmitted with the wrong numeric value.
## Issue Context
Either parse decimal input with `%u`, or explicitly change the prompt and validation messages to request hexadecimal input.
## Fix Focus Areas
- examples/spi_driver/mcuplus/empty_example.c[215-224]
ⓘ 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 dc18a4d |
a1248924
force-pushed
the
SPI_driver_demo
branch
from
August 19, 2026 07:35
dc18a4d to
41a382f
Compare
|
Code review by qodo was updated up to the latest commit 41a382f |
a1248924
force-pushed
the
SPI_driver_demo
branch
from
September 13, 2026 13:46
41a382f to
394ef65
Compare
Comment on lines
+124
to
+125
| dmem_write32(PRU0_DMEM_BASE, DMEM_CFG_COMMAND, commandWord); | ||
| dmem_write32(PRU0_DMEM_BASE, DMEM_CFG_TRIGGER, 1U); |
There was a problem hiding this comment.
4. Single-word transfers return stale data 🐞 Bug ≡ Correctness
PRU_SPI_runTransaction writes the obsolete command slot at 0x0C and reads the obsolete response slot at 0x08. Every PRU0 branch now consumes five commands from 0x14–0x24 and produces responses at 0x28–0x38, so invoking this retained helper triggers a burst from unrelated slots and returns a value PRU0 never updates.
Agent Prompt
## Issue description
`PRU_SPI_runTransaction` still accesses the legacy single-word DMEM slots, but the new PRU0 firmware only implements five-word bursts using different slots. Calling the helper therefore sends unrelated data and reads a stale response.
## Fix Focus Areas
- examples/spi_driver/mcuplus/empty_example.c[120-140]
- examples/spi_driver/mcuplus/empty_example.c[47-64]
## Recommended Fix
Remove `PRU_SPI_runTransaction` and the unused legacy single-word constants and comments. If single-word operation must remain supported, implement an explicit adapter that populates all required burst slots and returns the corresponding active response, or restore a matching firmware protocol.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| * | ||
| * SPDX-License-Identifier: BSD-3-Clause | ||
| * | ||
| * PRU-SPI encoder-emulation demo - R5F side (phase 3, MODE3/LSB only) |
There was a problem hiding this comment.
9. Source guidance names the wrong bit order 🐞 Bug ⚙ Maintainability
The application header describes the demo and bundled slave as MODE3/LSB even though initialization and the slave transfer routine both select MODE3/MSB. A maintainer following those source notes while changing or debugging configuration can pair the peers with opposite bit ordering, despite the project README documenting MSB correctly.
Agent Prompt
## Issue description
Several source comments describe the active demo as MODE3/LSB and claim only that path uses five-word bursts, while the executable configuration and slave firmware use MODE3/MSB and PRU0 implements five-word bursts for all eight combinations.
## Fix Focus Areas
- examples/spi_driver/mcuplus/empty_example.c[6-32]
- examples/spi_driver/firmware/am261x-lp/icss_m1_pru1_fw/main.asm[120-123]
- examples/spi_driver/firmware/am261x-lp/icss_m1_pru0_fw/main.asm[84-94]
## Recommended Fix
Update the comments to identify MODE3/MSB as the bundled master/slave configuration and state that all PRU0 mode and bit-order branches use the five-word protocol. Remove references to unchanged legacy single-transfer branches.
ⓘ 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 394ef65 |
a1248924
force-pushed
the
SPI_driver_demo
branch
from
September 13, 2026 13:53
394ef65 to
cbef2f2
Compare
|
Code review by qodo was updated up to the latest commit cbef2f2 |
a1248924
force-pushed
the
SPI_driver_demo
branch
from
September 13, 2026 13:56
cbef2f2 to
ff049dd
Compare
Comment on lines
+153
to
+156
| m_transfer_packet_spi_slave_gpi_sclk rx0, tx0, PACKET_SIZE, bitId, SCLK_PIN, SDI_PIN, SDO_PIN, "MODE3", "MSB" | ||
| m_transfer_packet_spi_slave_gpi_sclk rx1, tx1, PACKET_SIZE, bitId, SCLK_PIN, SDI_PIN, SDO_PIN, "MODE3", "MSB" | ||
| m_transfer_packet_spi_slave_gpi_sclk rx2, tx2, PACKET_SIZE, bitId, SCLK_PIN, SDI_PIN, SDO_PIN, "MODE3", "MSB" | ||
| m_transfer_packet_spi_slave_gpi_sclk rx3, tx3, PACKET_SIZE, bitId, SCLK_PIN, SDI_PIN, SDO_PIN, "MODE3", "MSB" |
There was a problem hiding this comment.
4. Aborted bursts desynchronize the slave 🐞 Bug ☼ Reliability
DO_5X_TRANSFER invokes transfer macros that wait only for clock transitions and cannot abort when chip select becomes inactive. If chip select is deasserted before all 80 clocks arrive, later bursts finish the interrupted transfer at the wrong boundary and the slave remains offset until its core is restarted.
Agent Prompt
## Issue description
PRU1 cannot abandon a partial five-word transfer when chip select becomes inactive, so subsequent bursts remain aligned to the interrupted transaction rather than their own boundaries.
## Fix Focus Areas
- examples/spi_driver/firmware/am261x-lp/icss_m1_pru1_fw/main.asm[116-157]
- examples/spi_driver/firmware/am261x-lp/spi_slave_macros.inc[362-410]
## Recommended Fix
Make the slave transfer path monitor the chip-select input while waiting for each clock edge and return an aborted status when it rises. On abort, discard partial receive data, wait for a fresh high-to-low chip-select transition, reinitialize all five transfer registers, and begin the next burst at word zero.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+138
to
+141
| ### Fixed at firmware build time (edit `firmware/am261x-lp/icss_m1_pru0_fw/main.asm` and rebuild) | ||
|
|
||
| - **Packet size** — fixed at 16 bits (`PACKET_SIZE`). The underlying macros | ||
| (`spi_master_macros.inc`/`spi_slave_macros.inc`) enforce **1-32 bits** at |
There was a problem hiding this comment.
5. Firmware customization breaks the demo 🐞 Bug ≡ Correctness
The fixed-configuration section directs users to edit only PRU0 firmware even though packet size and pin assignments are independently defined by PRU1 and the pin routing is generated from SysConfig. Changing either advertised setting by following these instructions leaves the two endpoints or physical routing inconsistent, preventing the customized round trip from operating correctly.
Agent Prompt
## Issue description
The customization instructions name only PRU0 firmware, but packet-size and pin changes also require corresponding PRU1 firmware and SysConfig updates.
## Fix Focus Areas
- examples/spi_driver/readme.md[138-150]
## Recommended Fix
Split the build-time settings by component. State that packet-size changes must be applied consistently to both PRU firmware files, pin changes must update the relevant PRU constants and SysConfig routing before regeneration, and only the master timing compensation values are confined to PRU0.
ⓘ 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 ff049dd |
signed-off by Ayushman <a-ayushman@ti.com>
a1248924
force-pushed
the
SPI_driver_demo
branch
from
September 13, 2026 14:11
ff049dd to
3f78d69
Compare
| ; Returns: | ||
| ; - The received data packet is stored in the `dataReg` register. | ||
| ;************************************************************************************ | ||
| m_read_packet_spi_slave_lsb_gpo_sclk .macro dataReg, PACKETSIZE, bitId, SCLK_PIN, SDI_PIN, MODE |
There was a problem hiding this comment.
4. Slave low-bit-first reads fail to build 🐞 Bug ≡ Correctness
m_read_packet_spi_slave_lsb_gpo_sclk is defined with gpo even though its own heading and the sibling MSB read macro expose the slave input-clock API as gpi. Any firmware that calls the documented LSB-only slave read name fails assembly with an undefined macro, while the other bit order builds.
Agent Prompt
## Issue description
The low-bit-first slave read macro is documented with a `gpi_sclk` name but defined with `gpo_sclk`, so callers using the documented and convention-consistent name cannot assemble.
## Fix Focus Areas
- examples/spi_driver/firmware/am261x-lp/spi_slave_macros.inc[115-147]
## Recommended Fix
Rename `m_read_packet_spi_slave_lsb_gpo_sclk` to `m_read_packet_spi_slave_lsb_gpi_sclk`, matching the macro heading, the input-clock implementation, and the corresponding MSB macro.
ⓘ 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 3f78d69 |
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.
No description provided.