Repository navigation
Conversation
PR Summary by QodoFix PRU conditional fallthrough and add block jump targets
AI Description
Diagram
High-Level Assessment
Files changed (108)
|
Code Review by Qodo 1. Simulated loop breaks keep iterating
|
![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.
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 |
b6ed027 to
c4c3445
Compare
|
/agentic_review |
… 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>
c4c3445 to
2ce605f
Compare
|
/agentic_review |
pratheesh
left a comment
pratheesh
left a comment
There was a problem hiding this comment.
At 2ce605f the NOP after endloop_N fixes the finite-loop break. I generated a design with a Flow Control inside a 10-count loop that jumps to the next block's _start. The target lands one instruction past endloop_N (generated asm only; not simulated or run on hardware). Continue now maps to endloop_N, and the last-block validation and the softer warning wording look fine. The SysConfig sanity tests pass with 0 errors on AM261x and AM64x, both PRUs.
Still needed:
- Saved designs that use
opCode = "HALT"still fail to load:cannot set 'opCode' to 'HALT'. I reproduced it with the conditional example plus that one line, using SysConfig 1.28.0 CLI (halt.syscfg:37). The commit message says HALT support was added, but theopCodeoption is hidden behind agetValueoverride, so it can't be set. Either accept it and map it tojumpTarget, or document the migration. - Add an example or test that breaks out of a finite loop. Nothing covers it yet.
- Minor:
flow_control_block.syscfg.js:197still mentions_endtargets.- The PR description is empty.
- The
docs_ai/AGENTS.mdchanges would be easier to review in their own PR.
I'm dropping item 5 (branchTerminates accepting any path). Your explanation makes sense.
On your 11-02 note about removing simulation from NoCode now that pru-simulator integration is coming: which way is that going? Several of the validation warnings rely on the built-in simulation, so it changes how they should work.
No description provided.