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

idex_modes: Fixed the case when carriages home in the same direction #6310

Merged
merged 5 commits into from
Sep 29, 2023

Conversation

dmbutyugin
Copy link
Collaborator

@dmbutyugin dmbutyugin commented Aug 5, 2023

Previous version of the code assumed that dual carriages home away from each other. However, some users built a bit unusual setups where both carriages home in the same direction (one of them homes over the other one). This PR fixes kinematic ranges calculation for these setups.

Also, the PR fixes the homing order of the carriage on some setups. Basically, when the carriages home away from each other, the relative order of homing does not matter, so the carriage_0 is homed first, and carriage_1 afterwards. However, when the carriages home towards the same side, it is important to home the outermost carriage first. In some configurations that happens to be carriage_1, which is now correctly identified too and homed first.

The PR was tested by an owner of such hardware who confirmed that Klipper is working correctly now for their setup.

@KevinOConnor
Copy link
Collaborator

Thanks. Is this a fix for a regression in the recent idex_modes changes, or is it a feature enhancement?

FWIW, the rules for when to home look complicated and I don't know how we'd explain those rules to users. I think users would need to know the rules to ensure their printer homes correctly.

As an alternative to making code changes, could a user just declare carriage 0 to be the carriage that homes first? That seems simpler to describe and simpler for users to understand. It is also the existing behavior.

Cheers,
-Kevin

@dmbutyugin
Copy link
Collaborator Author

Thanks Kevin,

Thanks. Is this a fix for a regression in the recent idex_modes changes, or is it a feature enhancement?

There are two parts of the PR. Kinematic range fixes addresses a reported regression (previously the dual_carriage worked for the cases when carriages home in the same direction, and now the homing state is cleared when switching carriages). The fixes to the kinematic range calculation address that exact problem.

Then there is a small change to the order of the carriages homing in a small number of cases. This is an enhancement, but see my comments below.

FWIW, the rules for when to home look complicated and I don't know how we'd explain those rules to users. I think users would need to know the rules to ensure their printer homes correctly.

I do not they are that complicated and most importantly, the behavior is not surprising. Basically, if you home the carriages in the same direction, the carriage that is 'outermost' homes first, and the other carriage that is supposed to home over the other carriage homes second. This is the only valid order of homing in such cases.

As an alternative to making code changes, could a user just declare carriage 0 to be the carriage that homes first? That seems simpler to describe and simpler for users to understand. It is also the existing behavior.

Basically, I just noticed that the current (and FWIW, previous) behavior could result in a carriage crash or other strange behavior if the user configures the carriages homing in the same direction in the wrong order. You could say that they should not misconfigure that, but the documentation does not make it apparent that certain definitions of carriages can be problematic, and there are no internal validations that would report a configuration error for that in Klipper either. I realized that describing the limitations of the current homing rules is probably more bother than fixing the homing order itself, so I did that. And introducing the checks would be the same or even more work too. Although if you do insist on either of the other options, I could do that too.

@KevinOConnor
Copy link
Collaborator

KevinOConnor commented Aug 6, 2023

This is the only valid order of homing in such cases.

Are you sure about that? I don't see why the endstop position would dictate the ordering for two reasons:

  1. It's not clear to me that existing users have synchronized the coordinate system of the two carriages (position_min and position_max of carriage_0 may have a different cartesian origin than the position_min/max of carriage_1). I'm therefore not sure we can make meaningful comparisons between the limits specified for the two carriages.
  2. It also seems possible that a machine could have two toolheads on an X rail such that they had an X overlap, but still required a particular homing order. (For example, a nozzle on the front of the rail and a nozzle on the back of the rail, or perhaps a printer with two separate physical X rails.)

So, I think a change in homing order behavior would require a note in Config_Changes.md .

As far as I understand it, the overwhelming majority of idex printers home the carriages away from each other. So, FWIW, I'm not sure it's worth spending much time on "corner cases". We could instead add a statement to the documentation: The [dual_carriage] carriage is always homed after the primary carriage. If the two carriages must be homed in a particular order then the carriage that homes last must be the carriage declared in the [dual_carriage] config section.

Cheers,
-Kevin

@fsironman
Copy link

fsironman commented Aug 6, 2023

There are multiple things to consider here:

  1. hybridcorexy kinematic hybridcorexy
    For it to work it relies on A motor to be connected to the left toolhead and B motor to be connected to the right toolhead.
    Therefore it is not possible to switch around [dual_carriage] to define the homing order or the kinematic will not work anymore.

  2. Homing the toolheads to each other is more reliable for at least two reasons:
    2.1) Each microswitch or other device you home with has a certain deviation. If the toolheads home away from each other the deviation of both devices are stacked up. Whereas homing the toolhead against each other only has the deviation from one device.(In the end for an IDEX mashine you only care about the position relative to each other)
    2.2) Heat expansion: When homing the toolheads away from each other at different temperatures the microswitchtes or other device will have physically moved away from each other.

  3. options how to home two toolheads:

I propose instead of trying to determine homing order by endstop position and homing direction, to just add a config_option to [dual_carriage] called homing_first (or any other name), that defaults to false.

This would solve this issue once and for all, not break any homing behaviour, is easy to document and allows the user full control of the homing sequence on a dual carriage

@KevinOConnor
Copy link
Collaborator

Okay, thanks. I did not realize that one can not set an arbitrary primary carriage on hybrid_corexy.

If you want to go forward with automatic detection based on endstop_position, then that's fine. I do think it is subtle enough that the documentation should mention how the detection works (along with a note about the change in Config_Changes.md). Maybe something like:
During G28 homing, typically the primary carriage is homed first followed by the carriage defined in the [dual_carriage] config section. However, the [dual_carriage] carriage will be homed first if both carriages home in a positive direction and the [dual_carriage] carriage has a position_endstop greater than the primary carriage, or if both carriages home in a negative direction and the [dual_carriage] carriage has a position_endstop less than the primary carriage.

Alternatively, adding an explicit homing order config parameter is okay too I guess.

-Kevin

@dmbutyugin
Copy link
Collaborator Author

@fsironman theoretically, one could remap carriage_1 to T0 and carriage_0 to T1 via corresponding macros, but that gets fiddly and confusing.

@KevinOConnor

This is the only valid order of homing in such cases.

Are you sure about that? I don't see why the endstop position would dictate the ordering for two reasons:

1. It's not clear to me that existing users have synchronized the coordinate system of the two carriages (position_min and position_max of carriage_0 may have a different cartesian origin than the position_min/max of carriage_1).  I'm therefore not sure we can make meaningful comparisons between the limits specified for the two carriages.

2. It also seems possible that a machine could have two toolheads on an X rail such that they had an X overlap, but still required a particular homing order.  (For example, a nozzle on the front of the rail and a nozzle on the back of the rail, or perhaps a printer with two separate physical X rails.)

Well, I cannot be 100% sure about all possible configurations. But as far as I can tell that currently Klipper does not support independent X rails that are controlled by separate Y motors. There's a fork with that support implemented as a separate kinematics. I was also thinking about putting together support for dual carriage for corexy kinematics (two motors required) and for that particular kinematics with independent Y rails (but I may hold off some of these changes in light of recent changes to the PRs).

Now, if I don't receive any further feedback, here's what I'll do:

  • keep the current calculations of the carriages relative positions based on their endstops positions (so the users of the more or less regular setups can benefit from the automatic calculation of the required homing order)
  • fully disable proximity checks when safe_distance is set (or inferred) equal to 0, as promised by the documentation (so the users with some very special configuration can disable those checks altogether)
  • add the suggested text about homing for G28 command and dual_carriage and update the Config_Changes.md

