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

Inconsistent Extrusion - ARM branch #699

Open
stecor7001 opened this issue Aug 10, 2017 · 24 comments
Open

Inconsistent Extrusion - ARM branch #699

stecor7001 opened this issue Aug 10, 2017 · 24 comments

Comments

@stecor7001
Copy link

Hello All,

Thank you and John Silvia for the great piece of software. I am finally printing with a 32-bit CPU. While setting up, I noticed I was not able to calibrate extrusion - it was inconsistent. Here is what I found:

Issue: Extruded length calibration inconsistent (e.g. extruding 1x50mm is calibrated at 50mm, however 5x10mm do not yield 50mm but rather 38 or so).

Cause: In Extruder.cpp, line 748 and further, we have:
'#if USE_ADVANCE
Printer::maxExtruderSpeed = (ufast8_t)floor(HAL::maxExtruderTimerFrequency() / (Extruder::current->maxFeedrate * next->stepsPerMM));
#if CPU_ARCH == ARCH_ARM
if(Printer::maxExtruderSpeed > 40) Printer::maxExtruderSpeed = 40;
#else
if(Printer::maxExtruderSpeed > 15) Printer::maxExtruderSpeed = 15;
#endif'

Where:
'HAL::maxExtruderTimerFrequency()' is F_CPU/32 or 2.25MHz.

It is used in HAL.cpp line 1268 and onwards to set the next extruder interrupt as:
'extruderChannel->TC_RC = Printer::maxExtruderSpeed;'

maxExtruderSpeed is the ExtruderCounter count when the next Extruder interrupt occurs. This count should not fall below a min value (causing too frequent extruder motor steps) rather than not exceed a max limit (as currently coded).
In my case, maxExtruderTimerSpeed calculated at 214, which was then brought down to 40. This caused the extruder to be driven too fast, missing steps.

Thus, I propose considering the following change be made:
Option A:
'if(Printer::maxExtruderSpeed < 40) Printer::maxExtruderSpeed = 40;'

This yields a min safe value of ~18us, which corresponds to 56250 steps/second. It is questionable whether this is actually a safe value or not, depending of the microstepping mode for the driver. 1/64 or higher modes yield acceptable results, I doubt this frequency is acceptable for motors driven 1/8 or lower.

Option B: remove the safe value check altogether since its purpose is to mask configuration issues. Since maxExtruderSpeed is calculated it should always be a safe value.


Also, the following observations apply:

  1. Please consider whether maxExtruderSpeed is to be retained or you would want to change the variable name e.g. ExtruderStepPeriod
  2. You may wish to review the typedef for ufast8_t, which is defined as 'unsigned int'. While the name is appropriate for AVR, it is certainly not an 8-bit value for ARM... it is a 32bit value here, which is good as we don't have overflow, however the '8' particle is misleading. Maybe make it neutral in terms of length?

Thank you!

@stecor7001
Copy link
Author

Please let me know if you require additional information. Sorry for the long post.

@Nibbels
Copy link

Nibbels commented Aug 23, 2017

I just read the code for AVR.
Just one shot into the blue to understand my Arduino Mega2560s (AVR) situation here:

HAL::maxExtruderTimerFrequency() --> 16.000.000UL / prescaler 64 = 250.000

Printer::maxExtruderSpeed = (uint8_t)floor(HAL::maxExtruderTimerFrequency() / (Extruder::current->maxFeedrate * Extruder::current->stepsPerMM));

250.000 / ([280steps/mm .. 560steps/mm]*25mm/s) = [35 .. 17]

If i really got this right: It seems that the minimum distance of time between interrupts for extrusion is always constrained to 15 on my Printer.
That means:

EXTRUDER_OCR = timer + Printer::maxExtruderSpeed;
EXTRUDER_OCR is OCR0A ->
OCR0A = timer + 15;

Some weeks earlier I read about the topic and I think that the extruder should drive Advance Steps at maximum speed. For me it is over maximum as it seems according to your thoughts. But why. Your post is quite interesting!

@repetier
Copy link
Owner

Always remember you are usinga AVR 8 bit processor. 15 means max. frequency of 16.6KHz which get called always adding extra load to the cpu. We can already easily use 100% cpu load with the normal stepper interrupt, so it should be clear that we have a limit what is useful and that is what I have choosen as limit. For normal extruders with 100steps per mm that is quite fast, but of course nowadays with 1/128 microstepping you can bring the processor to it's limit and the question is if that is a goo dthing to have such a resolution or if it is more cosmetric to have that many steps per mm. At some point you will not see any difference. And even with your high precision it becomes only a problem during retraction where you want high speeds for advance.

@Nibbels
Copy link

Nibbels commented Aug 23, 2017

thinking... As I understand this,

OCR0A = timer + 15 is a faster filament push cycle than OCR0A = timer + 35

So the point is that the function does not set this advance steps speed to "normal fastest steps". Its meaning is that it is set to something like at least 16.6khz . So that we dont constrain the advance algorithms steps to possible slower usermade rectraction-speeds?

@repetier
Copy link
Owner

Yes I think you got it. Not knowing when the timer interrupt will increase the steps to do we check more frequently then normally needed and work the counter down and then wait for new steps to execute. That we we have only a small delay for normal extrusion and depending on advance value may start lagging if it goes faster but we try to catch up as fast as interrupt allows.

@Nibbels
Copy link

Nibbels commented Aug 23, 2017

Ok, because the topic is still suitable I post this here in hope to not annoy anyone with my little problem :)

Again I am not working with latest repetier, as I am bound to the old version which was originally adopted by Conrad Electronic to drive their RF1000 and RF2000 printer.
On my long trip through the firmware I stumbled upon USE_ADVANCE which I am debugging rightnow.
First I updated the specific code to the repetier development branch by going through line by line. Added menu support and so on...

Now I am printing with this feature.

Originally I read this Issue to find some solution for my problem with blobs. Reason is that found out that those specific blobs I want to get rid of appear with retracts and not when hitting edges.

screenshot_3

When I use S3D as Slicer there are several options.

i1234 cimgpsh_orig
i1235 cimgpsh_orig
As stated within those pictures:

  • 1 Retract
  • 2 Wipe Nozzle
  • 3 Advanced -> Perform retraction during wipe movement.

Those blobs are there when 3 is activated with 1 and 2. I cannot tell if this is the whole pool of blobmaking settings, but if I remove the setting 3 the print goes well.
GCode Examples:

1+2+3 produces:

G92 E0
G1 E-0.9000 F1800
G1 X49.440 Y95.740 F2400
G1 Z1.700 F1002
G1 X45.181 Y96.260 F4800
G1 Z1.600 F1002
G1 E0.0000 F1800
G92 E0

1+2 produces

G92 E0
G1 X45.640 Y95.740 E-0.9000 F2400
G1 X49.440 Y95.740 F2400
G1 Z1.700 F1002
G1 X45.181 Y96.260 F4800
G1 Z1.600 F1002
G1 E0.0000 F1800
G92 E0

Which falls down to:

G1 E-0.9000 F1800
G1 X49.440 Y95.740 F2400
vs.
G1 X45.640 Y95.740 E-0.9000 F2400
G1 X49.440 Y95.740 F2400

I see a Move with X,Y and -E
In motion.cpp I collapsed all I can ignore:
screenshot_2
X,Y,-E would mean active advance while Retract. In this special case it might be bad?

That Is kind of how far I debugged this problem.
Of course I saw some spots in code where:
if (move->moveDecelerating())
else if (move->moveAccelerating())
else // full speed reached
played some role but I am stuck a bit, because I cannot fully trust my port to the newest repetier development 100%.

But .. cant it somehow be the case that this very uncommon kind of x-y-retract is bad because its huge amount of negative extrusion?
Is it possible that advance always suggests that extrusion is positive at the beginning?
For me it seemed that way too much filament got extruded at points where the nozzle is set to jump.
Here is some video with retract and wipe combined:
https://youtu.be/0WGzeGXWqs8
And this is with retract and wipe seperated - working good:
https://youtu.be/FzEtTbokslU
Sorry, If that all is my fault, but I guessed that this info could help some day.

Greetings

@repetier
Copy link
Owner

