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

Reduce _lcd_move() code size #4756

Closed
wants to merge 3 commits into from
Closed

Conversation

gudnimg
Copy link
Collaborator

@gudnimg gudnimg commented Aug 17, 2024

Reuse clamp_to_software_endstops() to avoid code duplication. The code should be functionally equivalent.

Change in memory:
Flash: -204 bytes
SRAM: 0 bytes

Copy link

github-actions bot commented Aug 17, 2024

All values in bytes. Δ Delta to base

Target ΔFlash ΔSRAM Used Flash Used SRAM Free Flash Free SRAM
MK3S_MULTILANG -204 0 247826 5653 6126 2539
MK3_MULTILANG -204 0 247184 5662 6768 2530

@3d-gussner
Copy link
Collaborator

While testing this PR on MK404 I have noticed few things.
Always performed a G28W or Auto home to home all axis.

Without this changes the LCD Settings -> Move axis limits are
X: +0.5 to +255.0 but after a fresh start without homing you can go down to -0.0
Y: +0.5 to +212.0 but after a fresh start without homing you can go down to -4.0
Z: +0.0 to +210.0

With this PR
X: +0.5 to +255.5 which triggers a crash and forces a XY home (MK404 detects X length 255.03 "Belt test") and after a fresh start without homing you can go down to -0.0
Y: +0.5 to +217.0 (MK404 detects Y length 220.03 "Belt test") and after a fresh start without homing you can go down to -4.0
Z: +0.2 to +210.0

There is a difference how far you can move the x and y axis. But also Z can't be moved to 0.0 anymore
Why does the LCD shows sometimes 0.0 as possible min but as soon you move the axis it changes to 0.5?

Y_MAX_POS is 212.5 and Y_MIN_POS is -4 and the Y_MAX_LENGTH is Y_MAX_POS-Y_MIN_POS --> 212.5-4 = 216.5. On old firmware the LCD stops at 212.0 and new firmware at 217.0 both are off by 0.5

I guess some odd rounding issue.

@gudnimg
Copy link
Collaborator Author

gudnimg commented Aug 19, 2024

@3d-gussner Thanks for the detailed testing. I will take another look 💪

@gudnimg gudnimg marked this pull request as draft August 20, 2024 19:10
@gudnimg
Copy link
Collaborator Author

gudnimg commented Aug 20, 2024

Without this changes the LCD Settings -> Move axis limits are
X: +0.5 to +255.0 but after a fresh start without homing you can go down to -0.0
Y: +0.5 to +212.0 but after a fresh start without homing you can go down to -4.0
Z: +0.0 to +210.0

Something is off with the Y-axis on current MK3 branch too. It should go from -4.0 to +212.5 on MK3/S.

re-use clamp_to_software_endstops() to avoid code duplication

Change in memory:
Flash: -192 bytes
SRAM: 0 bytes
lcd_draw_update is automatically set to 1 when the knob
is rotated. See lcd_knob_update()

_lcd_move is called before lcd_draw_update is decremented, since it is called as a submenu

Change in memory:
Flash: -12 bytes
SRAM: 0 bytes
@gudnimg gudnimg force-pushed the optimise-lcd-move branch from 342dfbf to c0904e4 Compare August 24, 2024 07:11
@gudnimg
Copy link
Collaborator Author

gudnimg commented Aug 24, 2024

I've done some testing on MK3S+.

The crashes are likely happening due to these lines being removed:

if (min_software_endstops && current_position[axis] < min) current_position[axis] = min;
if (max_software_endstops && current_position[axis] > max) current_position[axis] = max;

This code is technically incorrect though as it limits the axis movement even more than *_MIN_POS and *_MAX_POS

current_position uses world coordinates which is machine coordinates with additional XYZ calibration correction (skew and offset).

On my printer the offset in XYZ calibration is:

X: -1.58mm
Y: +1.30mm

This means that current_position[X_AXIS] can go between +1.58mm and +256.58mm (Axis length is 255.0mm) and it can with this pull request. However, in the current MK3 branch this is reduced to +1.58mm and +253.42 respectively (Note world2machine_clamp correctly forbids using current_position[X_AXIS] < 1.58). Reducing the axis length from 255.0mm to 251.84mm

When I run the belt test I get the following lengths:
X axis:

Measured axis length:252.690
Measured axis length:253.010
Axis length difference:0.320

Y axis:

Measured axis length:219.570
Measured axis length:219.570
Axis length difference:0.000

I haven't checked if my XYZ Calibration is out of date... but any skew in calibration is enough to cause a TMC crash with this PR. Especially if something like a cable or cable ties are blocking the X gantry.

@gudnimg
Copy link
Collaborator Author

gudnimg commented Aug 24, 2024

Re-ran XYZ calibration, my PINDA sensor was a bit too high. Unfortunately the results are pretty similar as before.

Offset:
X: -1.50mm
Y: +1.31mm

Results from belt test (to get axis lengths):
X axis:

Measured axis length:252.690
Measured axis length:253.490
Axis length difference:0.800

Y axis:

Measured axis length:219.570
Measured axis length:219.890
Axis length difference:0.320

I suspect the cable harness is limiting my X-axis length a little bit on the far right side.

@gudnimg
Copy link
Collaborator Author

gudnimg commented Aug 26, 2024

Closing this PR for now.

@gudnimg gudnimg closed this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants