repetier / Repetier-Firmware

Firmware for Arduino based RepRap 3D printer.
812 stars 734 forks source link

A short look over "maxJunctionSpeed" #716

Open Nibbels opened 6 years ago

Nibbels commented 6 years ago

Hello,

(Again I am not using the original repetier firmware but fixing my old branded Renkforce/Conrad version. Thus my problems could just be my problem - but also be a good hint for original repetier development)

I digged myself into debugging again, because I wanna resolve some tiny microblobs at ends or starts of my retract junctions. http://www.rf1000.de/viewtopic.php?f=73&t=2071&p=21077#p21070 Those blobs might be just normal or the result of anything else, but:

So I had a look into the code and started comparing again. https://github.com/repetier/Repetier-Firmware/blob/bca22739f154e9f409a8feab9ca6ee54f1bda953/src/ArduinoAVR/Repetier/motion.cpp#L732

First I found this change: https://github.com/repetier/Repetier-Firmware/commit/be24acec2e35365c1e273e3c08f76359d96984b0#diff-593812a66d7348c87b711b15b1ad5195L707

Then I was following the path to https://github.com/repetier/Repetier-Firmware/blob/bca22739f154e9f409a8feab9ca6ee54f1bda953/src/ArduinoAVR/Repetier/motion.cpp#L739

Code:

    if(previous->isEOnlyMove() != act->isEOnlyMove()) {
        previous->maxJunctionSpeed = act->startSpeed;
        previous->setEndSpeedFixed(true);
        act->setStartSpeedFixed(true);
        act->updateStepsParameter();
        firstLine->unblock();
        return;
    } else {
        computeMaxJunctionSpeed(previous, act); // Set maximum junction speed if we have a real move before
    }

And Inside of function computeMaxJunctionSpeed https://github.com/repetier/Repetier-Firmware/blob/bca22739f154e9f409a8feab9ca6ee54f1bda953/src/ArduinoAVR/Repetier/motion.cpp#L827 There Is some special for USE_ADVANCE: This is checking kind of the same thing again with the special that it minimizes the speed according to the comment in code below:

#if USE_ADVANCE
    if(Printer::isAdvanceActivated()) {
        // if we start/stop extrusion we need to do so with lowest possible end speed
        // or advance would leave a drolling extruder and can not adjust fast enough.
        if(previous->isEMove() != current->isEMove()) {
            previous->setEndSpeedFixed(true);
            current->setStartSpeedFixed(true);
            previous->endSpeed = current->startSpeed = previous->maxJunctionSpeed = RMath::min(previous->endSpeed, current->startSpeed);
            previous->invalidateParameter();
            current->invalidateParameter();
            return;
        }
    }
#endif // USE_ADVANCE

I guess - but dont know and cannot overlook all of this - that this check in #if USE_ADVANCE is never evaluated because the case is sort out by the code at the top.

