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
[1.1.x] Fix some planner bugs #8613
[1.1.x] Fix some planner bugs #8613
Conversation
9dccf03
to
c51e6cd
Compare
c51e6cd
to
d1a807f
Compare
I'm a little bit afraid about dropping position_float in LIN_ADVANCE. But I'm struggling with a cold and my brain is therefore maybe only running at 50%: During the development, my first attempt was to use the step values de and so on very simmilar as you do it now. But it didn't worked. The problem was the resolution of the calculated ratio when the planner calculates very short movements, resulting in single steps only on some axis. Think about a high-resolution arc with a G1 code every 0.05mm. And that's not even very high resolution.. Let's do some math: X and Y have 100steps/mm This results in bad E movements and I had to grab the raw float value from gcode to calculate real ratios that will work. |
@thinkyhead I just updated to 1.1.7 (BugFix) to check for the LIN_ADVANCE change and it indeed breaks its functionality! I suggest to undo d1a807f. |
@Sebastianv650 Please test the PR linked above and see if it allows |
I will test it as soon as possible, give me 24h max. |
I guess the current extruder "speed" within the ISR would be based on the rate of the ISR and the ratio between It would go up and down as the Stepper ISR accelerated and decelerated through the move. |
Maybe. It's similar to the feedrate scaling problem. On Delta the feedrate applies to the tower carriages and not to the effector. So, it needs an additional scaling operation to be done, essentially ensuring that the current move will take a certain amount of time. For example, if a Delta wants to do a 1mm move in 0.01 seconds (hence a feedrate of 100mm/s) then it needs to scale that feedrate before sending it to the planner to move the towers, which will cover more or less distance than the effector. With SCARA the linear feedrate must be converted into an angular rate — degrees-per-second. Not too different from Delta, really. Although on Delta the carriages move linearly, you can think of them as being like rotary joints. The bigger issue is that on Delta |
Hm, thanks for the idea about block->steps[E_AXIS]. Worth to do some experiments with it..
If I get a working version using your idea within the ISR, we can delete the complete position capturing with position_float. Luckily the extruders work the same in all kind of printers, and the rest of the checks only contains information if an axis is moving or not. No problems with degrees here. I'm checking the PR next. |
Intermediate report for today: While the prints are better with the PR, they still show significant errors that where not present in 1.1.x RCBugfix from 5.5.2017 I was using before with the same gcode & settings. But I'm not sure if this is due to a mistake in the PR or something that has changed in the motion system since then. Some of the movements, mostly niticeable after travel moves, seems to come to a very hard stop combined with a hard noise due to the rough stop. I know about the jerk changes that where now reverted again (and are also already reverted in the PR I guess), are there other deep changes since summer? I couldn't find something at a first search through the changelog. |
I'm surprised to hear that you're seeing "rough stops" at this point. As you mention, we did revert some changes to the planner code that seemed to fix the reported issue. I'm happy to add more commits to my branch, reverting further elements of the planner, until we find something that fixes your "rough stop" issue. |
That sounds like a good idea. Then we can try to narrow it down to a specific change.
Some Planner changes were made a couple weeks before release, intended to address CoreXY issues, and probably these side-effects would have been caught by now if we had delayed the release pending more testing. |
@Sebastianv650 I've added another commit to my branch/PR that reverts the Jerk code back to its previous form. Please give it another test when you have a chance. Thanks! |
Quite some geckos later: Due to this finding I also rechecked 1.1.7 with all 3 versions: as released, with your PR commit 1 and with both commits.
As a conclusion, I would vote for merging the PR at least with commit nr. 1 to get LIN_ADVANCE working again. The jerk change might have some errors, or it changes the meaning of the jerk values in some way I would guess. Maybe do some more research here or just undo it as done in commit nr. 2? I'm away from my computer from tomorow until new year, I plan to do some testing about how to get extruder speed inside ISR and avoid the need of position_float in KW1. |
Thanks for the excellent feedback and corrections. Knowing that the jerk/acceleration feels different is important. @Bob-the-Kuhn pointed out that the feel was different, but that the results didn't seem so far out of range as to be problematic. It seems that the effects may be more pronounced than anticipated. I'll transparently patch 1.1.7 to fix For the long-term future I'll work with Bob on refining the updated jerk code and making sure we understand its behavior and potential side-effects, looking for edge cases where values can get weird. As you point out, it's certainly possible for the "jerk" config values to change meaning. For example, they had to be cut in half for the earlier change incorporating Prusa MK2's jerk code. |
ENABLE_LEVELING_FADE_HEIGHT
for ABL/MBL.Planner::apply_leveling
andPlanner::unapply_leveling
with latest methods.Planner::position_float
— no longer needed forLIN_ADVANCE
.Planner::set_filament_size
for parity with 2.0.xSee also #8612