search

fluidd-core / fluidd

Fluidd, the klipper UI.
https://docs.fluidd.xyz
GNU General Public License v3.0
1.41k stars 432 forks source link

Perform sanity checks on UserInput field in Fluidd to prevent failed prints due to Klipper crashing. #908

Closed netweaver1970 closed 1 year ago

netweaver1970 commented 2 years ago

Is your feature request related to a problem? Please describe

I was live adjusting the PressureAdvance setting in FLuidd, made a mistake (entered 25 instead of 0.025, when overwriting 0.035) and the clearly impossible value was accepted and -obviously- sent to Klipper. Then Klipper tries to do something with it and crashes, as it doesn't make sense at all. Moral of the story, a 6 hr print wasted due to this: image

Describe the solution you'd like

a sensible value range check on all (at least the ones that can immediately kill a print) at input time, so the user (and printer) is protected against bad input.

Describe alternatives you've considered

don't touch anything in FLuidd after starting a print job. But that's not what's Fluidd is made for.

Additional information

No response

pedrolamas commented 2 years ago

Thanks for reporting this @netweaver1970.

I get your point and I do think there should be some sanity check on these values, but I wonder if these shouldn't actually be made centrally in Klipper instead of having all the interfaces do it...

netweaver1970 commented 2 years ago

Sure, one can do it on multiple levels. I agree that the lowest level (aka Klipper) should have its own input validation but knowing how busy Kevin is and me not being able to make a proper PR for the validation(s), with the added risk that it won't get reviewed/merged anyhow, I think the likeliest way forward is to do it one level up, at the GUI side.

Of course, it wouldn't be optimal, as the Fluidd enhancement wouldn't help Mainsail users but one could argue that's their problem then :)

slavedas commented 2 years ago

I'd like to comment that the CAUSE of this error is that when you have 0.035 and delete the 35 to make 0.0 the UI autocorrects this value to 0 Then when you type 25 it gets applied with no decimal point. Maybe don't live adjust the value in the box until the cursor leaves it or enter is pressed? It would avoid a TON of these issue? I came here to request this UI update.

netweaver1970 commented 2 years ago

Indeed, the editing is a bit awkward. But on other UserInput fields, there are also secondary/fail-safe checks on the ultimately entered values. Anything to make it safer, either by changing the UI or putting UserInput checks, or both, is a good thing to prevent wasting prints. It seems I'm not the only one being bitten by it :)

pedrolamas commented 2 years ago

@slavedas comment makes complete sense, that can cause serious issues!

Rest assured, we will try and look at what we can improve on this matter.

slavedas commented 2 years ago

@pedrolamas thank you! It is appreciated. @netweaver1970 you definitely aren't the only one and I agree that bounds checking would help as well - even if it's user defined values that we ourselves are responsible for setting. I'd love it if during a print if the temp is set to below min-temp either in the GCODE or by the user, it would pause the print and throw up a warning instead of allowing it to run to unrecoverable failure.

pedrolamas commented 1 year ago

@slavedas took a while, but we've just released 1.23.2 with an improved handling of input on those controls

I'd like to comment that the CAUSE of this error is that when you have 0.035 and delete the 35 to make 0.0 the UI autocorrects this value to 0

This scenario should not happen anymore, as no format of the input will happen until the user presses the Enter key!

pedrolamas commented 1 year ago

@netweaver1970 @slavedas I believe the changes introduced in 1.23.2 do mitigate this issue, but I would really appreciate your feedback on this matter.

netweaver1970 commented 1 year ago

Thanks. Will try when I'm back from the slopes!

slavedas commented 1 year ago

I pulled it last night but haven't had a chance to check, I'll let you know.

On Tue, Feb 21, 2023 at 5:00 AM Pedro Lamas @.***> wrote:

@netweaver1970 https://github.com/netweaver1970 @slavedas https://github.com/slavedas I believe the changes introduced in 1.23.2 do mitigate this issue, but I would really appreciate your feedback on this matter.

— Reply to this email directly, view it on GitHub https://github.com/fluidd-core/fluidd/issues/908#issuecomment-1438188390, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGEBXMGDS5K7GMZ6NM6P4FDWYSG2XANCNFSM6AAAAAARDLYWQM . You are receiving this because you were mentioned.Message ID: @.***>

slavedas commented 1 year ago

I haven't tested DURING a print, but it appears to work outside of printing so I assume it would work during a print as well.

Thank you!

On Tue, Feb 21, 2023 at 7:57 AM Stephen Lavedas @.***> wrote:

I pulled it last night but haven't had a chance to check, I'll let you know.

On Tue, Feb 21, 2023 at 5:00 AM Pedro Lamas @.***> wrote:

@netweaver1970 https://github.com/netweaver1970 @slavedas https://github.com/slavedas I believe the changes introduced in 1.23.2 do mitigate this issue, but I would really appreciate your feedback on this matter.

— Reply to this email directly, view it on GitHub https://github.com/fluidd-core/fluidd/issues/908#issuecomment-1438188390, or unsubscribe https://github.com/notifications/unsubscribe-auth/AGEBXMGDS5K7GMZ6NM6P4FDWYSG2XANCNFSM6AAAAAARDLYWQM . You are receiving this because you were mentioned.Message ID: @.***>

netweaver1970 commented 1 year ago

Here good results as well. The input handling is way more predictable. Still no sane input value checking but the underlying issue is fixed.

pedrolamas commented 1 year ago

Perfect, thank you both for testing and confirming your results!