-
-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Conversation
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, |
Thanks Kevin,
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.
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.
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. |
Are you sure about that? I don't see why the endstop position would dictate the ordering for two reasons:
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: Cheers, |
There are multiple things to consider here:
I propose instead of trying to determine homing order by endstop position and homing direction, to just add a config_option to 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 |
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: Alternatively, adding an explicit homing order config parameter is okay too I guess. -Kevin |
@fsironman theoretically, one could remap carriage_1 to T0 and carriage_0 to T1 via corresponding macros, but that gets fiddly and confusing.
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:
@fsironman My take on this is that |
I hit this error today with the idex-home branch while homing: 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]>
Signed-off-by: Dmitry Butyugin <[email protected]>
9777a11
to
d7c6fe1
Compare
@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. |
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]>
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. |
@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. |
Thanks. In general, it seems fine to me. Let me know if you feel it is ready to commit. -Kevin |
FWIW, for the 4th patch, something like this might be a little more readable:
It's your call, but fwiw, it took me a few minutes to understand what the new code was trying to do. Cheers, |
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. |
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:
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, PS: I'm just an automated script, not a human being. |
A more readable version of ded3f90
@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. |
@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 |
Thanks. -Kevin |
…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]>
…(#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]>
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.