I think you have found a bug here. Advance was not for retraction but for extrusion while accelerating/decelerating. Now you want the opposite direction in your wipe using the same code and it even contains fabs(speedE) so completely ignoring direction. So with enough speed it could even get an extrusion where yu wanted to retract. Have to check it in more detail, but that is what might happen here.

@Nibbels
Copy link

Nibbels commented Aug 30, 2017

Any news on this?
Only some cheap if(!isXYZMove() || !isEMove() || (isXOrYMove() && isENegativeMove()) )
in motion.cpp did not fix it on my side??

@qx1147
Copy link

qx1147 commented Aug 30, 2017

I think you always want to have advanceL=0 during retraction, so it should be:

if (!isXYZMove() || !isEPositiveMove())

 

In addition, you could try forcing a break in the path planning (in updateTrapezoids()) by replacing

if(previous->isEOnlyMove() != act->isEOnlyMove())

with

if(previous->isEOnlyMove() != act->isEOnlyMove() || previous->isEPositiveMove!=act->isEPositiveMove)

@repetier
Copy link
Owner

I had some thought son this but no solution yet. Problem is to have a cheap detection solution to find the cases where we do not want advance.

Advance wanted:
W1 - Extrusion while moving - printing

Not Wanted:
U1 - Pure E-move
U2 - Retracting E-move
U3 - Undo retraction while moving

Especially U3 is here hard to detect since it is very similar to W1. For U3 we could say if previous was a retraction don't advance but at that place we have no previous.