I mean: if(previous->isEOnlyMove() != act->isEOnlyMove()) { returns so that if(previous->isEMove() != current->isEMove()) { should be always wrong? -> So previous->endSpeed and current->startSpeed are never adjusted. previous->maxJunctionSpeed is not the smaller value, but just the next startspeed. Maybe doing computeMaxJunctionSpeed() before checking EOnly is the way to do it? I think that was the method changed here: https://github.com/repetier/Repetier-Firmware/commit/0ada2aa1426e004fd68b8c03cbbfe2edcfbd96da#diff-11347f18746fb080f5bda21c30428558L516

Another thing: if(previous->isEMove() != current->isEMove()) should be true when entering and leaving a retract, right? So It is correct having the "retract leaving maxjunctionspeed" set to the next paths start speed. And it is correct having the "before retract maxjunctionspeed" set to the retracts start speed. (I might be totally missguided here!)

It would be really awsome if you look over my problem here. Of course I might be able to play around with those values but if you make a statement or define some "best practice" on that I would be very glad.

Greetings from Stuttgart

Nibbels commented 6 years ago

Damn, ok. I saw it:

EOnlyMove vs. EMove ...

Sorry, if this missreading made all complicated. I have to rethink my position here. But the question if ADVANCE should have special threatment outside of computeMaxJunctionSpeed as well is still persistent.

Edit; And if entering or leaving a pure retract should be considered two cases.

And if previous->endSpeed = current->startSpeed should be updated with maxJunctionSpeed before quitting because of retract-only.

Nibbels commented 6 years ago

Ok, me again. Two things:

1) According to // if we start/stop extrusion we need to do so with lowest possible end speed // or advance would leave a drolling extruder and can not adjust fast enough.

For me the problem only seems to be there whenever some E-move is finishing, not starting. Wouldnt if(previous->isEMove() != current->isEMove() && previous->isEMove()) instead of if(previous->isEMove() != current->isEMove() ) be the better code? (If true you use the minimum speed for Junction) https://github.com/Nibbels/Repetier-Firmware/commit/7bd2f32ffc94fd13252b04f6d36f307ede61c59b#diff-b41781ad2ccef1b45ce8da5ca500778aR875

Or do I miss some following higher order problem here?

2) For retract style E-Only moves: I just guess that acceleration is no real deal on e-only-moves but for x and y moves. So maybe the junction-speed before some e-only-move should be the lowest possible as well. After the e-only-move we should start from the beginning jerk speed(?). (Especially this should be the case when Retracts tend to be in a corner where a slight overswing is quite a bad thing and make parts look ugly when having a slight overhang? Slicers might align the retracts to some edge to prevent additional z-stitch/z-Naht/...?)

If there is some broad consent on the fact, that Retracts should be within the flow(not changing dynamic) of X-Y-Movement and therefore not be considered to be an accurate move on another axis then we might have to ignore this. But nevertheless we should then accept the first comment when leaving an e-move with adrance activated? -> We should check if advance is active and in that case use min-speed? https://github.com/Nibbels/Repetier-Firmware/commit/7bd2f32ffc94fd13252b04f6d36f307ede61c59b#diff-b41781ad2ccef1b45ce8da5ca500778aL805

I would be very glad if someone remembers the best practice on that. For me: I think that I somehow finally got my personal best practise by slowing down both types. e-moves e-only-moves because advance is there and I dont want to have possible hardware overswing at corners.

Greetungs from Stuttgart

repetier commented 6 years ago

1.Switching from E-move to non e-move is more problematic as the extruder needs to unload the stress. But the other side is also not easy. We need to build up pressure according to speed so starting with higher speeds means starting with higher advance which needs to be catched up.

  1. Consider that F is interpreted differently for pure e moves and moves with head move. That is the main reason I reset here to normal min speeds. So yes, xy needs to be at safe junction speed at these cases. This is independent of advance on or of.

I think the line previous->maxJunctionSpeed = RMath::min(previous->endSpeed,act->startSpeed); should be simply removed. Since it was last move it ends with minim speed already. And pevious and act move use different feedrate interpretations, so that might be a better solution here.

Nibbels commented 6 years ago

Ok... thanks for your comment on that!

Some details got more clear for me.

You changed the place of computeMaxJunctionSpeed(previous, act); and this following line (previous->maxJunctionSpeed = previous->endSpeed; / previous->maxJunctionSpeed = act->startSpeed;) in that commits: https://github.com/repetier/Repetier-Firmware/commit/0ada2aa1426e004fd68b8c03cbbfe2edcfbd96da#diff-11347f18746fb080f5bda21c30428558L516 + https://github.com/repetier/Repetier-Firmware/commit/be24acec2e35365c1e273e3c08f76359d96984b0#diff-593812a66d7348c87b711b15b1ad5195L707 If I would just remove previous->maxJunctionSpeed = RMath::min(previous->endSpeed,act->startSpeed); and if computeMaxJunctionSpeed(previous, act); would stay within the "else". Would previous->maxJunctionSpeed still be filled with some valid value for all types of path?

repetier commented 6 years ago

No it would not be set, so just leave it. I think it is not relevant anyway since we also set end and start speed fixed without changing them. Main reason why it still worked:-) So if we are not allowed to change speeds it is clear with which speed junction happens.