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

Universal "popup freezing TFT" bug fix, loopProcessToCondition() removal #2591

Closed

Conversation

kisslorand
Copy link
Contributor

@kisslorand kisslorand commented Aug 28, 2022

Requirements

BTT or MKS TFT

Description

This PR is has more purposes linked together. PR #2525 introduced a possible bug ("LevelCorner.c" -> "while" loop at line 172), a spontaneous popup might freeze the TFT. It is an old bug which was thought to be fixed universally by PR #1959's "loopPorcessToCondition()". This "loopPorcessToCondition()" couldn't be applied in PR #2525 even by the author who created it. In fact noone ever used it than it's creator.
This PR fixes the popup bug universally.

"loopPorcessToCondition()":

  • programmers must know about it and be sure to use it to avoid the popup bug, they also have to be aware that there's this popup bug (newcomers are/will not be aware of this bug for sure)
  • boolean functions with parameter(s) cannot be used as conditional functions so it's not suitable for such
  • it's not universally applicable (see example from PR Fixed bug in L Corner menu #2525)
  • for each new condition, for which there's no available boolean function, a new such function must be created

The bugfix from this PR renders "loopPorcessToCondition()" unnecessary so it is removed. By removing it the resulting code is smaller, there's also less dynamic RAM used.

Benefits

  • "popup freezing the TFT" bug universally fixed
  • noone has to remember this bug ever
  • smaller code
  • less dynamic RAM used

Related Issues

Notes

In my printers I never ever used "loopPorcessToCondition()" but used this PR and I never ever had one single freeze (printed more than 20kg of filament).

@digant73
Copy link
Contributor

digant73 commented Sep 14, 2022

We already discussed a lot this topic also with BTT. The loopPorcessToCondition function is a "universal general" event handler that will wait for the passed event condition. Once the event is satisfied, the control is returned to the caller. The event handler handles the backend and frontend (as done by loopProcess function).
This PR will not reduce the code (you simply provided a number removing the loopProcessToCondition function) but it is simply moving the event handling on each piece of code requiring that (it means the code size will be increased as we see with all that while... you provided in this PR).
The "possible" bug in #2525 is fixed just with a specific handling in that function.

@kisslorand
Copy link
Contributor Author

kisslorand commented Sep 14, 2022

This PR will not reduce the code (you simply provided a number removing the loopProcessToCondition function) but it is simply moving the event handling on each piece of code requiring that (it means the code size will be increased as we see with all that while... you provided in this PR).

The compiled flash size is smaller. I guess it matters more than the source code size.

We already discussed a lot this topic also with BTT. The loopPorcessToCondition function is a "universal general" event handler that will wait for the passed event condition. Once the event is satisfied, the control is returned to the caller. The event handler handles the backend and frontend (as done by loopProcess function).

I know what loopProcessToCondition is, it's just simply not needed anymore

@digant73
Copy link
Contributor

you can simply remove from loopProcessToCondition the logic to handle the popup (because you fixed it in loopProcess). In that mode you will also avoid that while... provided in some files so you can reduce overall the code size. It is more elegant to use an event handler with an event condition (the even conditions are also reusable if needed by different modules)

@kisslorand
Copy link
Contributor Author

kisslorand commented Sep 14, 2022

This was my initial approach but I realized loopProcessToCondition just creates unnecessary hooks, it's not that flexible and it became redundant.
I really do not see why the avoidance of while... is desirable, especially since it is used anyways in loopProcessToCondition, more than that, using while... is easier for anyone to get the picture, no need too look up what it is for.
Using loopProcessToCondition reduces the source code, but we have practically unlimited source code space; getting rid of it saves MCU flash space which is limited.

...the even conditions are also reusable if needed by different modules

...which is also true for while....
More than that, while... can use conditions with parameters, ex:

while(isNotHappy(Digant))
{
  singSong("Don't worry, be happy!");
}

loopProcessToCondition had its glory but it was never used by anyone, it uses more RAM, uses more flash, cannot accept conditions with parameters (loopProcessToCondition(&checkSomeCondition(PARAMETER)) <- not possible), harder to comprehend and it's not needed anymore.

@kisslorand kisslorand force-pushed the Ditch-loopProcessToCondition() branch from 1cf5505 to e119f02 Compare March 15, 2023 18:19
@kisslorand kisslorand force-pushed the Ditch-loopProcessToCondition() branch from e119f02 to c72bd01 Compare March 23, 2023 04:48
@kisslorand kisslorand force-pushed the Ditch-loopProcessToCondition() branch 3 times, most recently from 080e82e to 7b39b4f Compare April 17, 2023 13:05
@kisslorand kisslorand force-pushed the Ditch-loopProcessToCondition() branch from 7b39b4f to 28a7291 Compare April 19, 2023 03:53
@kisslorand kisslorand force-pushed the Ditch-loopProcessToCondition() branch from 28a7291 to a5992d5 Compare May 2, 2023 08:55
@kisslorand kisslorand force-pushed the Ditch-loopProcessToCondition() branch 3 times, most recently from 1d23253 to 11f1c10 Compare May 30, 2023 13:36
@kisslorand kisslorand force-pushed the Ditch-loopProcessToCondition() branch from 11f1c10 to 578a0f2 Compare June 5, 2023 06:22
@stale
Copy link

stale bot commented Aug 7, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Abandoned label Aug 7, 2023
@kisslorand
Copy link
Contributor Author

Bump

@stale stale bot removed the Abandoned label Aug 7, 2023
Copy link

stale bot commented Dec 15, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Abandoned label Dec 15, 2023
@kisslorand
Copy link
Contributor Author

Closing due to being obsolete.

@kisslorand kisslorand closed this Dec 15, 2023
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.

2 participants