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

[1.1.x] Fix some planner bugs #8613

Merged
merged 3 commits into from Nov 30, 2017

Conversation

thinkyhead
Copy link
Member

@thinkyhead thinkyhead commented Nov 30, 2017

  • Fix a typo that broke ENABLE_LEVELING_FADE_HEIGHT for ABL/MBL.
  • Update Planner::apply_leveling and Planner::unapply_leveling with latest methods.
  • Drop Planner::position_float — no longer needed for LIN_ADVANCE.
  • Add Planner::set_filament_size for parity with 2.0.x

See also #8612

@thinkyhead thinkyhead merged commit ab43113 into MarlinFirmware:bugfix-1.1.x Nov 30, 2017
@thinkyhead thinkyhead deleted the bf1_planner_parity branch November 30, 2017 23:37
thinkyhead added a commit that referenced this pull request Dec 3, 2017
mtm88001 pushed a commit to mtm88001/Marlin that referenced this pull request Dec 9, 2017
@Sebastianv650
Copy link
Contributor

Sebastianv650 commented Dec 17, 2017

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
E has 400steps/mm (3mm filament)
With a line with and height of 0.5mm to 0.2mm, for a 0.05mm move we need (simplified, extrusion calculated as a rectangle):
5 steps on X axis
0.28 steps on E - oops, there are no 0.28 steps. Marlin will do 0 steps, the delta of 0.28 steps will be added to the next segment if it's then around a full step.
So our calculated ratio is (0 * 400)/(5 * 100)=0.
In reality, given the float values from gcode, it should read 0.0142.
If we have a 0.1mm move, we have:
10 steps on X,
0.57 steps on E. Now it would calculate (1 * 400)/(5 * 100)=0.8. Even worse.

This results in bad E movements and I had to grab the raw float value from gcode to calculate real ratios that will work.

@Sebastianv650
Copy link
Contributor

Sebastianv650 commented Dec 19, 2017

@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.

@thinkyhead
Copy link
Member Author

@Sebastianv650 Please test the PR linked above and see if it allows LIN_ADVANCE to function as it did previously.

@Sebastianv650
Copy link
Contributor

I will test it as soon as possible, give me 24h max.
Good point about delta and scara..
Maybe we could find a better way for the basic problem at all: All the extrusion height to with ratio calculation thing is only good for one thing - I had no clue how to get the current extruder speed inside stepper ISR. Therefore I calculate a ratio between nominal extrusion speed and nominal XYZ move speed inside planner. As the nominal speed, nominal step rate and actual step rate are known, I can recalculate extrusion speed from it inside the ISR.
If there is a clever way of grabbing the extruder speed at each ISR run, this would make things much easier. Most likely there is one and I'm just not smart enough.. Soving this should also solve SCARA / delta issue. I will think about that again after testing the PR.

@thinkyhead
Copy link
Member Author

thinkyhead commented Dec 20, 2017

I guess the current extruder "speed" within the ISR would be based on the rate of the ISR and the ratio between block->steps[E_AXIS] and block->step_event_count. For example, if the stepper ISR rate is 50kHz, steps[E_AXIS] is 512, and step_event_count is 1234, the E rate is 50000 * (512 / 1234) or 20745 steps per second (20.745kHz).

It would go up and down as the Stepper ISR accelerated and decelerated through the move.

@thinkyhead
Copy link
Member Author

thinkyhead commented Dec 20, 2017

Solving this should also solve SCARA / delta issue.

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 position_float is always set to the post-kinematic ABC tower values, and not to the XYZ positions. If they need to be set to XYZ then they must be set ahead of doing any kinematics.

@Sebastianv650
Copy link
Contributor

Sebastianv650 commented Dec 20, 2017

Hm, thanks for the idea about block->steps[E_AXIS]. Worth to do some experiments with it..

The bigger issue is that on Delta position_float is always set to the post-kinematic ABC tower values, and not to the XYZ positions. If they need to be set to XYZ then they must be set ahead of doing any kinematics.

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.

@Sebastianv650
Copy link
Contributor

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 will try with disabled advance and dry run again to see if I can isolate some moves from the gcode for this.