U2 is easy by adding one term:
if(!isXYZMove() || !isEMove() || isENegativeMove()) {
U1 is already detected and W1 is the default anyway.

So with this it comes down to the question how simplify undoes retraction. Is it a pure E-Move then it is ok, is it while moving we need to know if previous was a retraction.

@qx1147
Copy link

qx1147 commented Aug 31, 2017

Hm. Does U3 happen in practice (and if it does, wouldn't advance make as much sense as no advance)?
!isEMove() || isENegativeMove() simplifies to !isEPositiveMove(), doesn't it?

Anyway, in this particular case I would have guessed that adding (isXOrYMove() && isENegativeMove()) should have done the trick as well and the question is why it didn't. Lets see whether forcing a break in the path planning helps.

@Nibbels
Copy link

Nibbels commented Aug 31, 2017

Ok, I tested your suggestions:
screenshot_2
screenshot_1
This combination doesnt seem to work properly. But remember that I am still using this old conrad repetier firmware which I patched up to a very nice state by myself. It all works, I guess.. but not with "Wipe while Retract" compiled by S3D.

@repetier
Copy link
Owner

@qx1147 U3 would be important if it happens since it is done with higher speed then normal printing and then you have even more advance at the start. So that is a case you surely do not want and also why I catch the no move but extruder.

!isEMove() || isENegativeMove() simplifies to !isEPositiveMove(), doesn't it?
Yes, that is quicker.

@Nibbels Not sure what that should change at your problem. You only fix junction speeds there, but that does not say anything about not using advance.

@Nibbels
Copy link

Nibbels commented Aug 31, 2017

I just followed @qx1147 #699 (comment)
I never really worked myself into path planning and more or less feel blind on this topic. That is the reason why I just tested this hint from @qx1147.
All I know is that there is some cache getting filled up. It is 16 long on my printer and if the data within goes under 10 I should have a speeddown... And that there is this problem of having continues speeds. Otherwise I should get blobs. But I never looked into that more deeply.

@qx1147
Copy link

qx1147 commented Aug 31, 2017

Regarding U3, I just couldn't think of a useful case where the slicer would produce U3 G-code. Anyway, as you say, U3 is hard to catch.

Regarding the path planning, the idea was that a break might prevent some AdvanceL handling from choking over the on-the-fly extruder reversal. In a way, this reasoning might not be so much different from the reasoning that is behind the already implemented condition. But yeah, this break is not forcing the move (and advance) to come to a full stop before continuing with wipe and retraction. So, sorry for the useless suggestion.

@Nibbels
Copy link

Nibbels commented Aug 31, 2017

I thought much about this problem. But I cannot think it through because I have lacks in knowledge.

We try to generate some ON-OFF condition. But that is not what we actually need when we ignore some possible effect of microoptimisation - I think.

I guess that we have some incredible acceleration while retract. We have less acceleration while printing.
So my very cloudy thoughts say that the higher the acceleration is the lower L should be used?
Maybe something like L_max = L_standard/acceleration
But this could be absolute nonsense.

Or we should just allow negative Advance because faster then fastest isnt possible. Either way - retract and push.

@repetier
Copy link
Owner

Just tried to create an example that makes the problem. Not sure why, but simplify3d did not create your code. I used latest 4.0 with wipe and here is what I get:

G1 X130.222 Y130.196 E414.0965
G1 X129.658 Y129.813 E414.1073
G1 X129.865 Y129.606 E414.1119
G1 E413.3619 F3000
; inner perimeter
G1 X127.589 Y128.912 F9000
G1 X127.414 Y128.710 F9000
G1 X127.086 Y128.286 F9000
G1 X126.635 Y127.613 F9000
G1 X126.368 Y127.147 F9000
G1 X126.126 Y126.667 F9000
G1 X125.911 Y126.174 F9000
G1 X125.723 Y125.670 F9000
G1 X125.563 Y125.156 F9000
G1 X125.432 Y124.635 F9000
G1 X125.329 Y124.107 F9000
G1 X125.256 Y123.575 F9000
G1 X114.114 Y124.804 F9000
G1 E414.1119 F3000
G1 X113.889 Y125.556 E414.1243 F2684
G1 X113.709 Y126.037 E414.1324
G1 X113.503 Y126.508 E414.1405

As you see it does plain retract, then wipes goes to next position, unretracts and prints. That way it prevents the problem completely. So I guess we can ignore U3 for now and only handle the additional case of retract while moving, which is catched by

    if(!isXYZMove() || !isEPositiveMove()) {
#if ENABLE_QUADRATIC_ADVANCE
        advanceRate = 0; // No head move or E move only or sucking filament back
        advanceFull = 0;
#endif
        advanceL = 0;

@Nibbels
Copy link

Nibbels commented Aug 31, 2017

Here are some samples S3D did produce:

686G1 X45.971 Y98.751 E6.3065
687G1 X45.975 Y98.750 E6.3066
688G1 X46.143 Y98.707 E6.3096
689G1 X46.146 Y98.706 E6.3097
690G1 X47.369 Y98.428 E6.3315
691G1 X47.746 Y98.343 F1584
692G1 X47.759 Y98.341 F1584
693G92 E0
694G1 X48.439 Y98.264 E-0.5129 F2400
695G1 X48.452 Y98.264 E-0.5230
696G1 X48.955 Y98.264 E-0.9000
697G1 X52.755 Y98.264 F2400
698G1 Z0.325 F1002
699; inner perimeter
700G1 X48.438 Y105.310 F3600
701G1 Z0.225 F1002
702G1 E0.0000 F1800
703G92 E0
704G1 X48.866 Y105.224 E0.0076 F1315
705G1 X138.021 Y105.224 E1.5605
706G1 X137.970 Y105.475 E1.5650
707G1 X137.916 Y105.660 E1.5683
708G1 X137.721 Y105.696 E1.5718
709G1 X135.352 Y106.018 E1.6134
710G1 X129.298 Y106.627 E1.7194
711G1 X124.282 Y106.987 E1.8070
712G1 X113.029 Y107.490 E2.0032
713G1 X109.205 Y107.808 E2.0700
714G1 X106.083 Y108.197 E2.1248
1039G1 X89.659 Y163.758 E0.4176
1040G1 X89.495 Y163.441 E0.4238
1041G1 X89.398 Y163.255 F1315
1042G1 X89.543 Y163.131 F1315
1043G92 E0
1044G1 X89.847 Y163.354 E-0.2827 F2400
1045G1 X90.053 Y163.477 E-0.4628
1046G1 X90.428 Y163.657 E-0.7749
1047G1 X90.585 Y163.714 E-0.9000
1048G1 X90.657 Y163.741 F2400
1049G1 X91.068 Y163.849 F2400
1050G1 X91.314 Y163.889 F2400
1051G1 X91.747 Y163.916 F2400
1052G1 X91.989 Y163.909 F2400
1053G1 X92.227 Y163.886 F2400
1054G1 X92.647 Y163.804 F2400
1055G1 X92.882 Y163.734 F2400
1056G1 X93.270 Y163.577 F2400
1057G1 X93.485 Y163.466 F2400
1058G1 X93.841 Y163.237 F2400
1059G1 X94.131 Y162.996 F2400
1060G1 Z0.325 F1002
1061; outer perimeter
1062G1 X93.833 Y162.613 F3600
1063G1 X93.841 Y162.607 F3600
1064G1 X94.096 Y162.336 F3600
1065G1 X94.315 Y162.038 F3600
1066G1 X94.415 Y161.870 F3600
1067G1 X94.575 Y161.536 F3600
1068G1 X94.643 Y161.350 F3600
1069G1 X94.739 Y160.984 F3600
1070G1 X94.772 Y160.784 F3600

SmartphoneStand_slong.rar.txt

There is Wipe Nozzle + Retract and those combined with this advanced settings checkbox. Pusing of filament to its pre-retract state has not been a special condition. Only retraction (-E) was combined.

I'll try ´ if(!isXYZMove() || !isEPositiveMove()) {´ again, if my print is finished.

@Nibbels
Copy link

Nibbels commented Aug 31, 2017

screenshot_1

@repetier
Copy link
Owner

At least your version did also unretract without move so both do not require U3.

What Simplify3D version did you use? An older one that you still get retract during move or is there a setting that you want retract while wipe and not wipe after retract like S3D 4.0 seems to do?

@Nibbels
Copy link

Nibbels commented Aug 31, 2017

screenshot_3
Did you acknoledge this checkbox?
screenshot_2

This old code was 3.1.1 as stated at the first line of .gcode in the rar
; G-Code generated by Simplify3D(R) Version 3.1.1
; Jul 3, 2017 at 7:39:44

But my tests with those two possibilitys where done with 4.0.0
Here they are:
screenshot_4

stack_box.rar.txt
Compared:
screenshot_5

@repetier
Copy link
Owner

You are right that setting makes it retract during wipe. Now I also get the wipe. Will see what the print will look like with this. But maybe my L is too small for it to see. Will test "fixed" version like I suggested.

@Nibbels
Copy link

Nibbels commented Aug 31, 2017

If you take L to around L= {40 .. 70} you should be able to see blobs. At around L ={100 .. 200} I got effects like stepper jumps and so on (At least with my stiffer PLA).

@Nibbels
Copy link

Nibbels commented Sep 28, 2017

Well :)

I got it! Your fix was just right and really awsome!
-> My only problem was that I included your fix at the wrong position. Therefore I wasnt able to see its effect.

The pitfall on my side was that I had two code-locations for the same thing which renkforce implemented in 2014. They have a second coordinate system to do pause moves (and something like babystepping and so on) and the other to hold the parts paths.
I knew about that but didnt see that I tested the fix within the wrong function
(Which looks like a clone with minor changes)

Now it works like a charm.
Thank you for your work and all the commits I am reading carefully these days!!
At least my part of this issue is totally closed now 👍

Greetings!


And...
Prio lowest, but knowing that you tend to fix comments when you see them:

https://github.com/repetier/Repetier-Firmware/blob/development/src/ArduinoAVR/Repetier/Printer.cpp#L40
https://github.com/repetier/Repetier-Firmware/blob/development/src/ArduinoAVR/Repetier/Printer.cpp#L473
-> acceleration should be mm/s^2 not ^3 ?

And sometimes you cast F_CPU to float like this:
float timeForMove = (float)(F_CPU) ...
and sometimes like this:
float toTicks = static_cast(F_CPU) / stepsRemaining;
which probably results in the same but you will know which is the prettier version :)
https://github.com/repetier/Repetier-Firmware/blob/development/src/ArduinoAVR/Repetier/motion.cpp#L521
https://github.com/repetier/Repetier-Firmware/blob/development/src/ArduinoAVR/Repetier/motion.cpp#L487
https://github.com/repetier/Repetier-Firmware/blob/development/src/ArduinoAVR/Repetier/motion.cpp#L473

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants