Skip to content
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

[BUG] SERIAL_FLOAT_PRECISION no longer works as intended #26171

Closed
ellensp opened this issue Aug 12, 2023 · 9 comments
Closed

[BUG] SERIAL_FLOAT_PRECISION no longer works as intended #26171

ellensp opened this issue Aug 12, 2023 · 9 comments

Comments

@ellensp
Copy link
Contributor

ellensp commented Aug 12, 2023

Did you test the latest bugfix-2.1.x code?

Yes, and the problem still exists.

Bug Description

From #18367

### Description
Introduces DEFAULT_PRINT_FLOAT_PRECISION

### Benefits
Allows increasing the number of fractional digits when printing floats and doubles to serial port.
Effectively increasing the reported precision of probing operations(G30, G38, M48) and configs (M503).

the define is now SERIAL_FLOAT_PRECISION
But this no longer works as intended.

Bug Timeline

Since commit 2ef71c6 on Jun 13

Expected behavior

increasing SERIAL_FLOAT_PRECISION > 2 should increase the precision of G30, G38, M48, M503 and M114

Results from 25c7c43 (commit that introduced SERIAL_FLOAT_PRECISION)
with #define SERIAL_FLOAT_PRECISION 2
M114 X:0.00 Y:0.00 Z:0.00 E:0.00 Count X:0 Y:0 Z:0
with #define SERIAL_FLOAT_PRECISION 4
M114 X:0.0000 Y:0.0000 Z:0.0000 E:0.0000 Count X:0 Y:0 Z:0

Actual behavior

It does not do this any more

Steps to Reproduce

1 #define SERIAL_FLOAT_PRECISION 4
2 build (I build for BOARD_SIMULATED)
3 Do a M114
4 note that it still has 2 decimal places.

these do not show increased precision

Version of Marlin Firmware

Bugfix-2.1.x

@GMagician
Copy link
Contributor

All these function use fixed precision and G30/M48 (I just checked them) use different precision depending on value shown.
May be a solution (extrapolated from M48) to do something like:
p_float_t(min, _MAX(3, SERIAL_FLOAT_PRECISION))?

@thephantom1492
Copy link

Using a nightly snapshot from 2023-05-14, SERIAL_FLOAT_PRECISION is working.
The precision of 2 break the corner leveling of BTT TFT screens (atleast TFT70, >1 year and yesterday firmware) as it require 4 digits.
At 2 digits the TFT70 just display 0.0000 when you try to level the corners.

#define SERIAL_FLOAT_PRECISION 4
Recv: X:43.0000 Y:43.0000 Z:5.0000 E:0.0000 Count X:13760 Y:13760 Z:32000

#define SERIAL_FLOAT_PRECISION 6
Recv: X:360.000000 Y:360.000000 Z:0.000000 E:0.000000 Count X:115200 Y:115200 Z:0

@ellensp
Copy link
Contributor Author

ellensp commented Aug 13, 2023

was broken in this commit 2ef71c6 on Jun 13

Ie #25928 "Simpler flexible SERIAL_ECHO"

@thinkyhead
Copy link
Member

SERIAL_FLOAT_PRECISION applies in some places but may not apply everywhere. Please submit PRs to patch the serial reports where it is currently supposed to apply but doesn't.

The issue with some BTT serial displays requiring exactly 4 digits is a BTT display bug. They will need to fix the firmware for these displays to handle any number of digits. Meanwhile, we can add a hack as a custom flag to ensure 4 digits is sent in all reports where this display requires 4 digits, or to assert that SERIAL_FLOAT_PRECISION must be set to 4 whenever one of these displays is enabled in the configuration.

@thephantom1492
Copy link

While I do agree that the BTT screen does have a bug, I find it more than ordinary that the SERIAL_FLOAT_PRECISION got removed and fixed to 2 positions. The returned position precision should have been configurable IMO as it was before.

@thisiskeithb
Copy link
Member

thisiskeithb commented Aug 19, 2023

The issue with some BTT serial displays requiring exactly 4 digits is a BTT display bug. They will need to fix the firmware for these displays to handle any number of digits.

There is no display option to enable when connecting these serial displays...via serial.

For "Marlin Mode" on these displays, you'd enable REPRAP_DISCOUNT_FULL_GRAPHIC_SMART_CONTROLLER, CR10_STOCKDISPLAY, or REPRAP_DISCOUNT_SMART_CONTROLLER depending on how you have EXP cables connected and which emulation mode you've enabled in the TFT firmware. SERIAL_FLOAT_PRECISION doesn't matter for that mode.

Meanwhile, we can add a hack as a custom flag to ensure 4 digits is sent in all reports where this display requires 4 digits, or to assert that SERIAL_FLOAT_PRECISION must be set to 4 whenever one of these displays is enabled in the configuration.

BTT has instructions in their TFT firmware readme to set SERIAL_FLOAT_PRECISION in Marlin to 4, which used to work fine until setting SERIAL_FLOAT_PRECISION no longer worked as intended since it now sets it to 2 or less no matter what you have set in your config thanks to:

#define SFP _MIN(SERIAL_FLOAT_PRECISION, 2)

And of course #25928 / 2ef71c6 broke things further.

The "hack" is fixing SERIAL_FLOAT_PRECISION so it worked as it did before these refactors.

@thinkyhead
Copy link
Member

There is no display option to enable when connecting these serial displays

The code in print_heater_state has been emitting no more than 2 digits for temperature precision since #20602 published in Jan 2021 (so if they were aware or the issue, BTT has had since Dec 2020 to patch their TFT firmware). In the meantime we can provide an obscure option to override the maximum use of 2 digits in print_heater_state, which will mostly only apply as a workaround for BTT TFT. #26253

@ellensp
Copy link
Contributor Author

ellensp commented Sep 10, 2023

The feature needs to come back. We should not expect everyone else to update their code because it was accidentally omitted/broken. I am presuming it was accidentally removed as the configuration option remains.

@thinkyhead thinkyhead changed the title [BUG] SERIAL_FLOAT_PRECISION no longer works as intened [BUG] SERIAL_FLOAT_PRECISION no longer works as intended Oct 8, 2023
Copy link

github-actions bot commented Dec 8, 2023

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants