Skip to content

Report M48 max delta #26286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Apr 6, 2025

Conversation

vovodroid
Copy link
Contributor

Description

Add maximum delta from mean value in M48 report - i.e. std::max(mean - min, max - mean).

image

Precision is also reduced from six to three to save screen space, I guess 0.001 is good enough.
I didn't change string buffer size, as I per my understanding original 8 bytes buffer is too small for precision six (plus leading zero, dot, and trailing null). So for precision 3 six byte buffer needed. Do I miss something?

Benefits

It could help to discover case when one probe deviate too much.

@thisiskeithb
Copy link
Member

Precision is also reduced from six to three to save screen space, I guess 0.001 is good enough.

This will still overflow standard screens, so it should be <20 characters.

@vovodroid
Copy link
Contributor Author

it should be <20 characters.

Dev/delta .001/.001 is 19. Is it suitable? I don't think it will be more than 1 mm.))

@vlsi
Copy link
Contributor

vlsi commented Sep 17, 2023

What do you think of showing range (max-min) rather than 'max delta'?
I guess range would be easier to understand without reading the documentation

@vovodroid
Copy link
Contributor Author

What do you think of showing range (max-min) rather than 'max delta'?

Well, it was my first approach. But next I thought that what we need to know is how single probe could divert from real value.

Assuming we have following result sets:

0.1 0.2 0.3 0.4 0.5 - range is 0.4, max delta is 0.2
0.1 0.1 0.1 0.1 0.5 range is 0.4, but max delta is 0.32

So first set presents case with even measurements distribution, and second set is case when one probe gives bad result.
From y point of view we would like to recognize send case, indicating some problem with probe. And actually deviation report show range.

@ellensp
Copy link
Contributor

ellensp commented Sep 17, 2023

You might want to look at why CI keeps failing,,, Some platforms are older and the code needs to run on them all.

remove the std::max use max

@vlsi
Copy link
Contributor

vlsi commented Sep 17, 2023

Well, the deviation would be different for both cases.

The standard deviation itself does not report the range, but it shows "deviation from the mean"

0.1 0.1 0.1 0.1 0.5 yields sigma of 0.16, 95% coinf. interval of 0.14
0.1 0.2 0.3 0.4 0.5 yields sigma of 0.14, 95% coinf. interval of 0.124, 99% coinf. interval of 0.163, 99% coinf. interval of 0.184

So it reporting range=0.4, 99% c.i. = 0.184 would more-or-less clarify the summary.

Then, it might make sense to show the values (e.g. diff from a mean)

@vovodroid
Copy link
Contributor Author

0.1 0.1 0.1 0.1 0.5 yields sigma of 0.16
0.1 0.2 0.3 0.4 0.5 yields sigma of 0.14

That's what I mean! In first case sigma is only slightly worse than in second, but expected error is 0.4 in first case vs 0.2 in second. So max delta is more informative to detect probing problems.

remove the std::max use max

It fails, that's why I used std::

Marlin\src\gcode\calibrate\M48.cpp:266:183: error: 'max' cannot be used as a function
  266 |       ui.status_printf(0, F(S_FMT ": %s, " S_FMT ":%s"), GET_TEXT(MSG_M48_DEVIATION), dtostrf(sigma, 2, 3, sigma_str), GET_TEXT(MSG_M48_MAX_DELTA), dtostrf(max(mean - min, max - mean) , 2, 3, delta_str));

@vlsi
Copy link
Contributor

vlsi commented Sep 17, 2023

So max delta is more informative to detect probing problems

If you show both "range", and "max delta", then it is more-or-less fine.
However, If you show only "max delta", then it is worse than showing "range" alone.

I might be missing something. Could you please refer to an article suggesting to use max delta descriptive statistic?

@vovodroid
Copy link
Contributor Author

If you show both "range", and "max delta", then it is more-or-less fine.

Unfortunately there is no enough room on screen, especially on standard screens.

Could you please refer to an article suggesting to use max delta descriptive statistic?

It's called "maximum absolute deviation from the mean", I guess.

@vlsi
Copy link
Contributor

vlsi commented Sep 17, 2023

0.1 0.1 0.1 0.1 0.5 has max abs deviation of 0.32
0.1 0.5 0.1 0.5 0.1 has max abs deviation of 0.24

Does it really mean the second probe is better? I do not think so.
What is the use of "max abs deviation" in the M48 results?

@vovodroid
Copy link
Contributor Author

vovodroid commented Sep 18, 2023

Does it really mean the second probe is better?

Yes. Assuming in the first case real value is 0.1, we see that worst probe error is 0.4. And in second case real value seems to be 0.3, and worst error is 0.2. Default probing number is 10, so worst probe will less affect mean (real) value.

What is the use of "max abs deviation" in the M48 results?

It can indicate probe problem, when one of several probes has large errors. Obviously it could be better to show all available information, but we are limited by screen size.

@thinkyhead
Copy link
Member

On narrow screens without staus message scrolling enabled the status message will be too long to read, so I'd recommend checking for that before attempting to display an over-long message.

@vovodroid
Copy link
Contributor Author

Do we really need 6 digits for deviation? Any meaning for < 0.001?

@thinkyhead
Copy link
Member

Do we really need 6 digits for deviation? Any meaning for < 0.001?

No harm if it fits on the screen. People who deal with deviations tend to like precision.

thinkyhead and others added 5 commits September 23, 2024 23:56
* 🔨 Update ESP32 env for MKS Tinybee
* 🔨 Updated LPC common env
* 🔨 Other env improvements

Co-Authored-By: Michael <[email protected]>
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 3 times, most recently from 37d77d6 to aa44542 Compare September 28, 2024 01:10
@thinkyhead thinkyhead force-pushed the m48-report-delta-pr branch from 992e7eb to fc8c4ff Compare March 20, 2025 22:18
@thinkyhead thinkyhead force-pushed the m48-report-delta-pr branch from f606de6 to aec0e38 Compare March 21, 2025 02:35
@thinkyhead thinkyhead force-pushed the bugfix-2.1.x branch 3 times, most recently from 4354891 to efa1758 Compare March 28, 2025 01:57
@thinkyhead thinkyhead merged commit 493c5ee into MarlinFirmware:bugfix-2.1.x Apr 6, 2025
64 checks passed
@vovodroid vovodroid deleted the m48-report-delta-pr branch April 6, 2025 19:08
EvilGremlin pushed a commit to EvilGremlin/Marlin that referenced this pull request May 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants