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

Do we still need ENSURE_SMOOTH_MOVES? #5466

Closed
Sebastianv650 opened this issue Dec 11, 2016 · 10 comments
Closed

Do we still need ENSURE_SMOOTH_MOVES? #5466

Sebastianv650 opened this issue Dec 11, 2016 · 10 comments
Labels
T: Design Concept Technical ideas about ways and methods.

Comments

@Sebastianv650
Copy link
Contributor

Sebastianv650 commented Dec 11, 2016

I wrote this feature because of the very long LCD update. In fact, short time after this PR the slow update was fixed by updating the LCD in stripes. I did some tests today to see if it's still useful. I wasn't able to get a drained buffer at 115kbaud without it.
Aditionaly, @AnHardt pointed out that it's not easy to find good values for it.
I can't speak for other printers and I'm also not sure if I found a proper worst-case scenario for the test this time. Therefore I want to ask if someone is using this feature in RCBugFix or RC8 with a positive effect?

If it's not useful any longer, we could clean up the Configuration_adv file a little bit and avoid confusing users with another feature by removing it.

@Sebastianv650
Copy link
Contributor Author

@esenapaj you wrote about a problem with ENSURE_SMOOTH_MOVES and M600, so you are using it because it has still an advantage for you?

@thinkyhead
Copy link
Member

thinkyhead commented Dec 12, 2016

The reason for the performance boost is that we now yield back to the command parser after each screen segment, and that's a good strategy when the planner is in danger of starving. I think it still makes sense to keep the sum of all segment times and to be able to use that to inquire whether the machine is in danger of falling behind — for example, if the planner were to be blocked by a screen-segment update.

The one change I would make is to use that information to decide whether to draw the next graphical segment now, or to release control back to main loop and command parser. if the printer has the free cycles and the planner is in no danger, then it makes sense to update one more segment now.

This would make the display much more responsive, yet still retain the ability to return time to the planner when needed.

@thinkyhead thinkyhead added the T: Design Concept Technical ideas about ways and methods. label Dec 12, 2016
@Roxy-3D
Copy link
Member

Roxy-3D commented Dec 12, 2016

You know.... If Atmel wanted to do it... They could make a 64 MHz version of the AVR tomorrow. I think they are trying to encourage people to move to the 32-Bit platforms.

@Sebastianv650
Copy link
Contributor Author

Sebastianv650 commented Dec 12, 2016

@thinkyhead that makes sense, so we don't decide about updating or not, but about updating another stripe instantly or not. In this case, the value for the check shouldn't be very critical, so it should be able to find one valid for nearly every display. If it's a little bit too high, the display will be slower but not unresponsive.

Maybe this can be incorporated with #5430? I wasn't looking into this PR in detail, but maybe it's possible to replace the ENABLED(SPLIT_UP_SCREEN_DRAWING) with a if-statement checking for "long_move"?

In this case I would recommend to use a fixed value in long_move() to keep the config easy - we only have to find a valid one. I far as I have it in mind, the longest update now takes ~20ms, which would mean about 40ms (factor 2 as a worst case rule of tumb with ISRs).

@AnHardt
Copy link
Member

AnHardt commented Dec 12, 2016

I'll make some experiments with the following concept:

Keep track of the times for partial screen updates. Continuous update a max.-value. Reset it by homing/booting or what ever. (Can differ considerably - SREENROT_180)
As long as there are more partial updates to do (drawing_screen) - draw next partial update when max < block_buffer_runtime_us/2 or when block_buffer_runtime_us == 0.

That could work without configuration.

@AnHardt
Copy link
Member

AnHardt commented Dec 13, 2016

Looks and feels great!

Debug output format - printed only when display updated
';'max_update_time'*'time in buffer/2000

short moves

< 01:29:24.663 : N27401 G0 X95.388 Y99.71*17
> 01:29:24.667 : ok N27400 P0 B3
< 01:29:24.667 : N27402 G0 X95.384 Y99.807*39
> 01:29:24.671 : ok N27401 P0 B3
< 01:29:24.671 : N27403 G0 X95.383 Y99.903*36
> 01:29:24.683 : ;27*38
> 01:29:24.691 : ok N27402 P2 B2
< 01:29:24.692 : N27404 G0 X95.384 Y100*1
> 01:29:24.703 : ;27*32
> 01:29:24.711 : ok N27403 P5 B2
< 01:29:24.711 : N27405 G0 X95.386 Y100.097*18
> 01:29:24.715 : ok N27404 P4 B3
< 01:29:24.716 : N27406 G0 X95.391 Y100.193*18
> 01:29:24.719 : ok N27405 P4 B3
< 01:29:24.720 : N27407 G0 X95.398 Y100.29*42
> 01:29:24.727 : ;27*29
> 01:29:24.741 : ok N27406 P6 B2
> 01:29:24.745 : ok N27407 P6 B3
< 01:29:24.748 : N27408 G0 X95.407 Y100.386*18
< 01:29:24.748 : N27409 G0 X95.417 Y100.482*17
> 01:29:24.757 : ok N27408 P7 B3
> 01:29:24.757 : ok N27409 P6 B3
< 01:29:24.757 : N27410 G0 X95.43 Y100.577*39
< 01:29:24.758 : N27411 G0 X95.445 Y100.673*19
> 01:29:24.765 : ok N27410 P6 B3
< 01:29:24.765 : N27412 G0 X95.462 Y100.768*30
> 01:29:24.769 : ok N27411 P6 B3
< 01:29:24.770 : N27413 G0 X95.48 Y100.862*38
> 01:29:24.773 : ok N27412 P5 B3
< 01:29:24.774 : N27414 G0 X95.501 Y100.956*31
> 01:29:24.777 : ok N27413 P5 B3
< 01:29:24.778 : N27415 G0 X95.524 Y101.05*39
> 01:29:24.781 : ok N27414 P5 B3
< 01:29:24.782 : N27416 G0 X95.548 Y101.143*29
> 01:29:24.786 : ok N27415 P4 B3
< 01:29:24.786 : N27417 G0 X95.575 Y101.236*19
> 01:29:24.790 : ok N27416 P4 B3
< 01:29:24.790 : N27418 G0 X95.603 Y101.328*16
> 01:29:24.794 : ok N27417 P4 B3
< 01:29:24.794 : N27419 G0 X95.633 Y101.419*23
> 01:29:24.798 : ok N27418 P4 B3
< 01:29:24.798 : N27420 G0 X95.666 Y101.509*29
> 01:29:24.806 : ;27*29
> 01:29:24.818 : ok N27419 P6 B2
< 01:29:24.818 : N27421 G0 X95.7 Y101.599*20
> 01:29:24.822 : ok N27420 P6 B3
< 01:29:24.822 : N27422 G0 X95.736 Y101.688*17
> 01:29:24.826 : ok N27421 P5 B3
< 01:29:24.826 : N27423 G0 X95.774 Y101.777*23
> 01:29:24.830 : ok N27422 P5 B3
< 01:29:24.831 : N27424 G0 X95.813 Y101.864*19
> 01:29:24.835 : ok N27423 P5 B3
< 01:29:24.835 : N27425 G0 X95.855 Y101.951*23
> 01:29:24.838 : ok N27424 P4 B3
< 01:29:24.839 : N27426 G0 X95.898 Y102.036*30

status screen idle

01:43:21.807 : ;17*0
01:43:21.823 : ;17*0
01:43:21.840 : ;17*0
01:43:21.856 : ;17*0
01:43:22.851 : ;17*0
01:43:22.867 : ;17*0
01:43:22.884 : ;17*0
01:43:22.901 : ;17*0
01:43:23.900 : ;17*0
01:43:23.916 : ;17*0
01:43:23.932 : ;17*0
01:43:23.949 : ;17*0

Turning nob in menu

01:49:38.893 : ;21*0
01:49:38.909 : ;21*0
01:49:38.921 : ;21*0
01:49:38.938 : ;21*0
01:49:38.955 : ;21*0
01:49:38.971 : ;21*0
01:49:38.987 : ;21*0
01:49:39.003 : ;21*0
01:49:39.020 : ;21*0
01:49:39.032 : ;21*0
01:49:39.049 : ;21*0
01:49:39.064 : ;21*0
01:49:39.163 : ;21*0
01:49:39.179 : ;21*0
01:49:39.200 : ;21*0
01:49:39.216 : ;21*0
01:49:39.315 : ;21*0
01:49:39.327 : ;21*0
01:49:39.343 : ;21*0
01:49:39.355 : ;21*0
01:49:39.371 : ;21*0
01:49:39.388 : ;21*0
01:49:39.400 : ;21*0
01:49:39.417 : ;21*0
01:49:39.434 : ;21*0
01:49:39.453 : ;21*0
01:49:39.474 : ;21*0
01:49:39.572 : ;21*0
01:49:39.589 : ;21*0
01:49:39.605 : ;21*0
01:49:39.618 : ;21*0
01:49:39.634 : ;21*0
01:49:39.651 : ;21*0
01:49:39.667 : ;21*0
01:49:39.679 : ;21*0
01:49:39.786 : ;21*0
01:49:39.798 : ;21*0
01:49:39.814 : ;21*0
01:49:39.826 : ;21*0

PR tomorrow.

@AnHardt
Copy link
Member

AnHardt commented Dec 13, 2016

Preview at 'Adaptive screen updates AnHardt#71'

@AnHardt
Copy link
Member

AnHardt commented Dec 13, 2016

Adaptive screen updates for all kinds of displays #5495

@ghost
Copy link

ghost commented Dec 15, 2016

@Sebastianv650

it has still an advantage for you?

I had enabled it only for compilation error and warning detection.
I'm not sure if it has advantage.
Therefore I don't mind if it's abolished.

@github-actions
Copy link

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 Mar 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T: Design Concept Technical ideas about ways and methods.
Projects
None yet
Development

No branches or pull requests

4 participants