Add ESO gain optimization + report generation#136
Open
nerdCopter wants to merge 6 commits intomasterfrom
Open
Conversation
- Add src/eso.rs: 2nd-order LESO bandwidth optimization via golden-section search - Add src/report.rs: Per-axis signal statistics and ESO results in markdown format - Add CLI flags: --eso, --eso-axis, --eso-b0, --report - Add ESO constants to src/constants.rs (omega_0 bounds, GSS tolerance) - Update lib.rs and main.rs to wire modules and implement ESO/report execution - Update OVERVIEW.md with ESO and report sections - Add INFORMATION/ESO_HOWTO.md user guide - Tested on Betaflight 2025.12 and EmuFlight 0.4.3 full flights
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 29 minutes and 20 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (10)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an optional 2nd-order LESO bandwidth optimizer and optional Markdown statistical reporting, new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@OVERVIEW.md`:
- Line 207: The list under the "Generated PNG Plots" heading incorrectly
includes the bullet "`*_report.md`" (a Markdown statistical report); remove that
bullet from this PNG outputs list and either move the "`*_report.md`" entry into
a new or existing separate outputs subsection (e.g., "Other outputs" or
"Generated Reports") or rename the "Generated PNG Plots" heading to a more
general title so the report file logically belongs there; ensure the change
updates only the OV ERVIEW.md section referencing "Generated PNG Plots" and the
"`*_report.md`" bullet so the PNG list contains only image outputs.
In `@src/eso.rs`:
- Around line 151-158: The loop blindly zero-fills missing P/I/D/F terms and
accepts axes with no real control excitation; change the logic in the loop that
builds omega_meas and pid_sum (references: row.gyro[axis], row.p_term[axis],
row.i_term[axis], row.d_term[axis], row.f_term[axis], omega_meas, pid_sum) to
detect and reject axes where the control input is not usable:
skip/presume-invalid if all of p_term/i_term/d_term/f_term are None or if the
resulting pid_sum is effectively constant/zero (e.g., all entries equal or
variance below a small threshold), and surface that fact to the caller (return
an error or None) instead of returning an ESO result computed from zero-filled
inputs. Ensure this guard runs before running the optimizer so only axes with
real excitation are optimized.
- Around line 177-199: The public run_eso_optimization must validate its inputs
before using them: check that axis is within valid range (0 <= axis <
AXIS_COUNT) and return a descriptive Err instead of allowing extract_axis_data
to panic; validate sample_rate is finite and > 0; validate numeric config fields
(EsoConfig::omega0_min, EsoConfig::omega0_max, EsoConfig::b0) are finite and
that omega0_max > omega0_min; then compute omega0_max_stable as before and
proceed to call extract_axis_data and mse_cost. Update run_eso_optimization to
perform these checks (referencing run_eso_optimization, AXIS_COUNT,
extract_axis_data, and EsoConfig::omega0_min/omega0_max/b0) and return early Err
results with clear messages when validations fail.
In `@src/report.rs`:
- Around line 67-76: The code currently uses unwrap_or(0.0) on
row.p_term/i_term/d_term/f_term inside the gyro gate so missing PID fields are
recorded as real zeros; remove the unwrap_or(0.0) usage and instead test each
Option and only push PID samples when the Option is Some (e.g. if let Some(p) =
row.p_term[axis] { pid_p.push(p) } ), and compute/push pid_sum only when all
required components are present (e.g. only when p, i, d, f are Some); this
ensures row.gyro is still collected but missing PID fields are not misreported
as zero-valued samples.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08ac2d4c-95fd-4948-88ac-a47b23f153b8
📒 Files selected for processing (6)
OVERVIEW.mdsrc/constants.rssrc/eso.rssrc/lib.rssrc/main.rssrc/report.rs
Comment on lines
+67
to
+76
| if let Some(g) = row.gyro[axis] { | ||
| let p = row.p_term[axis].unwrap_or(0.0); | ||
| let i_val = row.i_term[axis].unwrap_or(0.0); | ||
| let d = row.d_term[axis].unwrap_or(0.0); | ||
| let f_val = row.f_term[axis].unwrap_or(0.0); | ||
| gyro.push(g); | ||
| pid_p.push(p); | ||
| pid_i.push(i_val); | ||
| pid_d.push(d); | ||
| pid_sum.push(p + i_val + d + f_val); |
Contributor
There was a problem hiding this comment.
Don't report missing PID fields as zero-valued samples.
unwrap_or(0.0) here turns absent P/I/D/F values into real zeroes, and the surrounding gyro gate gives them a full sample count whenever gyro exists. That makes the markdown report unable to distinguish "signal missing" from "signal logged and zero", which is misleading for tuning/debugging.
Possible fix
for row in log_data {
if let Some(g) = row.gyro[axis] {
- let p = row.p_term[axis].unwrap_or(0.0);
- let i_val = row.i_term[axis].unwrap_or(0.0);
- let d = row.d_term[axis].unwrap_or(0.0);
- let f_val = row.f_term[axis].unwrap_or(0.0);
gyro.push(g);
- pid_p.push(p);
- pid_i.push(i_val);
- pid_d.push(d);
- pid_sum.push(p + i_val + d + f_val);
+ }
+ if let Some(p) = row.p_term[axis] {
+ pid_p.push(p);
+ }
+ if let Some(i_val) = row.i_term[axis] {
+ pid_i.push(i_val);
+ }
+ if let Some(d) = row.d_term[axis] {
+ pid_d.push(d);
+ }
+ let pid_terms = [
+ row.p_term[axis],
+ row.i_term[axis],
+ row.d_term[axis],
+ row.f_term[axis],
+ ];
+ if pid_terms.iter().any(|term| term.is_some()) {
+ pid_sum.push(pid_terms.iter().map(|term| term.unwrap_or(0.0)).sum());
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/report.rs` around lines 67 - 76, The code currently uses unwrap_or(0.0)
on row.p_term/i_term/d_term/f_term inside the gyro gate so missing PID fields
are recorded as real zeros; remove the unwrap_or(0.0) usage and instead test
each Option and only push PID samples when the Option is Some (e.g. if let
Some(p) = row.p_term[axis] { pid_p.push(p) } ), and compute/push pid_sum only
when all required components are present (e.g. only when p, i, d, f are Some);
this ensures row.gyro is still collected but missing PID fields are not
misreported as zero-valued samples.
…-domain plot - Add argmin 0.11 dependency; implement CostFunction trait for ESO bandwidth search - Replace monotone 1-step MSE cost with N-step-ahead open-loop prediction cost (unimodal: low omega0 = stale f_hat; high omega0 = noise amplification) Constants: ESO_N_AHEAD_STEPS=5, ESO_WARMUP_FRACTION=0.20 - EsoResult now carries omega_meas_trace, omega_hat_trace, f_hat_trace, timestamps - Add plot_eso.rs: stacked time-domain ESO output plot per axis (omega_meas blue thin over omega_hat orange thick, f_hat green scaled) - Wire ESO output plot into main.rs after optimization; generated as *_ESO_output_stacked.png - Update report.rs: MSE label clarified to N-step-ahead prediction - Add ESO plot colors to constants.rs (COLOR_ESO_MEAS/HAT/FHAT) - ESO_GSS_MAX_ITER type corrected to u64 for argmin compatibility
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
src/main.rs (1)
1316-1331: Minor:any_validguard is effectively unreachable.Any unknown/empty axis token already triggers
print_usage_and_exitvia theotherarm, so theif any_validcheck at line 1329 never fires for thefalsecase. It's harmless, but you could drop it and just always assignplot_config.eso_axes = axes;for clarity. Not a bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1316 - 1331, The any_valid boolean guard is redundant because unknown tokens call print_usage_and_exit in the match's other arm; remove the any_valid flag and its conditional and always assign plot_config.eso_axes = axes after parsing. Specifically, delete the any_valid variable and the if any_valid { ... } check, leaving axes to be set directly into plot_config.eso_axes (referencing the axes array, the any_valid variable, and plot_config.eso_axes and the print_usage_and_exit call to locate the code).src/report.rs (2)
176-176: UseAXIS_COUNTinstead ofAXIS_NAMES.len().min(3).
axis_names.rsalready definesAXIS_COUNT = 3with a compile-time assertion that it equalsAXIS_NAMES.len(). The.min(3)embeds a magic number that duplicates that invariant.- for axis_idx in 0..AXIS_NAMES.len().min(3) { + for axis_idx in 0..crate::axis_names::AXIS_COUNT {As per coding guidelines: "Axis indices (0=Roll, 1=Pitch, 2=Yaw) must use the
src/axis_names.rsmodule withAXIS_COUNT,AXIS_NAMES, andaxis_name()function instead of creating redundant axis index constants".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` at line 176, The loop uses a magic-number expression AXIS_NAMES.len().min(3); replace that with the canonical constant AXIS_COUNT (from axis_names.rs) so axis indexing relies on the single authoritative value; update the for loop range to for axis_idx in 0..AXIS_COUNT and ensure any related uses rely on AXIS_NAMES and axis_name() rather than duplicated constants.
78-82: Dead condition and magic number.The outer loop iterates
0..AXIS_NAMES.len().min(3)(=0..3), soaxis < 4is always true and the check is dead. If the intent was to also report Throttle (setpoint index 3), the loop bound needs to change; otherwise drop the guard. The literal4is also a magic number per the repo guideline.- if axis < 4 { - if let Some(sp) = row.setpoint[axis] { - setpoint.push(sp); - } + if let Some(sp) = row.setpoint[axis] { + setpoint.push(sp); }As per coding guidelines: "All constants go in
src/constants.rs— no hardcoded magic numbers in function code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` around lines 78 - 82, The condition `if axis < 4` in the block using `axis` and `row.setpoint[axis]` is dead because the loop already iterates `0..AXIS_NAMES.len().min(3)`; either extend the loop bound to include Throttle (index 3) or remove the guard entirely. Replace the hardcoded `4` with a named constant (e.g., MAX_REPORTED_AXIS) defined in src/constants.rs and use that constant to control the loop or the check; update the loop range to `0..AXIS_NAMES.len().min(MAX_REPORTED_AXIS)` if you intend to include index 3, otherwise delete the `axis < ...` guard. Ensure references are to `AXIS_NAMES`, the loop variable `axis`, and `row.setpoint` so the change is localized to the existing logic.src/eso.rs (1)
244-253: Minor: move the/ 3.0stability divisor intoconstants.rs.The
sample_rate / 3.0stability bound is a domain-specific constant (LESO discrete-time stability limit) repeated inline. Putting it insrc/constants.rs(e.g.ESO_STABILITY_DIVISOR: f64 = 3.0) keeps the rule in one place and matches how the other ESO knobs are defined.As per coding guidelines: "All constants go in
src/constants.rs— no hardcoded magic numbers in function code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eso.rs` around lines 244 - 253, Replace the hardcoded divisor 3.0 with a named constant defined in src/constants.rs (e.g. ESO_STABILITY_DIVISOR: f64 = 3.0) and use that constant in the omega0_max_stable calculation in the function that computes omega0_max_stable (references: omega0_max_stable, sample_rate, config.omega0_max, config.omega0_min); update the error message computation that multiplies config.omega0_min * 3.0 to use the same constant (config.omega0_min * ESO_STABILITY_DIVISOR) so the stability rule is centralized in constants.rs.src/plot_functions/plot_eso.rs (1)
51-54: Nit:p95_idxname is misleading.The index is computed from
UNIFIED_Y_AXIS_PERCENTILE, not a hardcoded 95th percentile. Rename topctl_idx(or similar) so it doesn't imply a specific value if the constant is later tuned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plot_functions/plot_eso.rs` around lines 51 - 54, The local variable name p95_idx is misleading because the index is derived from UNIFIED_Y_AXIS_PERCENTILE (a tunable constant); rename p95_idx to a neutral name like pctl_idx (or percentile_idx) in the block that computes half_range and update any subsequent uses in the same scope to match (referencing all_abs, UNIFIED_Y_AXIS_PERCENTILE, UNIFIED_Y_AXIS_HEADROOM_SCALE, UNIFIED_Y_AXIS_MIN_SCALE and the half_range calculation) so the code reflects the generic percentile-based computation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/plot_functions/plot_eso.rs`:
- Around line 98-108: The f_hat scaling uses UNIFIED_Y_AXIS_MIN_SCALE instead of
the actual plot half_range, so compute fhat_scale after half_range is known
(i.e., inside draw_stacked_plot or by passing half_range into where fhat_scale
is built) and base it on data.fhat_max_abs and half_range so f_hat fills the
intended fraction of the Y range; replace the magic 0.5 with a new constant
ESO_FHAT_Y_FRACTION placed in src/constants.rs, and update usages of
fhat_scale/fhat_max_abs in plot_eso.rs (references: build_axis_data,
draw_stacked_plot, data.fhat_max_abs, fhat_scale, UNIFIED_Y_AXIS_MIN_SCALE) to
use the new calculation and constant.
---
Nitpick comments:
In `@src/eso.rs`:
- Around line 244-253: Replace the hardcoded divisor 3.0 with a named constant
defined in src/constants.rs (e.g. ESO_STABILITY_DIVISOR: f64 = 3.0) and use that
constant in the omega0_max_stable calculation in the function that computes
omega0_max_stable (references: omega0_max_stable, sample_rate,
config.omega0_max, config.omega0_min); update the error message computation that
multiplies config.omega0_min * 3.0 to use the same constant (config.omega0_min *
ESO_STABILITY_DIVISOR) so the stability rule is centralized in constants.rs.
In `@src/main.rs`:
- Around line 1316-1331: The any_valid boolean guard is redundant because
unknown tokens call print_usage_and_exit in the match's other arm; remove the
any_valid flag and its conditional and always assign plot_config.eso_axes = axes
after parsing. Specifically, delete the any_valid variable and the if any_valid
{ ... } check, leaving axes to be set directly into plot_config.eso_axes
(referencing the axes array, the any_valid variable, and plot_config.eso_axes
and the print_usage_and_exit call to locate the code).
In `@src/plot_functions/plot_eso.rs`:
- Around line 51-54: The local variable name p95_idx is misleading because the
index is derived from UNIFIED_Y_AXIS_PERCENTILE (a tunable constant); rename
p95_idx to a neutral name like pctl_idx (or percentile_idx) in the block that
computes half_range and update any subsequent uses in the same scope to match
(referencing all_abs, UNIFIED_Y_AXIS_PERCENTILE, UNIFIED_Y_AXIS_HEADROOM_SCALE,
UNIFIED_Y_AXIS_MIN_SCALE and the half_range calculation) so the code reflects
the generic percentile-based computation.
In `@src/report.rs`:
- Line 176: The loop uses a magic-number expression AXIS_NAMES.len().min(3);
replace that with the canonical constant AXIS_COUNT (from axis_names.rs) so axis
indexing relies on the single authoritative value; update the for loop range to
for axis_idx in 0..AXIS_COUNT and ensure any related uses rely on AXIS_NAMES and
axis_name() rather than duplicated constants.
- Around line 78-82: The condition `if axis < 4` in the block using `axis` and
`row.setpoint[axis]` is dead because the loop already iterates
`0..AXIS_NAMES.len().min(3)`; either extend the loop bound to include Throttle
(index 3) or remove the guard entirely. Replace the hardcoded `4` with a named
constant (e.g., MAX_REPORTED_AXIS) defined in src/constants.rs and use that
constant to control the loop or the check; update the loop range to
`0..AXIS_NAMES.len().min(MAX_REPORTED_AXIS)` if you intend to include index 3,
otherwise delete the `axis < ...` guard. Ensure references are to `AXIS_NAMES`,
the loop variable `axis`, and `row.setpoint` so the change is localized to the
existing 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 209c4fdb-6a38-4c19-8c97-10636f7e228e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
Cargo.tomlsrc/constants.rssrc/eso.rssrc/main.rssrc/plot_functions/mod.rssrc/plot_functions/plot_eso.rssrc/report.rs
✅ Files skipped from review due to trivial changes (3)
- Cargo.toml
- src/plot_functions/mod.rs
- src/constants.rs
- estimate_b0(): OLS closed-form b0 = sum(u*d_omega) / (Ts * sum(u^2)) on samples where |PID sum| >= ESO_B0_MIN_CONTROL_THRESHOLD (10.0) - b0 auto-estimated from data when user has not overridden --eso-b0; falls back to ESO_DEFAULT_B0 (1.0) when too few valid samples - EsoResult.b0_auto: true when estimated, false when user-supplied - main.rs: print shows b0=X.XXXX (estimated|user) - report.rs: b0 row labels (auto-estimated|user-supplied); updated guidance - Add ESO_B0_MIN_CONTROL_THRESHOLD = 10.0 to constants.rs - Fix pre-existing clippy collapsible_match in pid_metadata.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main.rs (2)
1279-1337: Missing duplicate-argument detection for--eso-b0and--eso-axis.Unlike
--dps(line 1221) and--output-dir(line 1245), these two new options silently let a later occurrence overwrite an earlier one. For consistency and to catch typos in scripts, mirror the existing "specified more than once" check.Proposed guard
+ } else if arg == "--eso-b0" { + // add a tracking bool near other flags, e.g. `let mut eso_b0_flag_present = false;` + if eso_b0_flag_present { + eprintln!("Error: --eso-b0 argument specified more than once."); + print_usage_and_exit(program_name); + } + eso_b0_flag_present = true;(Same pattern for
--eso-axis.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1279 - 1337, The new --eso-b0 and --eso-axis branches need the same "specified more than once" guard used by --dps/--output-dir: check whether the option was already set before accepting a new value and if so call print_usage_and_exit(program_name) with an error; for --eso-b0 test the existing value on plot_config.eso_b0 (or a companion boolean like plot_config.eso_b0_set) and for --eso-axis test plot_config.eso_axes (or plot_config.eso_axes_set) and emit a clear error message that the option was specified more than once, then exit, before parsing the new value.
1334-1337: Empty--eso-axisvalue silently keeps defaults.If
valcontains only whitespace/commas (e.g.--eso-axis ","or--eso-axis ""), everypart.trim()is""and hits theother =>arm, which already exits — good. But the explicitany_validbranch here is then unreachable for truly empty input; theif any_validwrap ends up being a no-op because an empty value would already have errored on the first emptypart. Minor: the conditional assignment can be dropped, or add an explicit "at least one axis required" error to make intent clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1334 - 1337, The parsing currently silently keeps defaults when `--eso-axis` is given an empty/whitespace/comma-only value because the loop returns early on the first empty `part` and the `if any_valid { plot_config.eso_axes = axes; }` path is effectively unreachable for truly empty input; change the logic in the `--eso-axis` parsing loop (the code that builds `axes` and tracks `any_valid`) to explicitly detect "no valid axes found" after processing `val` and return an error like "at least one axis required for --eso-axis" instead of silently keeping defaults, and then either assign `plot_config.eso_axes = axes` unconditionally or assert `any_valid` before assignment so the intent is clear (update the block that references `any_valid`, `axes`, and `plot_config.eso_axes`).src/report.rs (2)
176-176: UseAXIS_COUNTinstead of hardcoded3.
AXIS_NAMES.len()is already 3 by the project convention, so.min(3)is redundant and the3is a magic number. Prefer the canonical constant fromsrc/axis_names.rs.Proposed change
-use crate::axis_names::AXIS_NAMES; +use crate::axis_names::{AXIS_COUNT, AXIS_NAMES}; @@ - for axis_idx in 0..AXIS_NAMES.len().min(3) { + for axis_idx in 0..AXIS_COUNT {As per coding guidelines: "Axis indices (0=Roll, 1=Pitch, 2=Yaw) must use the
src/axis_names.rsmodule withAXIS_COUNT,AXIS_NAMES, andaxis_name()function instead of creating redundant axis index constants" and "no hardcoded magic numbers in function code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` at line 176, Replace the hardcoded magic number by iterating using the canonical axis count: change the loop header currently using AXIS_NAMES.len().min(3) to use AXIS_COUNT (e.g. for axis_idx in 0..AXIS_COUNT) so the code relies on the src/axis_names.rs constants (AXIS_COUNT / AXIS_NAMES) and removes the magic `3`; keep using axis_idx and axis_name() elsewhere as before.
32-37: Variance uses population divisor (N) rather than sample (N-1).For descriptive stats of a sampled time series, reporting sample std‑dev (divide by
count - 1, with a guard forcount < 2) is more conventional and matches what most tuning tools produce. Not a correctness bug, but may cause small discrepancies if users cross-check against NumPy/Pandas defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` around lines 32 - 37, The variance currently divides by count (population); change the variance calculation in src/report.rs so sample variance is used: compute var by dividing the sum of squared deviations by (count - 1) when count > 1 and set var (and consequently std_dev) to NaN (or another sentinel) when count < 2 to avoid divide-by-zero; update std_dev = var.sqrt() to use that var. Refer to the mean, var, std_dev variables in the diff and add the guard around the variance/std_dev computation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 1275-1278: The flags --eso-axis and --eso-b0 currently set their
values but are ignored unless plot_config.run_eso is true; update the argument
handling so when parsing "--eso-axis" or "--eso-b0" you also set
plot_config.run_eso = true (or alternatively emit a warning if
plot_config.run_eso is false) so the ESO pipeline controlled by
plot_config.run_eso is executed when those related flags are supplied; make this
change in the same arg-parsing logic that handles "--eso-axis" and "--eso-b0"
alongside the existing "--eso" case so behavior matches user expectations.
In `@src/report.rs`:
- Around line 78-82: The guard "axis < 4" in the block that accesses
row.setpoint[axis] is a hardcoded magic number and inconsistent with the caller
that iterates 0..AXIS_NAMES.len().min(3); change the guard to a length-based
check (e.g., axis < row.setpoint.len()) or compare against a named constant
(add/used a constant in src/constants.rs like SETPOINT_SLOTS) and use that
instead so the check is accurate and consistent with the row.gyro indexing and
avoids the magic literal.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 1279-1337: The new --eso-b0 and --eso-axis branches need the same
"specified more than once" guard used by --dps/--output-dir: check whether the
option was already set before accepting a new value and if so call
print_usage_and_exit(program_name) with an error; for --eso-b0 test the existing
value on plot_config.eso_b0 (or a companion boolean like plot_config.eso_b0_set)
and for --eso-axis test plot_config.eso_axes (or plot_config.eso_axes_set) and
emit a clear error message that the option was specified more than once, then
exit, before parsing the new value.
- Around line 1334-1337: The parsing currently silently keeps defaults when
`--eso-axis` is given an empty/whitespace/comma-only value because the loop
returns early on the first empty `part` and the `if any_valid {
plot_config.eso_axes = axes; }` path is effectively unreachable for truly empty
input; change the logic in the `--eso-axis` parsing loop (the code that builds
`axes` and tracks `any_valid`) to explicitly detect "no valid axes found" after
processing `val` and return an error like "at least one axis required for
--eso-axis" instead of silently keeping defaults, and then either assign
`plot_config.eso_axes = axes` unconditionally or assert `any_valid` before
assignment so the intent is clear (update the block that references `any_valid`,
`axes`, and `plot_config.eso_axes`).
In `@src/report.rs`:
- Line 176: Replace the hardcoded magic number by iterating using the canonical
axis count: change the loop header currently using AXIS_NAMES.len().min(3) to
use AXIS_COUNT (e.g. for axis_idx in 0..AXIS_COUNT) so the code relies on the
src/axis_names.rs constants (AXIS_COUNT / AXIS_NAMES) and removes the magic `3`;
keep using axis_idx and axis_name() elsewhere as before.
- Around line 32-37: The variance currently divides by count (population);
change the variance calculation in src/report.rs so sample variance is used:
compute var by dividing the sum of squared deviations by (count - 1) when count
> 1 and set var (and consequently std_dev) to NaN (or another sentinel) when
count < 2 to avoid divide-by-zero; update std_dev = var.sqrt() to use that var.
Refer to the mean, var, std_dev variables in the diff and add the guard around
the variance/std_dev computation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a56e0eb8-8077-4118-b86c-db70872a4c64
📒 Files selected for processing (5)
src/constants.rssrc/data_input/pid_metadata.rssrc/eso.rssrc/main.rssrc/report.rs
✅ Files skipped from review due to trivial changes (3)
- src/data_input/pid_metadata.rs
- src/constants.rs
- src/eso.rs
- OVERVIEW.md: move *_report.md bullet out of 'Generated PNG Plots'
into a new 'Generated Reports' subsection (was self-contradictory)
- src/eso.rs: add upfront validation in run_eso_optimization
* axis >= AXIS_COUNT → descriptive Err instead of panic
* non-finite / non-positive sample_rate → Err
* invalid EsoConfig fields (omega0_min/max, b0) → Err
* add control-input excitation guard: reject axes where pid_sum
is all-zero / sub-epsilon so flat-input axes are not 'optimised'
- src/report.rs: fix PID term collection in extract_axis_signals
* replace unwrap_or(0.0) with per-field Option guards so absent
P/I/D/F terms are not silently reported as real zero samples
* replace hardcoded 'axis < 4' magic literal with
row.setpoint.get(axis).copied().flatten() (bounds-safe)
- src/plot_functions/plot_eso.rs + src/constants.rs:
* add ESO_FHAT_Y_FRACTION = 0.5 constant (no more magic 0.5)
* store fhat_max_abs in AxisEsoData instead of pre-computed scale
* compute fhat_scale inside draw_stacked_plot after half_range is
known: (half_range * ESO_FHAT_Y_FRACTION) / fhat_max_abs
fixes f_hat being squashed when half_range > UNIFIED_Y_AXIS_MIN_SCALE
- src/main.rs: --eso-axis and --eso-b0 now imply --eso
(previously silently ignored when --eso was omitted)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/report.rs (1)
184-184: Redundant.min(3)onAXIS_NAMES.len().
AXIS_NAMEShas a compile-time-enforced length ofAXIS_COUNT = 3(seesrc/axis_names.rs), so.min(3)never trims anything. PreferAXIS_COUNTfor consistency withsrc/eso.rs, which already imports it.Suggested simplification
- for axis_idx in 0..AXIS_NAMES.len().min(3) { + for axis_idx in 0..crate::axis_names::AXIS_COUNT {As per coding guidelines: "Axis indices (0=Roll, 1=Pitch, 2=Yaw) must use the
src/axis_names.rsmodule withAXIS_COUNT,AXIS_NAMES, andaxis_name()function instead of creating redundant axis index constants".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` at line 184, Replace the redundant range expression "0..AXIS_NAMES.len().min(3)" with "0..AXIS_COUNT" in the loop over axis_idx; ensure AXIS_COUNT is imported alongside AXIS_NAMES (and prefer using axis_name() for human-readable labels if needed) so the loop uses the compile-time axis count rather than a hardcoded 3.src/main.rs (1)
385-385: Help text for--eso-axisomits the supportedalltoken.The parser accepts
roll | pitch | yaw | 0 | 1 | 2 | all(lines 1310–1325), but the CLI help on line 385 and the error message on line 1328 only advertiseroll,pitch,yaw. Either documentall(and numeric aliases) or drop them from the parser to keep the surface area consistent with the documentation.Suggested minimal fix
- eprintln!( - " --eso-axis <axes>: Axes to optimize (comma-separated: roll,pitch,yaw). Default: all." - ); + eprintln!( + " --eso-axis <axes>: Axes to optimize (comma-separated: roll,pitch,yaw or all). Default: all." + );Also applies to: 1322-1325
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` at line 385, The CLI help string for "--eso-axis" currently lists only "roll,pitch,yaw" but the parsing code also accepts "all" and numeric aliases (0,1,2); update the help text and the related error message to document the supported tokens (e.g., "roll,pitch,yaw,all (or 0,1,2)") so the CLI and parser match. Locate the help string literal containing "--eso-axis" and the parser branch that handles axis tokens (the parsing logic around the "all"/numeric aliases) and make their user-facing text consistent.src/eso.rs (1)
229-233: Magic numbers inestimate_b0violate the repository's constants rule.
count < 10,den.abs() < 1e-12, andb0.abs() > 1e-9are all hardcoded in function code.VALUE_EPSILON(already imported on line 21) fitsden.abs() < 1e-12directly; the minimum-sample-count threshold and the b0 significance floor should be named constants insrc/constants.rs.Suggested refactor
- if count < 10 || den.abs() < 1e-12 { + if count < ESO_B0_MIN_OLS_SAMPLES || den.abs() < VALUE_EPSILON { return None; } let b0 = num / den; - if b0.is_finite() && b0.abs() > 1e-9 { + if b0.is_finite() && b0 > ESO_B0_ESTIMATE_MIN_POSITIVE { Some(b0) } else { None }Then in
src/constants.rs:/// Minimum high-excitation samples required for OLS b0 estimation. pub const ESO_B0_MIN_OLS_SAMPLES: usize = 10; /// Minimum |b0| to accept as a meaningful estimate (rejects ~0 and ill-conditioned fits). pub const ESO_B0_ESTIMATE_MIN_POSITIVE: f64 = 1e-9;As per coding guidelines: "All constants go in
src/constants.rs— no hardcoded magic numbers in function code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eso.rs` around lines 229 - 233, Replace the hardcoded magic numbers in the estimate_b0 function: replace `count < 10` with a new constant ESO_B0_MIN_OLS_SAMPLES, replace `den.abs() < 1e-12` to use the already-imported VALUE_EPSILON, and replace `b0.abs() > 1e-9` with a new constant ESO_B0_ESTIMATE_MIN_POSITIVE; add the two new constants to src/constants.rs (names as above with the suggested values: 10 and 1e-9), update the estimate_b0 function to reference these constants, and add any necessary use/import so the constants are visible where estimate_b0 is defined.src/constants.rs (1)
262-276: LGTM — new ESO constants are well-documented and consistent with file conventions.Small consideration:
ESO_OMEGA0_MAX = 500.0rad/s (≈80 Hz) will be the binding upper bound for any loop rate ≥ ~1.5 kHz, sinceeso.rsusesmin(sample_rate/3, config.omega0_max). For 4–8 kHz logs the discrete-stability cap would otherwise allow ~1300–2660 rad/s; consider raisingESO_OMEGA0_MAX(or documenting the intentional conservative ceiling) if users are expected to get stuck at the boundary on fast loops.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/constants.rs` around lines 262 - 276, The ESO_OMEGA0_MAX constant (ESO_OMEGA0_MAX = 500.0) may unintentionally bind the observer bandwidth for high sample rates because eso.rs clamps omega0 to min(sample_rate/3, config.omega0_max); update the constant or docs: either increase ESO_OMEGA0_MAX to a higher value appropriate for target high-rate logs (e.g., to accommodate sample_rate/3 for 4–8 kHz) or add a clear comment near ESO_OMEGA0_MAX explaining the intentional conservative ceiling and linking to the min(sample_rate/3, config.omega0_max) behavior in eso.rs so users won’t be surprised when they hit the bound.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/eso.rs`:
- Around line 323-332: The code currently detects a user-supplied b0 by
comparing config.b0 to ESO_DEFAULT_B0 with a magic epsilon; instead add an
explicit boolean flag (e.g., b0_user_override) to EsoConfig (or PlotConfig) that
the CLI sets when --eso-b0 is provided, then change the branching here to check
config.b0_user_override instead of the floating-point comparison, preserving the
existing fallback to estimate_b0(&omega_meas, &pid_sum, ts) when the flag is
false and still returning (config.b0, false) when the flag is true.
- Around line 211-238: The function estimate_b0 currently accepts negative b0
values despite the docstring saying non-positive estimates should return None;
update the final guard in estimate_b0 to enforce a strictly positive estimate
(e.g., replace the b0.abs() check with b0 > 1e-9 while keeping b0.is_finite()),
so negative b0 is rejected and None returned; ensure the logic still returns
None when count < 10 or den is effectively zero (existing checks on count and
den remain), and keep or update the docstring to reflect this strict positivity
requirement; relevant symbols: function estimate_b0, variable b0, den, count,
and ESO_B0_MIN_CONTROL_THRESHOLD.
---
Nitpick comments:
In `@src/constants.rs`:
- Around line 262-276: The ESO_OMEGA0_MAX constant (ESO_OMEGA0_MAX = 500.0) may
unintentionally bind the observer bandwidth for high sample rates because eso.rs
clamps omega0 to min(sample_rate/3, config.omega0_max); update the constant or
docs: either increase ESO_OMEGA0_MAX to a higher value appropriate for target
high-rate logs (e.g., to accommodate sample_rate/3 for 4–8 kHz) or add a clear
comment near ESO_OMEGA0_MAX explaining the intentional conservative ceiling and
linking to the min(sample_rate/3, config.omega0_max) behavior in eso.rs so users
won’t be surprised when they hit the bound.
In `@src/eso.rs`:
- Around line 229-233: Replace the hardcoded magic numbers in the estimate_b0
function: replace `count < 10` with a new constant ESO_B0_MIN_OLS_SAMPLES,
replace `den.abs() < 1e-12` to use the already-imported VALUE_EPSILON, and
replace `b0.abs() > 1e-9` with a new constant ESO_B0_ESTIMATE_MIN_POSITIVE; add
the two new constants to src/constants.rs (names as above with the suggested
values: 10 and 1e-9), update the estimate_b0 function to reference these
constants, and add any necessary use/import so the constants are visible where
estimate_b0 is defined.
In `@src/main.rs`:
- Line 385: The CLI help string for "--eso-axis" currently lists only
"roll,pitch,yaw" but the parsing code also accepts "all" and numeric aliases
(0,1,2); update the help text and the related error message to document the
supported tokens (e.g., "roll,pitch,yaw,all (or 0,1,2)") so the CLI and parser
match. Locate the help string literal containing "--eso-axis" and the parser
branch that handles axis tokens (the parsing logic around the "all"/numeric
aliases) and make their user-facing text consistent.
In `@src/report.rs`:
- Line 184: Replace the redundant range expression "0..AXIS_NAMES.len().min(3)"
with "0..AXIS_COUNT" in the loop over axis_idx; ensure AXIS_COUNT is imported
alongside AXIS_NAMES (and prefer using axis_name() for human-readable labels if
needed) so the loop uses the compile-time axis count rather than a hardcoded 3.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63a172d8-e215-4544-b783-1d8959e256ca
📒 Files selected for processing (6)
OVERVIEW.mdsrc/constants.rssrc/eso.rssrc/main.rssrc/plot_functions/plot_eso.rssrc/report.rs
- src/eso.rs: fix estimate_b0 doc/behavior mismatch and magic numbers
* replace 'count < 10' with ESO_B0_MIN_OLS_SAMPLES constant
* replace 'den.abs() < 1e-12' with VALUE_EPSILON (already imported)
* replace 'b0.abs() > 1e-9' with 'b0 > ESO_B0_ESTIMATE_MIN_POSITIVE'
to enforce strictly positive estimates (negative b0 = inverted sign
convention, was accepted silently despite docstring saying otherwise)
* update docstring to reflect strict positivity requirement
- src/eso.rs + src/main.rs: replace fragile float epsilon b0 detection
* add b0_user_override: bool to EsoConfig
* add eso_b0_user_override: bool to PlotConfig
* --eso-b0 CLI flag now sets eso_b0_user_override = true
* run_eso_optimization branches on config.b0_user_override instead of
(config.b0 - ESO_DEFAULT_B0).abs() < 1e-12 so an explicit
'--eso-b0 1.0' is respected rather than silently overridden by OLS
- src/constants.rs: add two new constants for estimate_b0 thresholds
* ESO_B0_MIN_OLS_SAMPLES: usize = 10
* ESO_B0_ESTIMATE_MIN_POSITIVE: f64 = 1e-9
* add explanatory comment on ESO_OMEGA0_MAX conservative ceiling
(links to min(sample_rate/3, config.omega0_max) clamping behavior)
- src/report.rs: replace AXIS_NAMES.len().min(3) with AXIS_COUNT
* imports AXIS_COUNT from axis_names alongside AXIS_NAMES
* eliminates redundant .min(3) magic number
- src/main.rs: fix --eso-axis help text and error message
* help now lists 'roll,pitch,yaw,all (or 0,1,2)'
* error message lists 'roll, pitch, yaw, all, or 0, 1, 2'
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/main.rs (2)
55-55: UseAXIS_COUNTfor theeso_axesarray size.The
eso_axesfield is declared as[bool; 3]at Lines 55, and initialized as[true; 3]/[false; 3]at Lines 80, 107, 1311, and 1328. Per the centralized axis-naming system already used elsewhere in this file (e.g.,crate::axis_names::AXIS_NAMES), prefer[bool; AXIS_COUNT]and[_; AXIS_COUNT]so axis-count changes don't require touching every declaration.As per coding guidelines: "Axis indices (0=Roll, 1=Pitch, 2=Yaw) must use the
src/axis_names.rsmodule withAXIS_COUNT,AXIS_NAMES, andaxis_name()function instead of creating redundant axis index constants".Proposed fix
+use crate::axis_names::AXIS_COUNT; @@ - pub eso_axes: [bool; 3], + pub eso_axes: [bool; AXIS_COUNT], @@ - eso_axes: [true; 3], + eso_axes: [true; AXIS_COUNT], @@ - eso_axes: [true; 3], + eso_axes: [true; AXIS_COUNT], @@ - let mut axes = [false; 3]; + let mut axes = [false; AXIS_COUNT]; @@ - "all" => { - axes = [true; 3]; + "all" => { + axes = [true; AXIS_COUNT];Also applies to: 80-80, 107-107, 1311-1311, 1328-1328
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` at line 55, The eso_axes field and its initializations use a hard-coded length of 3; change the field declaration from "pub eso_axes: [bool; 3]" to use the centralized axis count (e.g., "pub eso_axes: [bool; AXIS_COUNT]") and replace all array initializers "[true; 3]" and "[false; 3]" with "[true; AXIS_COUNT]" and "[false; AXIS_COUNT]" respectively, importing or referencing AXIS_COUNT from crate::axis_names (or axis_names::AXIS_COUNT) so the code uses the shared axis sizing used elsewhere.
1356-1367: Fragile save/restore pattern for ESO/report settings.This manual save-and-restore of five
eso_*/run_reportfields aroundPlotConfig::none()is easy to break when neweso_*fields are added later (any forgotten field would silently regress to itsnone()default). Consider either:
- Adding an
impl PlotConfig { fn disable_plots(&mut self) { /* zeroes only plot_* flags */ } }method that leavesrun_eso,run_report,eso_b0,eso_b0_user_override, andeso_axesuntouched, or- Grouping the ESO/report fields into a sub-struct so a single field assignment preserves all of them.
Example (option 1)
- // Preserve ESO/report settings through the plot-type reset - let saved_run_eso = plot_config.run_eso; - let saved_run_report = plot_config.run_report; - let saved_eso_b0 = plot_config.eso_b0; - let saved_eso_b0_user_override = plot_config.eso_b0_user_override; - let saved_eso_axes = plot_config.eso_axes; - plot_config = PlotConfig::none(); - plot_config.run_eso = saved_run_eso; - plot_config.run_report = saved_run_report; - plot_config.eso_b0 = saved_eso_b0; - plot_config.eso_b0_user_override = saved_eso_b0_user_override; - plot_config.eso_axes = saved_eso_axes; + plot_config.disable_all_plots(); // preserves run_eso/run_report/eso_* fields🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1356 - 1367, Replace the fragile manual save/restore around PlotConfig::none() by adding a PlotConfig method that clears plot-related flags but preserves ESO/report fields: implement e.g. impl PlotConfig { fn disable_plots(&mut self) { /* reset plot_* flags and other non-ESO report state, but do NOT touch run_eso, run_report, eso_b0, eso_b0_user_override, eso_axes */ } } and then replace the save/restore block with plot_config.disable_plots(); alternatively, group the ESO/report fields into a sub-struct on PlotConfig so you can assign PlotConfig::none() while preserving that single sub-struct (e.g. let saved = plot_config.eso_report; plot_config = PlotConfig::none(); plot_config.eso_report = saved;).src/eso.rs (1)
316-325: Consider naming thesample_rate / 3.0stability factor.The divisor
3.0(used both for the upper-bound computation at Line 317 and the error threshold at Line 322) encodes the LESO discrete-time stability ratio. Extracting it to a named constant (e.g.,ESO_OMEGA0_STABILITY_RATIO = 3.0) insrc/constants.rswould eliminate the magic number and keep the two related usages in sync if the ratio ever needs to change.As per coding guidelines: "All constants go in
src/constants.rs— no hardcoded magic numbers in function code".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/eso.rs` around lines 316 - 325, Replace the hardcoded stability divisor 3.0 with a named constant: add ESO_OMEGA0_STABILITY_RATIO = 3.0 to src/constants.rs and import it into src/eso.rs, then compute omega0_max_stable as (sample_rate / ESO_OMEGA0_STABILITY_RATIO).min(config.omega0_max) and use ESO_OMEGA0_STABILITY_RATIO when computing the error threshold (config.omega0_min * ESO_OMEGA0_STABILITY_RATIO) so both uses (omega0_max_stable and the error message computation) reference the single constant instead of the magic number.src/report.rs (2)
124-124: UseAXIS_COUNTin theeso_resultsparameter type.The parameter
eso_results: &[Option<EsoResult>; 3]hardcodes the axis count as3. For consistency with the centralized axis-naming system already imported on Line 10 and used on Line 184, prefer&[Option<EsoResult>; AXIS_COUNT]. (The corresponding call site atsrc/main.rsLine 1112 would benefit from the same change.)As per coding guidelines: "Axis indices (0=Roll, 1=Pitch, 2=Yaw) must use the
src/axis_names.rsmodule withAXIS_COUNT,AXIS_NAMES, andaxis_name()function instead of creating redundant axis index constants".Proposed change
- eso_results: &[Option<EsoResult>; 3], + eso_results: &[Option<EsoResult>; AXIS_COUNT],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` at line 124, The parameter type eso_results: &[Option<EsoResult>; 3] hardcodes the axis count; change it to use the centralized constant by replacing the array size with AXIS_COUNT (i.e., &[Option<EsoResult>; AXIS_COUNT]) and update any call sites that construct or pass that fixed-size array to match the new type; ensure you import or reference AXIS_COUNT from the axis_names module consistent with other uses (e.g., where axis_name()/AXIS_NAMES are used).
33-34: Document population vs. sample std-dev choice.
vardivides bycount(population variance), notcount - 1(sample variance). For signal RMS/statistics of a full recorded time-series this is appropriate, but readers of a "statistical report" often expect the sample form. Consider adding a brief doc note onSignalStats::std_devor oncompute_signal_statsclarifying the convention so the markdown values aren't interpreted ambiguously.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.rs` around lines 33 - 34, The code currently computes variance as population variance (dividing by count) in compute_signal_stats (var = ... / count as f64) and exposes std_dev via SignalStats::std_dev; add a brief doc comment on either SignalStats::std_dev and/or compute_signal_stats stating explicitly that the implementation uses the population (N) variance convention (not the sample N-1 estimator), and update any generated markdown/report text to call out the convention so readers won't misinterpret the reported RMS/std-dev values; if desired, mention the alternative (sample variance using count - 1) and where to change the computation to switch conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/eso.rs`:
- Around line 316-325: Replace the hardcoded stability divisor 3.0 with a named
constant: add ESO_OMEGA0_STABILITY_RATIO = 3.0 to src/constants.rs and import it
into src/eso.rs, then compute omega0_max_stable as (sample_rate /
ESO_OMEGA0_STABILITY_RATIO).min(config.omega0_max) and use
ESO_OMEGA0_STABILITY_RATIO when computing the error threshold (config.omega0_min
* ESO_OMEGA0_STABILITY_RATIO) so both uses (omega0_max_stable and the error
message computation) reference the single constant instead of the magic number.
In `@src/main.rs`:
- Line 55: The eso_axes field and its initializations use a hard-coded length of
3; change the field declaration from "pub eso_axes: [bool; 3]" to use the
centralized axis count (e.g., "pub eso_axes: [bool; AXIS_COUNT]") and replace
all array initializers "[true; 3]" and "[false; 3]" with "[true; AXIS_COUNT]"
and "[false; AXIS_COUNT]" respectively, importing or referencing AXIS_COUNT from
crate::axis_names (or axis_names::AXIS_COUNT) so the code uses the shared axis
sizing used elsewhere.
- Around line 1356-1367: Replace the fragile manual save/restore around
PlotConfig::none() by adding a PlotConfig method that clears plot-related flags
but preserves ESO/report fields: implement e.g. impl PlotConfig { fn
disable_plots(&mut self) { /* reset plot_* flags and other non-ESO report state,
but do NOT touch run_eso, run_report, eso_b0, eso_b0_user_override, eso_axes */
} } and then replace the save/restore block with plot_config.disable_plots();
alternatively, group the ESO/report fields into a sub-struct on PlotConfig so
you can assign PlotConfig::none() while preserving that single sub-struct (e.g.
let saved = plot_config.eso_report; plot_config = PlotConfig::none();
plot_config.eso_report = saved;).
In `@src/report.rs`:
- Line 124: The parameter type eso_results: &[Option<EsoResult>; 3] hardcodes
the axis count; change it to use the centralized constant by replacing the array
size with AXIS_COUNT (i.e., &[Option<EsoResult>; AXIS_COUNT]) and update any
call sites that construct or pass that fixed-size array to match the new type;
ensure you import or reference AXIS_COUNT from the axis_names module consistent
with other uses (e.g., where axis_name()/AXIS_NAMES are used).
- Around line 33-34: The code currently computes variance as population variance
(dividing by count) in compute_signal_stats (var = ... / count as f64) and
exposes std_dev via SignalStats::std_dev; add a brief doc comment on either
SignalStats::std_dev and/or compute_signal_stats stating explicitly that the
implementation uses the population (N) variance convention (not the sample N-1
estimator), and update any generated markdown/report text to call out the
convention so readers won't misinterpret the reported RMS/std-dev values; if
desired, mention the alternative (sample variance using count - 1) and where to
change the computation to switch conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f2bf4684-96db-40eb-b134-a77d7f272849
📒 Files selected for processing (4)
src/constants.rssrc/eso.rssrc/main.rssrc/report.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/constants.rs
- src/constants.rs: add ESO_OMEGA0_STABILITY_RATIO = 3.0
* named constant for the LESO discrete-time stability divisor
(omega_0 < sample_rate / ESO_OMEGA0_STABILITY_RATIO)
- src/eso.rs: replace hardcoded 3.0 stability divisor with constant
* omega0_max_stable = (sample_rate / ESO_OMEGA0_STABILITY_RATIO).min(...)
* error threshold = config.omega0_min * ESO_OMEGA0_STABILITY_RATIO
* import ESO_OMEGA0_STABILITY_RATIO from constants
- src/main.rs: replace all [bool; 3] / [true; 3] / [false; 3] with AXIS_COUNT
* PlotConfig::eso_axes: [bool; AXIS_COUNT]
* Default / none() initializers: [true; AXIS_COUNT]
* --eso-axis parser: [false; AXIS_COUNT] and [true; AXIS_COUNT] for 'all'
* eso_results declaration: [Option<eso::EsoResult>; AXIS_COUNT]
* import crate::axis_names::AXIS_COUNT at top level
- src/main.rs: replace fragile save/restore with PlotConfig::disable_plots()
* add disable_plots(&mut self) method that zeroes all plot-type flags
but leaves run_eso, run_report, eso_b0, eso_b0_user_override, eso_axes
untouched — prevents future ESO field additions from silently regressing
* remove now-unused PlotConfig::none() (was only called in the
save/restore block)
* replace the 9-line save/restore block with plot_config.disable_plots()
- src/report.rs: change eso_results parameter type from [_; 3] to [_; AXIS_COUNT]
- src/report.rs: document population-variance convention on compute_signal_stats
* add doc comment stating N (not N-1) divisor is intentional for a
complete time-series and showing how to switch to sample variance
Owner
Author
|
@coderabbitai, all nitpicks resolved? full review |
Contributor
|
🧠 Learnings used✅ Actions performedFull review triggered. |
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.
How to Use the ESO (Extended State Observer) Feature
This guide explains how to run the 2nd-order Linear Extended State Observer (LESO) gain optimization and generate statistical reports using the BlackBox_CSV_Render tool.
Prerequisites
cargo build --releaseRunning ESO Optimization and Report Generation
Use the release binary from
./target/release/:--esoenables 2nd-order LESO gain optimization for all axes (Roll, Pitch, Yaw).--reportwrites a markdown statistical report (*_report.md) to the output directory.--output-dirspecifies where to save plots and reports.Optional Flags
--eso-axis roll,pitch,yaw— Restrict optimization to specific axes (comma-separated, e.g.--eso-axis roll,pitch). Implies--eso.--eso-b0 <value>— Set the control effectiveness parameter (default: 1.0). Implies--eso.Output Files
*_report.md) with:Example
Interpreting ESO Results
Troubleshooting
For more details, see the
OVERVIEW.mdand the generated report files in./output/.Summary by CodeRabbit
New Features
Documentation
CodeRabbitAI Review Fixes (commit 6e600ca)
OVERVIEW.md— Moved*_report.mdbullet out of the "Generated PNG Plots" list into a new "Generated Reports" subsection (was self-contradictory).src/eso.rs— Added upfront input validation to the publicrun_eso_optimizationAPI:axis >= AXIS_COUNTreturns a descriptive error instead of panicking.sample_ratereturns an error.EsoConfigfields (omega0_min/omega0_max/b0) return an error.pid_sumis all-zero or sub-epsilon are rejected before the optimizer runs.src/report.rs— Fixed PID term collection inextract_axis_signals:unwrap_or(0.0)with per-fieldOptionguards so absent P/I/D/F terms are not silently counted as real zero samples in statistics.axis < 4magic literal withrow.setpoint.get(axis).copied().flatten()(bounds-safe).src/plot_functions/plot_eso.rs+src/constants.rs— Fixedf_hatvisual scaling:ESO_FHAT_Y_FRACTION = 0.5constant (eliminates magic0.5literal).half_rangeis known (inside the draw closure), using(half_range * ESO_FHAT_Y_FRACTION) / fhat_max_abs. Previously usedUNIFIED_Y_AXIS_MIN_SCALEas the reference, which squashed the trace when real gyro data expandedhalf_rangebeyond the floor value.src/main.rs—--eso-axisand--eso-b0now imply--eso; previously they were silently ignored when--esowas omitted.Summary by CodeRabbit
--esoflag with per-axis control and optional effectiveness parameter (--eso-b0).--reportflag, including signal statistics and ESO results summary.