Alternatively, adding an explicit homing order config parameter is okay too I guess.

@fsironman My take on this is that homing_first is not scalable enough. For example, later on we may have something like quad_carriage and I'm not sure that depending on the kinematics a simple boolean would suffice. I may be wrong here. However, it may make sense to defer some of this work and changes until after Klipper supports multiple toolheads, in which case the whole configuration of these extra carriage may have to be done differently. For now we could either a) have a fixed order of carriages homing (0, then 1, what Klipper used to do) or b) infer the homing order based on the assumed carriages positions. I take that a) is easy to keep and extend later on and b) will be reasonably easy to override in the future, if that proves to be necessary.

@joseph-greiner
Copy link

joseph-greiner commented Aug 9, 2023

I hit this error today with the idex-home branch while homing:

image

Here is the log file: klippy.log

May not be directly related, but before the error threw I had unusual communication errors.

Update: From what I have been able to test on my printers, the error is repeatable on the main branch of Klipper but only when I can get the CAN communication errors while homing. It is intermittent and very hard to reproduce. But appears to happen when there is a communication error only when homing with the probe / switch connected to a secondary / tool head MCU not connected to the stepper driver that is moving the axis.

Previous version of the code assumed that dual carriages home away
from each other, which is not true on some machines, which have the
second dual carriage homing on the first carriage. The new code
correctly identifies the relative order of the carriages now.

Signed-off-by: Dmitry Butyugin <[email protected]>
This fixes discrepancies between the documentation and the actual
implementation of the carriages kinematic ranges calculation.

Signed-off-by: Dmitry Butyugin <[email protected]>
@dmbutyugin
Copy link
Collaborator Author

@KevinOConnor I implemented the suggested changes. If there is no further feedback in the next few days, perhaps the fix could be merged into Klipper.

@joseph-greiner the error does not look like it is directly with the fix from this PR, or the idex support in general. It seems that you may have some instability in the system somewhere - CAN communication errors will naturally result in unexpected behavior during homing and afterwards. Perhaps you have issues with the cable(s), maybe terminating resistors are set incorrectly, maybe the second toolboard has hardware issues, etc.

@KevinOConnor
Copy link
Collaborator

Thanks. Looks fine to me. I'll be travelling for a couple of weeks - I'll look to commit when I return.

-Kevin

In case of multi-MCU homing it is possible that the carriage position
will end up outside of the allowed motion range due to latencies in data
transmission between MCUs. Selecting certain modes after homing could
result in home state clearing instead of blocking the motion of the
active carriage. This commit fixes this undesired behavior.

Signed-off-by: Dmitry Butyugin <[email protected]>
@dmbutyugin
Copy link
Collaborator Author

Thanks, Kevin! BTW, I added another commit which fixes resetting homing state in case of multi-MCU homing when choosing certain modes right after homing. I can move it to another PR however if you prefer that. Separately, I hope that this PR will get some testing by the community by the time you get to merge it.

@joseph-greiner
Copy link

@joseph-greiner the error does not look like it is directly with the fix from this PR, or the idex support in general. It seems that you may have some instability in the system somewhere - CAN communication errors will naturally result in unexpected behavior during homing and afterwards. Perhaps you have issues with the cable(s), maybe terminating resistors are set incorrectly, maybe the second toolboard has hardware issues, etc.

@dmbutyugin That is correct, I wanted to make sure that I updated my post with what I found and I tried to make it clear that the error was not caused by this PR or anything to do with IDEX support. It happens with my IDEX and standard printers. Most likely by an issue with the wiring. But this isn't the place for that. I have been successfully running this branch for over 100 print hours and it is awesome. Thank you for your work.

@KevinOConnor
Copy link
Collaborator

Thanks. In general, it seems fine to me. Let me know if you feel it is ready to commit.

-Kevin

@KevinOConnor
Copy link
Collaborator