@Sebastianv650
Copy link
Contributor

Sebastianv650 commented Dec 20, 2017

Something seems to be broken with the planner/movement code. Maybe this should be in a new issue instead related to this Change here.

My goal was to prove or check that 1.1.6 is still OK and the LIN_ADVANCE errors started with 1.1.7 as the planner (jerk, split first move) changes are made in this version.
But as my growing army of geckos is clearly showing, something was already going on in 1.1.6. I switched back and forth between my 5.5.2017 (seems to be 1.1.0) code base and 1.1.6 many times every time printing a gecko and eleminating other possible error sources:

  • Changed from printing over USB to USB unplugged while printing from SD card (always same gcode)
  • Power cycling the printer and checking the settings twice after every new FW flash
  • disabling LIN_ADVANCE

Here is the result:
pc204483

While the 1.1.0 produces a very clean surface finish with no gaps, layer shifting, wrong extrusions etc.:
pc204485

Every 1.1.6 gecko looks exactly like this, note the visible wrong "placed" layers, like Z banding:
pc204484
It looks much worse in reality, and you can clearly feel the steps while the 1.1.0 is smooth as silk.

For me it looks like some changes might have broken the planner, maybe some numerical problem as you mentioned going from float to integer values?
If you don't have a better idea, I will continue tomorrow going back to 1.1.5, 1.1.4 and so on until the artefact disapears. As you can imagine as I'm running on 1.1.0 I wasn't following the issues very closely in last month - are there reports about shiftet layers, rough movements and so on? Feels strange that should be the only one having issues after an update.

@thinkyhead
Copy link
Member Author

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.

@thinkyhead
Copy link
Member Author

thinkyhead commented Dec 20, 2017

If you don't have a better idea, I will continue tomorrow going back to 1.1.5, 1.1.4 and so on until the artifact disappears.

That sounds like a good idea. Then we can try to narrow it down to a specific change.

I wasn't following the issues very closely in last month - are there reports about shifted layers, rough movements and so on? Feels strange that should be the only one having issues after an update.

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.

@thinkyhead
Copy link
Member Author

@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!

@Sebastianv650
Copy link
Contributor

Sebastianv650 commented Dec 21, 2017

Quite some geckos later:
I have to revert all I said yesterday about the "z-banding" like steps I got with 1.1.6 and also what I wrote about your PR. I tested 1.1.3, 5, and 1.1.6 again and all geckos printed flawlessly. The only thing I changed compared to yesterday is that I selected "Initialize EEPROM" after each flash. But I was also checking all movement values in the LCD yesterday, so there should be no change at all. So I have no clue what's going on, but the ghost has left the machine..

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.

  • 1.1.7 as released failed with LIN_ADVANCE as expected (yay, at least one result hasn't changed!)
  • 1.1.7 with commit number 1 it works, but I still have "a feeling" that at least some moves are executed different, harder, compared to <1.1.7 versions. As an engineer I don't like to base something on feelings: There are two elements that realy changed a little bit. First, the sparse rectangular infill get's stringy when it starts the infill quite often. Instead of solid lines bridging over the last layer infill, it's more like a pointy underextrusion. I don't get that on perimeters, at least not to a visible amount. Second, on the nearly horizontal areas of the gecko there are 2-3 small voids visible that don't show up in all of my other geckos and even the one printed with 1.1.7, both commits from next section.
    I would expect to see both of this if jerk and / or acceleration is increased, which fits to the "feeling". There is definitly something going on..
  • 1.1.7 with both commits of the PR, runs fine just as with pre 1.1.7 versions. "Solid" sparse infill lines, perimeters and external quality as in all other prints.

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.

@thinkyhead
Copy link
Member Author

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 LIN_ADVANCE —as it has not had a widespread impact— and the jerk code as well. We'll just recommend that CoreXY users stick with an earlier Marlin. I'll also do a 1.1.8 release tomorrow to make sure all the custom packages out there (such as those for the CR-10) get an upgrade too.

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.

damicreabox pushed a commit to damicreabox/Marlin that referenced this pull request Sep 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants