Fix coils alignment

[GMagician]

This should fix coils alignment in all cases

Should fix #18520
@Fabi0San please validate this fix

[GMagician]

OT: Is there any possibility for people with no write access to enable labels?

[thisiskeithb]

OT: Is there any possibility for people with no write access to enable labels?

Github's permissions are not very granular, but I added some tags.

[GMagician]

Github's permissions are not very granular, but I added some tags.

The possibility to add them will help a lot, all of you, to immediately identify what is done, but it's ok

[thinkyhead]

It looks correct to me. But, since I didn't know enough to see the problem in the original homing phase code, I may not be the best judge of the new code either.

[GMagician]

@thinkyhead I really didn't had a fault to fix it. I was just having a Z issue (not repeatable position) and I saw the message (in debug mode). I checked code and I figured out it was not correct, this is why I asked @Fabi0San to take a look at it

[Fabi0San]

@GMagician you are absolutely right, with my delta head I made the assumption that Z home direction is the only one that is.

This would have been easy to spot if M122 showed the stepper phase, I have that change on a branch somewhere, I'll add it to this PR and test everything a bit later today or tomorrow.
It'll be easier if you give me push perms on your branch, Other wise I'll send in a pr to it for you to merge.

[GMagician]

@Fabi0San not sure it's possible to give write access to a branch, if it's possible, please guide me to such procedure..thanks

[thinkyhead]

not sure it's possible to give write access to a branch

A GitHub user can submit a PR to any fork + branch that they check out, modify, and push to up to their own fork (origin).

If a PR is submitted by Fabi0San to the GMagician branch "Fix-coils-alignment" then as soon as you merge that PR the new commits will be added to this PR — just as if you added the commits to your Fix-coils-alignment branch by any other method.

[Fabi0San]

That said, @GMagician's fix doesn't quite address it. It is effectively just flipping direction on everything homing to max, which includes all the deltas and may cause some endstop crashes.

I stand corrected, @GMagician's fix was mostly effective with exception of the removed int rounding, but that code is confusing as hell so I cleaned it all up, gave variables better names and also added lots of comments.

I also included my changes to M122 which exposes the phase counter and other very useful registers for tuning and debugging TMC drivers.

I created PR https://github.com/GMagician/Marlin/pull/1, please merge it.

A GitHub user can submit a PR to any fork + branch

Alternatively @GMagician can temporarily grant me contributor status to his Marlin repo.

I'll run a few tests tomorrow on my delta, will report back.

[GMagician]

@Fabi0San you have access to repository

[Fabi0San]

I'm still testing this btw so hold the merge for now.

[Fabi0San]

I've tested this and it works properly on my machine where all axes home the same direction but not all steppers are mounted the same direction.

#define INVERT_X_DIR false
#define INVERT_Y_DIR true
#define INVERT_Z_DIR true
#define X_HOME_DIR 1  // deltas always home to max
#define Y_HOME_DIR 1
#define Z_HOME_DIR 1
#define TMC_HOME_PHASE { 896, 896, 896 }

M111 S38 ; enable debug msg
M666 X0 Y0 Z0 ;disable end stop trim on deltas
G28
>>> G28
Machine Type: Delta
Endstop X hit at Phase:452 Delta:444 Distance:-0.13736
Endstop Y hit at Phase:692 Delta:820 Distance:-0.25500
Endstop Z hit at Phase:940 Delta:44 Distance:-0.01249
<<< G28
M122
uStep count     892     900     900 

896 is unreachable so steppers stay consistently close to it depending which side they come from.

@GMagician can you please test these changes on your machine?

[GMagician]

That doesn't seems correct either..let's consider X
invert = false home = +1 you stop at 452 with desired phase at 896.
You can't go to next positive phase so you have to move back to previous then 1024-896+452=580 usteps then you have to move back (-580*usteptommfactor). I'm just thinking how it should works, not read any document

[GMagician]

@GMagician can you please test these changes on your machine?

At the moment I can't I just discovered that my printer has Z not 90° (I'm building it but was all mounted) so I'm going to dismount all component to properly align (it's a bear)

[Fabi0San]

That doesn't seems correct either..let's consider X

Though it is landing on the right spot, but we surely should understand why.

invert = false home = +1 you stop at 452 with desired phase at 896.
You can't go to next positive phase so you have to move back to previous then 1024-896+452=580 usteps then you have to move back (-580*usteptommfactor). I'm just thinking how it should works, not read any document

effectorBackoutDir = -X_HOME_DIR;
stepperBackoutDir = INVERT_X_DIR ? effectorBackoutDir : -effectorBackoutDir;

effectorBackoutDir =-1 // reducing axis mm position moves away from end stop
stepperBackoutDir = 1 // increasing tmc usteps position moves away from endstop
896-452 = +444 tmc usteps
Increasing tmc usteps in the backout direction so it does not have to be inverted.
The number of tmc usteps to take is >0 so it doesn't have to skip 1024 more.

444 is the ammounto of tmc usteps we have to take, that makes 0.13736mm in the effectorBackoutDir (-1)

clear as mud 😄

[GMagician]

stepperBackoutDir = 1 // increasing tmc usteps position moves away from endstop

Yes, this is right, my assumption was wrong on this...I though decreasing ustep moved away from endstop but since dir=0 going negative direction increments them...double inversion.

[GMagician]

@Fabi0San thanks for your support. I think now this is ready to merge

[Fabi0San]

stepperBackoutDir = 1 // increasing tmc usteps position moves away from endstop

Yes, this is right, my assumption was wrong on this...I though decreasing ustep moved away from endstop but since dir=0 going negative direction increments them...double inversion.

Yeah, all this inversions make my head hurt, it was worse before, this is the least number of inversions I could find.

@Fabi0San thanks for your support. I think now this is ready to merge

Grazie Giuliano for finding the issue, creating the bug, wrapping your head around it and help coding the fix.
I really appreciate it. 🍻

@thinkyhead, we may be ready to merge now.

[GMagician]

@Fabi0San I wrote on Discord but none answered.
I'm wondering if the message "Selected home phase nnn too close to endstop trigger phase nnn. Pick a different phase for ?" shouldn't be echoed always and not only when debug is on. This may hide the cause of a variable home position and on Z may be problematic
I think that once distance from one phase to other is at least twice than "tolerance for display message" this message may be echoed to alert users

[GMagician]

How fast...but it's what if axis resolution is too small? it may be possible that from phase to phase is less or near 0.05?