Skip to content

Conversation

@mengchaoheng
Copy link
Contributor

@mengchaoheng mengchaoheng commented Dec 12, 2025

This PR improves the robustness of Flight Review plotting in two aspects:

  1. Parameter change visualization:

    • Filters out parameter change records where the value did not actually change
      (e.g. repeated param set with identical values).
    • Effective changes are now detected by comparing against the previous value
      (initial value or last update), avoiding misleading annotations such as
      identical CAL_* parameters being shown as changes.
  2. Actuator controls plotting:

    • Fixes incorrect handling of topic_instance for actuator controls.
    • The plot layer now consistently uses ActuatorControls.topic_instance
      instead of hard-coded instance values.
    • This makes actuator control plots work correctly for both
      dynamic control allocation (vehicle_torque/thrust_setpoint) and
      legacy actuator_controls_* topics.

These changes improve correctness and compatibility without altering
existing plot behavior for valid data.

Before/after plots are attached for comparison.

1.Parameter change visualization:

Before:
bokeh_plot (21)

After:
bokeh_plot (22)

2.Fix actuator_controls_1 torque plot not showing due to incorrect topic instance selection.

Before:

bokeh_plot (14)

After:

bokeh_plot (15)

@mengchaoheng mengchaoheng changed the title Fix missing torque plots for actuator_controls_1 Filter effective parameter changes and fix actuator controls topic instance handling Dec 14, 2025
@bkueng bkueng self-requested a review December 16, 2025 07:33
@bkueng
Copy link
Member

bkueng commented Dec 17, 2025

Nice.

Parameter change visualization:

PX4 already checks for identical parameter changes: https://github.com/PX4/PX4-Autopilot/blob/main/src/lib/parameters/parameters.cpp#L431
However, it looks like the problem is that PX4 still flags it as unsaved (https://github.com/PX4/PX4-Autopilot/blob/main/src/lib/parameters/parameters.cpp#L441), which is what the logger then uses for reading the changed parameters.
I think we should rather change that on the PX4 side.
Can you test with this change in PX4 and see if there's no identical parameters in the log anymore?

diff --git a/src/lib/parameters/parameters.cpp b/src/lib/parameters/parameters.cpp
index 560d6eec82..772a5c2a9e 100644
--- a/src/lib/parameters/parameters.cpp
+++ b/src/lib/parameters/parameters.cpp
@@ -438,7 +438,7 @@ param_set_internal(param_t param, const void *val, bool mark_saved, bool notify_
     }

     if (user_config.store(param, new_value)) {
-        params_unsaved.set(param, !mark_saved);
+        params_unsaved.set(param, !mark_saved && param_changed);
         result = PX4_OK;

     } else {

Actuator controls plotting:

That makes sense. Can you squash the first 2 commits?

@mengchaoheng
Copy link
Contributor Author

I think we should rather change that on the PX4 side.

That would be great.

Can you test with this change in PX4 and see if there's no identical parameters in the log anymore?

It looks great.

Before:
bokeh_plot (26)

After:
bokeh_plot (27)

Can you squash the first 2 commits?

I squashed the first two commits into one and dropped the last commit, since the issue was already fixed elsewhere.
Thanks for the review!

@mengchaoheng
Copy link
Contributor Author

The request has been processed, please review. @bkueng

@bkueng
Copy link
Member

bkueng commented Jan 6, 2026

Thanks for testing. Did you create a pull request to PX4 already?

There's a minor CI failure, the line is a bit too long:

plot_app/configured_plots.py:564:0: C0301: Line too long (127/100) (line-too-long)

@mengchaoheng
Copy link
Contributor Author

mengchaoheng commented Jan 6, 2026

Thanks for testing. Did you create a pull request to PX4 already?

Oh, there is a misunderstanding here, I thought px4 had been updated. I create now.

I have created the PR.

There's a minor CI failure, the line is a bit too long:

I have already used line breaks.

@bkueng

Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@bkueng bkueng merged commit 6fdf870 into PX4:main Jan 6, 2026
1 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants