Clean up setGeometry (main)#5713
Conversation
Reorganize setGeometry to perform all checks first before applying the values. This lets us accurately detect changes before starting a transition.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough
ChangesSegment Geometry Refactor
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@wled00/FX_fcn.cpp`:
- Around line 466-468: Clamp oversized stop values in the segment handling logic
so 2D matrix segments cannot extend past the matrix domain; the current `i2`
adjustment in `FX_fcn.cpp` allows `stop` to reach `strip.getLengthTotal()` even
when the segment starts in the matrix area, which can make `Segment::length()`
exceed the framebuffer bounds. Update the `i2` clamping branch in this
segment-stop calculation to limit by the segment’s matrix dimensions first, and
only permit total-length stops for trailing 1D segments, using the existing
`Segment` and `strip` checks around the `i2` logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8e5c94a2-e9df-4bbb-8b12-4e9a710fd1a7
📒 Files selected for processing (1)
wled00/FX_fcn.cpp
| #ifndef WLED_DISABLE_2D | ||
| if (Segment::maxHeight>1) { // 2D | ||
| if (i1Y >= Segment::maxHeight) i1Y = startY; | ||
| if (i1Y >= Segment::maxHeight) i1Y = 0; // Unlike i1, doesn't inherit old value |
There was a problem hiding this comment.
can you explain why one does inherit and the other does not? in what scenario does this even happen? I imagine if loading a preset with oob segment settings?
There was a problem hiding this comment.
Historical compatibility, ie. that's what the code prior to this PR did. I agree it's a strange semantic. Like so many API quirks I imagine it was an early implementation decision that stuck around. The tweak here was because I misread the original implementation - in the original code, Y was always reset to 0 if invalid, while X keeps the old value.
I ended up with a little test script to validate that the API was consistent with main, except fixing the crash and notification bugs. Sadly there is one incompatibility I left in here as I judged it wasn't worth the effort to fix: if you have a system with a matrix + trailing strip, and you send an update to a segment that was in the trailing strip with a trailing-strip stop, but matrix start index; then the original code would disable the segment, but this code keeps the old start value.
eg. for a system with a 10x10 matrix followed by a 100 LED strip:
original segment: { "start": 120, "len": 60 } (stop = 180)
is sent a bad update: { "start": 2 } (unlike every other value, omitting "len" doesn't keep the length the same, it keeps the stop coord the same...!!!)
The old code would produce:
{ "start": 120, "len": 0 } (disabled)
while the new code will still land at:
{ "start": 120, "len": 60}
There was a problem hiding this comment.
thanks for the explanation, makes sense to me. Matrix with trailing strip is a bit of a special case I assume not many people use.
could you please update the comment to // Unlike i1, doesn't inherit old value (legacy) for clarity?
|
@coderabbitai is this PR compatible with #5729 ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="wled/WLED"
echo "=== PR metadata ==="
for pr in 5713 5729; do
gh api "repos/${repo}/pulls/${pr}" \
--jq '"PR #\(.number): \(.title)\nbase=\(.base.ref)@\(.base.sha)\nhead=\(.head.ref)@\(.head.sha)\nmergeable=\(.mergeable) mergeable_state=\(.mergeable_state)\n"'
done
echo "=== Changed files and patch excerpts ==="
for pr in 5713 5729; do
echo "--- PR #${pr} ---"
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "FILE: \(.filename) status=\(.status)\n\(.patch // "[no textual patch]")\n"'
done
echo "=== Commit ancestry / comparisons ==="
sha5713="$(gh api "repos/${repo}/pulls/5713" --jq '.head.sha')"
sha5729="$(gh api "repos/${repo}/pulls/5729" --jq '.head.sha')"
base5713="$(gh api "repos/${repo}/pulls/5713" --jq '.base.sha')"
base5729="$(gh api "repos/${repo}/pulls/5729" --jq '.base.sha')"
for label in "5713...5729:${sha5713}...${sha5729}" "base5713...5729:${base5713}...${sha5729}" "base5729...5713:${base5729}...${sha5713}"; do
name="${label%%:*}"
range="${label#*:}"
echo "--- compare ${name} ---"
gh api "repos/${repo}/compare/${range}" --jq '"status=\(.status) ahead_by=\(.ahead_by) behind_by=\(.behind_by) total_commits=\(.total_commits)"' || true
doneLength of output: 11729 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="wled/WLED"
pr5713_base="14709dd3ea9fca9e5164509772f6c75185e3ab63"
pr5713_head="acb9487fdbde3d1c74ad8ad38f796c8ab7e3d5cb"
pr5729_base="bc2c80d985d09cac2f9f912f6b14ee9f71f4a9bd"
pr5729_head="72692e533bbeedf1ecbd8798c1fdea5ec18f0edd"
main_sha="$(gh api "repos/${repo}/git/ref/heads/main" --jq '.object.sha')"
echo "=== PR commit lists ==="
for pr in 5713 5729; do
echo "--- PR #${pr} ---"
gh api --paginate "repos/${repo}/pulls/${pr}/commits?per_page=100" \
--jq '.[] | "\(.sha[0:12]) \(.commit.message | split("\n")[0])"'
done
echo "=== Is the `#5713` patch represented after its head? ==="
gh api "repos/${repo}/compare/${pr5713_head}...${pr5729_base}" \
--jq '"status=\(.status) ahead_by=\(.ahead_by) behind_by=\(.behind_by)\nfiles:\n" + ([.files[] | .filename] | join("\n"))'
echo "=== setGeometry signatures/comments at `#5729` base and current main ==="
for ref in "$pr5713_head" "$pr5729_base" "$main_sha"; do
echo "--- ${ref} ---"
gh api "repos/${repo}/contents/wled00/FX_fcn.cpp?ref=${ref}" --jq -r '.content' \
| base64 --decode \
| sed -n '/void Segment::setGeometry(/,/^}/p' \
| sed -n '1,145p'
doneLength of output: 2159
The only caveat is normal branch freshness: rebase |
3 participants
Reorganize setGeometry to perform all checks first before applying the values. This lets us accurately detect changes before starting a transition.
Forward port from #5474.
Sorry for the noise, I accidentally pushed this directly to main while setting up the branch. :(
Summary by CodeRabbit