Repository navigation
Conversation
PR Summary by QodoAdd mirror_input + spi_10mhz AM261x examples; tighten Flow Control labels and SPI timing
AI Description
Diagram
High-Level Assessment
Files changed (92)
|
Code Review by Qodo 1. Choosing Halt emits a JMP with no target
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
461a2a3 to
a8bf168
Compare
bb486a3 to
a69f359
Compare
pratheesh
left a comment
pratheesh
left a comment
There was a problem hiding this comment.
Thanks for this. The Flow Control jump-target work is a good addition. I generated and tested several designs against this head (a69f359) and found some problems I think should be fixed before merge. They're listed most severe first.
How I checked: SysConfig 1.28.0 CLI against the PR-head .metadata/product.json (AM261x_ZFG, icss_m1_pru0), with the generated asm assembled using clpru 2.3.3. I checked loop behaviour on the TI pru-simulator (cycle-accurate hardware-LOOP model) but have not tested it on silicon.
1. <Loop>_end "break" behaves like continue on finite loops
Loop_0_end: is emitted at the same address as endloop_0. A JMP to the LOOP end address triggers the hardware loop-back. I simulated the exact generated asm with a 5-count loop whose branch jumps to Loop_0_end. It still ran the body 5 times and then halted, where a real break would run it once. Infinite loops are fine, because their _end comes after a QBA and no hardware loop is involved.
Suggested fix: place the break target at an address other than endloop_N (for example one instruction past a trampoline), then confirm on silicon.
2. <Loop>_start re-arms the LOOP, so the documented "continue" pattern never terminates
The label sits between LDI and LOOP. The code comment says a jump there "resumes with the current counter value", but that isn't what happens: re-executing LOOP reloads the count from the register. The conditional_block/flow_control docs tell users to end both branches with JMP Loop_0_start. I simulated that pattern with GPI high and got 714 body entries in 5000 steps with no halt. Also, if the counter register gets reallocated inside the body, the count would be garbage.
3. The dropdown offers targets that are never emitted, and validation accepts them
getValidJumpTargets() lists _start/_end for every /pru_blocks/ instance. But addToPruRegisterAllocationSummary drops 0-cycle entries for blocks with an empty opCode (Memory Variable, LUT). A design with jumpTarget = "Mem_Var_start" passes SysConfig validation, and then clpru fails with E0300 undefined symbol Mem_Var_start. The docs say validation prevents exactly this failure. Unconnected blocks, which pushInstruction never visits, are probably affected the same way.
4. Breaking change: legacy Flow Control designs no longer load
opCode is now a getter-only hidden config. On this head, main's examples/empty/.../example.syscfg (which sets opCode = "HALT") fails with Error: cannot set 'opCode' to 'HALT'. The repo examples were migrated by deleting that line, which also changes HALT into JMP sysconfig_generated_end plus a label in main.asm. User designs have no migration path. Could the old setter be accepted and mapped, or could a migration note be added?
5. SPI Data Setup Time units change silently from cycles to ns
Existing designs will get a much shorter setup time: a value of 10 used to mean 10 cycles and now means 10 ns, which is 2 cycles at 200 MHz. CS Setup/Hold were labelled ns but were previously passed through as raw cycles. This PR fixes that bug, but it also changes behaviour (500 used to mean 2.5 µs and now means 500 ns). This needs a release note or migration.
6. The spi_10mhz example contradicts its README
- The syscfg has SCLK high=16, low=6, but the README says 14/6.
- PRU0 (the controller) leaves
pruClkFreqat its default of 200 while PRU1 sets 225. That gives SCLK = 9.09 MHz at 200 or 10.23 MHz at 225, never 10 MHz. - The caption says "1 bit = 10ns" (should be 100 ns).
- The README block table now says "SPI Transfer up to 7.69 MHz", which contradicts this 10 MHz SPI Transfer example.
7. If_Else_N_start comes after the input computations
The docs say "jump to an If/Else's _start to re-evaluate it". A jump there compares stale registers that may already have been reallocated, and it does not re-read the inputs.
8. branchTerminates ORs paths (from reading the code; not tested)
It accepts a branch if any next/data-consumer path reaches Flow Control, and it treats data-flow edges as control flow. The docstring says every path must reach it.
9. Small defects
- uart_tx_op: the input port displayName is
"output1"(copy-paste error). - spi_write docs still say MODE0 needs a minimum High of 2 for 25 MHz, but validation now allows High=1 (7 cycles, 28.57 MHz).
- mirror_input readme: the title is "SPI Transfer at 10MHz", it says
Loop_0_startwhere the config usesstartloop_0, and it references a pin table that doesn't exist. - The mirror R5F syscfg still has leftover SPI pinmux entries (PRU1 GPIO5/6/11, PRU0 GPIO2/7/9).
- The mirror main.c has dead AM64x/AM243x RTU/TX loads in an AM261x-only example.
10. Scope
The title says "mirror input example", but the PR also includes:
- the Flow Control/label redesign
- the SPI timing rework
- an AM261x CI SDK bump from 10.02.00.15 to 26.00.00.06.STS (mcelf boot images and
freertos.*libs, which breaks older SDKs; the README doesn't state the new minimum SDK) - new ZNC and ZFG_400 device variants
Splitting these into separate PRs would make review and bisecting much easier. The PR description is also empty.
Note: CI is green, but that only shows the shipped examples compile. None of them uses a finite-loop jump or a declaration-block jump target, which is where items 1 to 3 appear. The qodo bot also flagged #4. Its claim that the repo examples still set opCode is out of date at this head.
| * branches must recursively terminate the same way. | ||
| * Returns an error message, or null if the branch terminates correctly. | ||
| */ | ||
| function branchTerminates(headInst, ownerName, branchLabel) { |
There was a problem hiding this comment.
From reading the code: walk accepts the branch if any reachable next/data-consumer path hits Flow Control, and it treats output1 data edges as control flow. The docstring says every path must terminate (item 8).
a1248924
changed the title
5a3debf to
84bc91f
Compare
- add docs_ai signed-off by Ayushman <a-ayushman@ti.com>
- add option to jump to start of block from flow control - correct/update documentation for conditional, flow control, loop block - enforce graph of blocks termination with flow control - add warning for unreachable blocks - add details on how to implement break/continue for loop signed-off by Ayushman <a-ayushman@ti.com>
- update r5f code to use latest sdk - remove spi_10mhz example, to be added as seperate PR signed-off by Ayushman <a-ayushman@ti.com>
84bc91f to
763a2fa
Compare
- update readme signed-off by Ayushman <a-ayushman@ti.com>
763a2fa to
30dbcaa
Compare
|
/agentic_review |
pratheesh
left a comment
pratheesh
left a comment
There was a problem hiding this comment.
Thanks for the update. Several things are clearly better: the Flow Control target list is now filtered to labels that are actually emitted, so the Memory Variable case is rejected in SysConfig instead of failing in the assembler; the If/Else check that every connected branch ends in a Flow Control is a good addition and works on the shipped conditional example; and the scope is much tighter now that the SPI and mirror/10 MHz example changes are out.
A few things still need attention:
- Breaking out of a finite loop. Jumping to the start of the block connected after the loop (as the new docs describe) still behaves like a continue. In the generated assembly that block's
_startlabel sits at the same address asendloop_N, so the hardware loop-back still triggers. I simulated a 5-count loop with an If/Else whose true branch clears an output and jumps to the next block's_start: the clear executed 5 times instead of once, and the next block ran only after the fifth pass. Infinite loops are fine. Could the exit target be placed past the loop end (for example one instruction after, or via a small trampoline), ideally with an example or test that breaks out of a finite loop? Worth confirming on hardware too. - Continue in a finite loop. The docs now correctly say that jumping to
<Loop>_startre-armsLOOP, but the option is still offered for finite loops without any warning. In simulation a Flow Control that jumps there never terminates. Consider hiding it for finite loops, or warning when it is selected. - Existing designs. Designs saved with
opCode = "HALT"still fail to load (cannot set 'opCode' to 'HALT'). A short migration note, or acceptingopCodeand mapping it tojumpTarget, would avoid breaking saved projects. - New validation false positives. A finite loop that is the last block (no
next) is reported as "Disconnected chunk ... missing Flow Control termination" even when its body ends in Flow Controls. The "Unreachable chunk" warnings are based on a single fixed-input simulation, so correct If/Else designs, including the shipped conditional example, show warnings for the branch the simulated input doesn't take. Static reachability, or softer wording, would avoid noise. - Branch termination check.
branchTerminatesstill accepts a branch if any one next/data-consumer path reaches a Flow Control. Requiring every control-flow path (and not counting data-flow edges) would match the comment and the "run both branches" failure it is meant to prevent. - Minor: README and the error text still mention
_endtargets in the dropdown although they were removed;If_Else_N_startsits after the input computations, so jumping there re-compares stale registers (worth a sentence in the docs); the description is still empty, and thedocs_ai/and AGENTS.md changes might be easier to review as their own PR.
|
regarding this comment : lets say we have this block combination: i think we dont expect the user to connect every flow control using next/prev and for if/else blocks specifically the inputs are two for the block and it has only one prev port so the user cannot connect both the branches next to the if/else's prev that is why for if/else block we have a seperate check that both T/F branch should end in a flow control and it behaves as expected, the check for a chunk of blocks to end in flow control is seperate than this |
|
for the loop block issue not going out and re-entering the loop, i have added a NOP after the loop block |
… not overlap with endloop - update documentation - terminating loop block may not have flow control termimation - disable jumping to loop_start if using finite loop - add opCode="HALT" support so that previous designs do not fail signed-off by Ayushman <a-ayushman@ti.com>
b6ed027 to
c4c3445
Compare
|
/agentic_review |
| handled = walk(nextInst) || handled; | ||
| } | ||
| for (const consumer of outputConsumers) { | ||
| handled = walk(consumer) || handled; | ||
| } |
There was a problem hiding this comment.
3. An unterminated branch passes validation 🐞 Bug ≡ Correctness
branchTerminates combines the executable next path and data-only output1 consumers with an OR, so one consumer reaching Flow Control masks a dead end on next. If a branch block sends data to Flow Control but its next chain ends without a terminator, validation accepts the branch although execution can fall through.
Agent Prompt
## Issue description
A data consumer can conceal an unterminated executable conditional branch.
## Fix Focus Areas
- .metadata/sysconfig/.meta/pru_blocks/program_control/conditional_block.syscfg.js[19-71]
## Recommended Fix
Determine termination from executable control-flow successors, requiring each executable path to terminate; do not let an output1 data consumer override a failing next path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // Unreachable-chunk warnings: only for block start labels, once globally | ||
| for (const lbl of emittedLabels) { | ||
| if (!visitedLabels.has(lbl)) { | ||
| const isBlockStart = lbl.endsWith('_start') || lbl.startsWith('startloop_'); | ||
| // Skip endloop labels: unvisited endloop is normal (loop exits after iterations) | ||
| if (isBlockStart && !warnedLabelsGlobal.has(lbl)) { |
There was a problem hiding this comment.
6. Group entry points get false warnings 🐞 Bug ◔ Observability
The new unreachable-label check warns on every unvisited _start label, including group entry points. Groups are emitted for explicit calls from application assembly rather than reached from the simulator’s default entry point, so a normal simulation warns about valid groups.
Agent Prompt
## Issue description
Normal groups produce misleading unreachable-label warnings.
## Fix Focus Areas
- .metadata/sysconfig/.meta/pru_blocks/common/pru_blocks_static_module.syscfg.js[1247-1271]
## Recommended Fix
Identify group entry labels and exclude them from warnings based solely on the default simulation path.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
valid, will exclude group blocks
| if ((String(inst["jumpTarget"]).includes('Loop_') || String(inst["jumpTarget"]).includes('loop_')) && String(inst["jumpTarget"]).includes('start')) { | ||
| const jt = String(inst["jumpTarget"]); | ||
| const match = jt.match(/Loop_(\d+)/); | ||
| if (match) return `endloop_${match[1]}`; | ||
| return jt.replace(/_start$/, '').replace(/^Loop_/, 'endloop_'); | ||
| } |
There was a problem hiding this comment.
5. Jumps to blocks named with 'loop' go astray 🐞 Bug ≡ Correctness
constant1 rewrites any jump target whose text contains "Loop_" or "loop_" plus "start" as if it were a finite loop. Names matching Loop_(\d+) become endloop_N; any other name has _start removed and becomes a bare label that the code never emits. A user block named, say, Loop_1_delay or main_loop_gpo (both default-style names) therefore jumps to some loop's end, or to an undefined symbol that breaks assembly, instead of to the selected block's entry.
Agent Prompt
## Issue description
The Flow Control `constant1` getValue remaps any target containing "Loop_"/"loop_" and "start", so ordinary blocks with such names get jumps to the wrong label or to an undefined one.
## Fix Focus Areas
- .metadata/sysconfig/.meta/pru_blocks/program_control/flow_control_block.syscfg.js[238-250]
- .metadata/sysconfig/.meta/pru_blocks/program_control/flow_control_block.syscfg.js[42-57]
## Recommended Fix
Only remap a target when it exactly equals `${loopInst.$name}_start` for a finite loop instance. Find that instance in `system.modules["/pru_blocks/program_control/loop_block"].$instances` and map to `endloop_${index}`. Pass every other target through unchanged. Also build the finite-loop dropdown entry from `inst.$name` rather than `Loop_${i}`, so renamed loops still appear.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| main: | ||
| JMP sysconfig_generated_start | ||
| sysconfig_generated_end: |
There was a problem hiding this comment.
8. Example prus run past the end of main 🐞 Bug ☼ Reliability
The empty and spi_loopback examples removed opCode = "HALT" while leaving Flow Control to jump to sysconfig_generated_end, a label at the end of main.asm with no stopping instruction. When that jump executes, the PRU passes the label and falls through beyond .text:main into the following .text section, which contains generated code, instead of stopping at the former termination point.
Agent Prompt
## Issue description
The empty and spi_loopback firmware examples now jump to an end label that has no stopping instruction, rather than halting as they did before.
## Fix Focus Areas
- examples/spi_loopback/firmware/main.asm[37-37]
- examples/empty/firmware/main.asm[38-38]
- examples/empty/firmware/am243x-evm/icss_g0_pru0_fw/example.syscfg[15-19]
- examples/empty/firmware/main.asm[34-38]
## Recommended Fix
Add a `halt` immediately after `sysconfig_generated_end:` in both examples' `main.asm` files, or set their active Flow Control `jumpTarget = "HALT"` in the syscfg files. Verify the other examples whose HALT assignments were removed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| * References between modules. | ||
| */ | ||
| loop_block1.$groupContents = [load_constant_block1, load_constant_block2, arithmetic_block1, pru_spi_write1]; | ||
| loop_block1.$groupContents = [load_constant_block1, load_constant_block2, arithmetic_block1, conditional_block1, load_constant_block3, flow_control_block1, flow_control_block2]; |
There was a problem hiding this comment.
9. Am261x incremental spi example sends no spi 🐞 Bug ≡ Correctness
The am261x incremental_spi firmware design removed the pru_spi_write block and replaced it with an If/Else plus two Flow Control blocks. It also raised loopCount to 15, so the loop now only increments a register and never transmits. The example README and the R5F side still describe sending 1..10 over SPI, so on AM261x the example no longer demonstrates what it is named for.
Agent Prompt
## Issue description
The AM261x incremental_spi firmware no longer contains a SPI Write block, so nothing is transmitted.
## Fix Focus Areas
- examples/incremental_spi/firmware/am261x-lp/icss_m1_pru0_fw/example.syscfg[12-76]
## Recommended Fix
Put `PRU0_SPI_Write_0` back in the loop (with its CS signal setting), fed from `Arithmetic_0.output1`, and end its chain with a Flow Control "continue" as the am243x variant does. Restore `loopCount = 10` to match the README. Or, if the new behaviour is intended, update the README.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| ```javascript | ||
| function getLongDescription() { ... } (see source file for full definition) | ||
| ``` | ||
|
|
||
| Source file: utils\look_up_table.syscfg.js No newline at end of file |
There was a problem hiding this comment.
11. Ai docs for two blocks are empty stubs 🐞 Bug ⚙ Maintainability
docs_ai/utils/look_up_table.md and docs_ai/utils/data_splitter.md contain only a placeholder
(function getLongDescription() { ... }), even though both blocks' long descriptions now tell the
AI to read these files first. The getAIContext content for the lookup table, which held the
parameter tables, valid values and examples, was deleted in the same change. The AI assistant
therefore has no configuration reference for these blocks.
Agent Prompt
## Issue description
The docs_ai files for look_up_table and data_splitter are placeholders, while the block descriptions send the AI to them.
## Fix Focus Areas
- docs_ai/utils/look_up_table.md[1-5]
- docs_ai/utils/data_splitter.md[1-5]
## Recommended Fix
Write full docs (parameters, valid values, example scripting snippets, connection notes) in the same format as the other docs_ai files. For look_up_table, reuse the removed `getAIContext` content.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
3 participants
No description provided.