FWIW, for the 4th patch, something like this might be a little more readable:

    if range_min > range_max:
        # Returning an invalid range may confuse the kinematics code.
        # So, return an empty range instead.
        return (range_min, range_min)

It's your call, but fwiw, it took me a few minutes to understand what the new code was trying to do.

Cheers,
-Kevin

@Fuergrissa1
Copy link

I tried this out to fix the issue of losing homed status when switching between T0 and T1 and it works great. The only issue I am currently having is that homing all axis causes a collision because the X carriages are not sent to their respective parking positions after being homed.

I have both homing switches on the negative X side. Carriage 1 parking position is also on this side, but carriage 2 parking position is at the positive X side. So after homing both carriages, they are both on the left side. When carriage one tries to move to the center of the the bed for Z probing it collides with carriage 2.

@github-actions
Copy link

Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html

There are some steps that you can take now:

  1. Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review
    If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
  2. Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
  3. Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available.

Best regards,
~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

@dmbutyugin
Copy link
Collaborator Author

@KevinOConnor I think the code is ready for commit, it has been tested successfully already. FWIW, I updated the code and comments to reflect your suggestion. Hopefully it is more readable now. And I think the whole thing can be squash-merged, but it is your call.

@dmbutyugin
Copy link
Collaborator Author

I tried this out to fix the issue of losing homed status when switching between T0 and T1 and it works great. The only issue I am currently having is that homing all axis causes a collision because the X carriages are not sent to their respective parking positions after being homed.

I have both homing switches on the negative X side. Carriage 1 parking position is also on this side, but carriage 2 parking position is at the positive X side. So after homing both carriages, they are both on the left side. When carriage one tries to move to the center of the the bed for Z probing it collides with carriage 2.

@Fuergrissa1, I understand the current issue you are experiencing is not a regression, right? Did the homing work in some previous version of Klipper before? Otherwise, can you configure the [homing_override] such that you first run G28 X Y, then park your toolheads on X axis appropriately, and then activate T0, move to the Z probing position, and issue G28 Z then?

@KevinOConnor KevinOConnor merged commit a4cd033 into Klipper3d:master Sep 29, 2023
1 check passed
@KevinOConnor
Copy link
Collaborator

Thanks.

-Kevin

voidtrance pushed a commit to voidtrance/klipper that referenced this pull request Nov 4, 2023
…lipper3d#6310)

Previous version of the code assumed that dual carriages home away
from each other, which is not true on some machines, which have the
second dual carriage homing on the first carriage. The new code
correctly identifies the relative order of the carriages now.

This fixes discrepancies between the documentation and the actual
implementation of the carriages kinematic ranges calculation.

Notes about dual_carriage homing and proximity checks changes

Fixed clearing of homing state after homing in certain modes

In case of multi-MCU homing it is possible that the carriage position
will end up outside of the allowed motion range due to latencies in data
transmission between MCUs. Selecting certain modes after homing could
result in home state clearing instead of blocking the motion of the
active carriage. This commit fixes this undesired behavior.

Signed-off-by: Dmitry Butyugin <[email protected]>
rogerlz referenced this pull request in DangerKlippers/danger-klipper Dec 19, 2023
…(#6310)

Previous version of the code assumed that dual carriages home away
from each other, which is not true on some machines, which have the
second dual carriage homing on the first carriage. The new code
correctly identifies the relative order of the carriages now.

This fixes discrepancies between the documentation and the actual
implementation of the carriages kinematic ranges calculation.

Notes about dual_carriage homing and proximity checks changes

Fixed clearing of homing state after homing in certain modes

In case of multi-MCU homing it is possible that the carriage position
will end up outside of the allowed motion range due to latencies in data
transmission between MCUs. Selecting certain modes after homing could
result in home state clearing instead of blocking the motion of the
active carriage. This commit fixes this undesired behavior.

Signed-off-by: Dmitry Butyugin <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Sep 30, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants