idex_modes: Bugfix for kinematic position calculation. #6486
Conversation
|
@dmbutyugin Could you take a look at this, seeing as you've been involved with this code last ? Bug that it intends to fix can be spotted by using the GET_POSITION command, checking that kinematic and toolhead position match in both carriages. |
Frans-Willem
force-pushed
the
bugfix/idex-kinematic-position-calc
branch
from
e94e4f3 to
bbca255
Compare
idex_mode would swap the X and dual-carriage rail in some cases (homing), but not in others. As such, the position calculation was correct while homing, but incorrect for the second carriage during normal moves. This commit fixes homing to work without swapped rails, removes the swapping of rails while homing, and removes the ability to swap rails (as it is now no longer used). Fix has been tested in a Hybrid_CoreXY IDEX printer (Voron Double Dragon). Hybrid_CoreXZ has identical changes and is similar enough that I am confident it will work as intended. Changes to cartesion seem simple enough, but would benefit from someone running a couple of tests. Signed-off-by: Frans-Willem Hardijzer <[email protected]>
Frans-Willem
force-pushed
the
bugfix/idex-kinematic-position-calc
branch
from
bbca255 to
8f21ae9
Compare
|
OK, thanks, I'll take a look at this soon. Then I also wanted to take a look at #6351, which also reported to fix the kinematic position calculation for the dual_carriage. So, probably one of them, or something combined, should be committed to Klipper mainline. |
|
Took a look at the #6351 PR, and to be honest I'm not entirely sure what problem it's solving, but I suspect there's a simpler (less complex) solution to it. |
|
What I meant, that PR has two things going on: first, fix the position calculation for the dual carriage, and second, support the inverted kinematics. The latter can be supported by swapping carriages, but that's cumbersome. The former is somewhat aligned with what you are trying to achieve, and you PR seems to cover more kinematics. I think it makes sense to disentangle the two, and commit the kinematics position calculation fix first. That said, I just wanted to take a look at both PRs and see which part of the two fixes makes sense to commit. |
|
There's more going on in that PR, it also appears to add support for double CoreXY (what he calls AWD), where both carriages have both A and B motors. I'm not a big fan of embedding that in the Hybrid CoreXY kinematics, I believe at that point it's not so much hybrid, but just normal corexy+idex, and as such would belong in the corexy kinematics. My Double Dragon also has carriage that I'd ideally like to swap (carriage 0 is right, carriage 1 is left currently), I'd be happy to see if I can come up with a cleaner solution for "inverting" (e.g. making sure that carriage 0 is left), what's the best way to get in touch to discuss some of this with you on a more real-time platform ? |
There was a problem hiding this comment.
Thanks! Really sorry for the delay. I finally looked at it and can confirm the problem that can be traced back to HomingMove.calc_toolhead_pos that can be called for all probing moves and if oversteps are detected for regular homing. In the latter case, the problem can be triggered as simple as
SET_DUAL_CARRIAGE CARRIAGE=1
G28 X
G28 Y
The first homing G28 X will correctly adjust the dual rail in the correct implementation, so it won't cause problems. However, the second G28 Y will keep the order of rails, including dual_carriage, the default, and as a result will compute a new toolhead position using [stepper_x] rail, that will then be applied to dual_carriage, since it's active. As a result, the toolhead will have a coordinates of carriage_0, even though carriage_1 is active.
I thought about the issue, and your fix looks like the most reasonable one. I've also tested it on cartesian IDEX and can confirm it works as expected. Given that it's also been tested on Hybrid CoreXY IDEX, I think it should be fine to merge the fix.
|
Thanks. -Kevin |
|
you killed idex homing with this update for hundrets of users. |
Instead of being shocked, it might be way more helpful to actually point out what kind of problem arose. |
like i said, it killed IDEX homing, to be precise of the DC carriage. whcih results now in out of range moves |
Well, at least two. And I'm not sure what your point regarding this is. The previous Klipper code had a real, non-hypothetical bug that could result in invalid dual_carriage moves. As I mentioned before, the GCode sequence would typically result in a move of a dual_carriage in the wrong direction and into an endstop and end of the axis. I personally ran into it myself after assembling the idex printer (a very typical scenario to run into it was to print one print with tool 1, and then start another one, also with tool 1). It had to be fixed. Then, you mention that IDEX homing is broken. Given it is
I cannot begin to guess what that wrong usage is. So, if you provide an exact sequence of commands that previously worked, and does not anymore, then we can take a look at it and at the very least provide some recommendations, e.g. add a notice to Config_Changed.md or even make some code adjustments if at all possible. But just saying "killed IDEX homing" isn't really helpful, sorry. |
|
Since HelgeKeck is not responsive, I'll just post what I gathered myself so far regarding the issue. It seems to affect RatOS and RatRig IDEX users. This is because they use custom kinematics class (https://github.com/Rat-OS/RatOS-configuration/blob/development/klippy/kinematics/ratos_hybrid_corexy.py) that mostly implements suggested changes from #6351. That kinematics class is just injected into the regular Klipper install. And since it depends on the low-level implementation of kinematics in other Klipper places, it no longer works correctly with this fix merged. The changes to that class should be similar to the ones for hybrid_corexy changes in this PR. So this situation is unfortunate, and had we known about this dependency, we could have at least given some notice to the RatOS devs. However this kind of setup is, anyways, too fragile, and it should not have been done this way - at the very least, the Klipper version should be fixed in RatOS to prevent breaking changes to be pulled in before its modules are updated. Separately, it would probably be a good idea to revive that #6351 PR, make it compatible with the current Klipper code and merge it in. The situation when Klipper doesn't natively support the kinematics of one of the prominent IDEX printers on the market is not ideal, admittedly. |
|
@dmbutyugin i agree with your conclusion, we'll need to sum up the problems and missing features with the current implementation, explain how we went about fixing it, and then come up with a plan for getting it into mainline.
It's actually explicitly mentioned in the referenced PR: #6351 (comment) Also buzz of the V-Core 4 has been all over youtube since RMRRF, but i totally understand not following the "news" that closely. I can't say this was unexpected, i knew it was bound to happen at some point, but i was kind of caught by surprise by this one. I was the one that pushed for getting the (very necessary IMO) fixes to hybrid_corexy merged into mainline, resulting in #6351, before it could become officially supported in RatOS, precisely to avoid situations like this. However after watching the PR just wither away for half a year with little feedback other than what essentially boils down to "get rid of 90% of the PR and discuss it somewhere else", blocking a major product release which was right around the corner, we had no choice but to run a separate kinematic class. The process of getting things into mainline is long, tiresome and at times frustrating. Communicating the need for changes seem to be our major weakness, i don't know exactly what to do about that given the limited time we have available. Last i proposed a change to kinematics (and it was quite limited in scope - attempting to solve PROBE_CALIBRATE requiring negative position_min - plus it was backwards compatible) it was rejected because changing kinematics classes was bad, and then we get sweeping changes like this without any real discussion? It's challenging to follow the logic in what's OK and what's not OK, and sometimes it feels like having the right logo on your printer makes things easier (of course this isn't the case, but it's hard to shake that feeling in high pressure situations, however irrational it may be). As mentioned, we're fully aware we're doing "unsanctioned" things by extending klipper and we understand the consequences, it just comes at a really bad time with no apparent warning, it would be nice if we could tag the authors who have attempted similar PR's (especially since it was already mentioned in this thread) before such merges happen to give them a heads up or have a discussion with affected parties - something... I'm always available on discord and github, but i can't react if i'm not pinged. There are thousands of RatOS installations out there, thankfully this part wasn't officially released yet, so "only" affected just under a hundred or so users, including long-term beta testers of the 3.1 IDEX and the current testers of the V-Core 4 Hybrid and IDEX models (there are youtubers building these as we speak who could be affected). The V-Core 4 Hybrid is shipping any day now (literally), this is a thoroughly tested production printer running on https://github.com/Rat-OS/RatOS-configuration/blob/development/klippy/kinematics/ratos_hybrid_corexy.py. We haven't encountered the issue you're addressing here through months of heavy testing. However, along the way i've personally identified multiple klipper internal state mismatches that we had to work around, which i believe may have been unknowingly fixed with this PR, so at least that's something - i haven't confirmed this to be the case though (i simply don't have the time right now), and there might still be problems remaining. I don't really like the way that Regardless, we've made the necessary changes to our implementation and we're moving forward. Once you're ready to discuss adding support for printers requiring X on the left and DC on the right and single toolhead hybrid corexy, please don't hesitate to reach out, i can't stress enough how badly i want to get rid of this custom kinematics class. |
Sorry, but I [mis]read that comment as, well, we implemented our own thing and do not depend on Klipper implementation anymore, which was fair from my PoV. I intended to revive the conversations around that PR after this one - at the end of the day, the fix for homing/probing in this PR was more versatile (covering all kinematics) rather than that one - which covered only hybrid_corexy, so when I got the time, I found more appropriate to start with this one.
No, of course, I knew about vcore 4 and its IDEX variant, and had this been released half a year ago - I may have went with that one, actually. But it is what it is, and I do not have any RatRig machines, and similarly I did not watch closely how the RatOS was implemented under the hood. My expectation was that you had a Klipper fork with your own changes applied, such that you'd have a direct control over the changes from the mainline, especially the ones that could break you. I personally do not mind having custom changes to Klipper, and I have one fork, or a branch of Klipper, if you will, of my own, and I manage it that way. But then it's on me to take a burden of maintaining and syncing it with the mainline. The timing of this incident is indeed unfortunate, I agree, but sadly the odds are that this would have happened sooner or later with the approach you took. Anyways, it was not my intention to cause you troubles, and I'm glad this was resolved.
Please understand that while this change was disruptive for vcore 4, in reality this is a minor change that modifies some very technical details of kinematics implementation under the hood without any user-visible impact (besides fixing a bug, naturally) for regular Klipper installations to the best of my knowledge. When I had a chance to look at the code, I found it very reasonable and if I were to implement the fix, I'd go along the lines of doing a very similar thing. Since there was no negative feedback on this PR that was also posted months ago, I went ahead and provided positive review for it.
In case of this particular PR, I directed the original author to the #6351 PR, but the conversation did not seem to conclude, more so that there was no indication that this PR would break the other existing installations. But yes, I'll make sure to ping you in case more changes to IDEX are coming that you are not aware of. Similarly, you could have ping me, either on Discord or here. It's just that as you can see, the conversation and the issue report here were not particularly constructive, so I had no information or opportunity to help you with addressing the issue (otherwise, the fix does the same changes as this PR to the regular hybrid_corexy kinematics).
FWIW, I don't really like that either, and so when I put together a better IDEX support (last year?), my intention was to add to it as little as possible (including the configuration and commands) to make it functional, but not block potential major overhauls that may happen later, since Kevin indicated that he's interested in reworking the toolhead handling for better IDEX and toolchanger support (but I'm not sure when that could happen). Anyways, once the dust settles, let's get back to inverted hybrid_corexy kinematics class. I have a bit more time now to look at various things in Klipper than I had in the past half a year. |
|
@dmbutyugin All sounds perfectly reasonable! it's not uncommon for people to think RatOS is a fork, but it's been a principle of mine from the beginning that RatOS is "just klipper", with a higher level user experience built on top of it. FWIW so far (3 years now), there hasn't been any major incidences, and this one was rather small too out of context.
Appreciate it, and will do in the future!
Yessir, onwards and upwards! Thank you for being level headed when we failed to be 🙏 |
I agree. For what it is worth, I've also written (and variously resync) dozens of experimental Klipper branches.
I agree that idex support in Klipper could be improved. If someone wants to rework it, I'm onboard. I'd suggest starting with a description of the major classes and their interfaces, and then break up the changes into a series of small commits that transition to that improved design. I understand this is a lot of work, but since these changes are likely to touch the core Klipper kinematics I think it makes sense to go slow and cautiously. -Kevin |
|
Cross-posting here that I posted in PR #6351 asking for clarification. I'm more than happy to put some more time into this, but I'm having trouble understanding the differences between "normal" hybrid_corexy, and the envisioned "inverted" hybrid_corexy. Any help on this is greatly appreciated. |
inverted hybrid corexys just have a different belt path that needs a inverted calculation. the hybrid corexy kinematic expects the 0 carriage on the left side of the printer. if oyu have a inverted belt path the calcution is wrong and y moves result in diagonal moves for the carraiges. only way to solve this is toput the DC carraige on the left side, but then the y direction is inversed. and no, you cant jsut invert the motor directions, this just doesnt work. its a known issue |
idex_mode would swap the X and dual-carriage rail in some cases (homing), but not in others. As such, the position calculation was correct while homing, but incorrect for the second carriage during normal moves. This commit fixes homing to work without swapped rails, removes the swapping of rails while homing, and removes the ability to swap rails (as it is now no longer used). Fix has been tested in a Hybrid_CoreXY IDEX printer (Voron Double Dragon). Hybrid_CoreXZ has identical changes and is similar enough that I am confident it will work as intended. Changes to cartesion seem simple enough, but would benefit from someone running a couple of tests.