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

Lower the max feedrate #409

Merged
merged 3 commits into from
Mar 6, 2018
Merged

Lower the max feedrate #409

merged 3 commits into from
Mar 6, 2018

Conversation

BarbourSmith
Copy link
Member

It seems like we may have an issue where the motors can't keep up with the maximum feed rate under some circumstances.

We've seen several reports in the forums, and I just encountered one where during the calibration process the machine started to lower the z-axis before arriving at it's destination because it couldn't keep up.

This PR lowers the maximum feed rate to 700mm/min and sets the G0 rapid moves to use the same value as the maximum feed rate in settings.

@krkeegan
Copy link
Collaborator

krkeegan commented Mar 6, 2018

Oops missed a hardcoded feed rate.

To be clear, this just changes the default value. Users can still manually change this back using the $$ settings.

I don't think the option has been incorporated in GC yet.

@davidelang
Copy link
Contributor

davidelang commented Mar 6, 2018 via email

@krkeegan
Copy link
Collaborator

krkeegan commented Mar 6, 2018

There is no check that we reached the end of the G0. Each distance step happens in a fixed period of time, if that distance is greater than what can be achieved (exceeds the motor's max feed) then it won't make the end of the line before moving to the next command.

@davidelang
Copy link
Contributor

davidelang commented Mar 6, 2018 via email

@krkeegan
Copy link
Collaborator

krkeegan commented Mar 6, 2018

Would this "check" that we have reached the end point of a G0 command only apply to G0 commands?

We can't really do this with other commands, otherwise the machine would stutter a lot.

Personally, I would prefer to get the max feed rate dialed in correctly, even if we fix this for G0 commands, a G1 command could still request a feed rate too high and the same z-axis problem would occur. This is in part why the maxfeed is a user-definable setting.

@BarbourSmith
Copy link
Member Author

I agree that this is a stop gap, we should really figure out why the max feedrate is lower now. The goal here is just to make 1.09 a stable release to work from.

I agree with @davidelang that there should be a warning issued if a move completes and we are still far from the target location and I also agree with @krkeegan that if we add a pause at the end of every move we will see very jerky behavior and that having the max feedrate dialed in is the way to go. We actually had that check at one point and it did result in jerky behavior.

@BarbourSmith BarbourSmith merged commit 3d7763a into master Mar 6, 2018
@BarbourSmith BarbourSmith deleted the lower-the-max-feedrate- branch March 6, 2018 18:12
@davidelang
Copy link
Contributor

davidelang commented Mar 6, 2018 via email

@davidelang
Copy link
Contributor

davidelang commented Mar 6, 2018 via email

@krkeegan
Copy link
Collaborator

krkeegan commented Mar 6, 2018

I understand the thought process, but I still don't think this is the right approach. A few of my arguments:

  1. This is a significant departure from our current structure and would require a significant rework.
  2. Acceleration/deceleration isn't instantaneous. As the machine approaches a destination point it will start to slow down, not as fancy as the GRBL code, but this happens naturally because the error approaches 0 so the PID output approaches 0. If we try to hit the end point, stuttering cannot be avoided.
  3. The arduino is slow. To do this properly, it would have to be handled in the interrupt loop, which is already rather slow.
  4. Doing this doesn't do anything to save your part. If your feed rate is too high horizontal cuts will end up with a curved cut, the failure to keep up isn't tied to both motors, generally only the left or the right motor will not keep up. Making sure the final stop point is hit before moving on to the next cut won't fix this, you will still have a curved line.
  5. GRBL works the same way our current code works.
  6. More sophisticated machines also work the way our current code works. The laser cutter here at the office will start to round off corners if the feedrate is increased too much and you don't compensate in your g-code. If it did what has been proposed, as the feedrate was increased it would overcut each corner not under. We have a water jet that does it's own compensation, but if you turn it off, it will also undercut.

I also disagree that the max feed rate shouldn't have to be set by the user. Particularly if we add warnings that the machine isn't keeping up. This isn't a plug and play device, and making it so would restrict more advanced uses and would increase the complexity and resulting bugs in the code.

the max rate the machine can move is going to depend on where you are on the workpiece,

Here lies dragons, if you are opening the possibility to a dynamic feed rate, we need a lot more work and a much more complicated controller. I messed with this for a while on G0 moves and it gets messy fast, at minimum you need yet another PID controller to handle the dynamic decrease and increase.

The only possible design I can see for this is define some circle of acceptable error as "hitting the end point." This would decrease the stuttering, but now this opens up yet another user definable setting a faster moving machine would need a larger circle, a slower machine a smaller one. This setting is less measurable and would be more confusing for a user to enter. And still doesn't solve issue 4 above.

@davidelang
Copy link
Contributor

davidelang commented Mar 6, 2018 via email

@blurfl
Copy link
Collaborator

blurfl commented Mar 6, 2018

we should really figure out why the max feedrate is lower now

I'm not sure that is so. I recently looked at a couple of previous firmware versions, and v0.68 limited the feed rate 635, v0.76 limited it to 900.

@krkeegan
Copy link
Collaborator

krkeegan commented Mar 7, 2018

  1. This is a significant departure from our current structure and would require a significant rework.
    if so, our current structure is wrong and needs to be fixed

This is a an overly idealistic and simplistic statement. We have finite resources and time. Spending our resources to redesign our code to this degree has a cost in the form of delayed improvements and fixes for other features.

this is what dead zones are for

I assume this is what I called a circle of error.

  1. Doing this doesn't do anything to save your part. If your feed rate is too
    high horizontal cuts will end up with a curved cut, the failure to keep up
    isn't tied to both motors, generally only the left or the right motor will not
    keep up. Making sure the final stop point is hit before moving on to the next
    cut won't fix this, you will still have a curved line.
    a coordinated move should slow down the other axis to stay synced with the one that can't move fast enough.

OK, it sounds like my assumption is correct, you are not just referring to waiting for the final point to be reached before moving onto the next line, but rather describing a dynamic calculation for each step in the cut. Again, this is more complicated than your description.

Isn't this idea just a method of finding the max feed rate for each segment? Why wouldn't setting the max feed rate be a better more robust solution, since you only have to do it once? We can figure out where the feed rate is the most restricted, I think I made a chart at one point that would probably identify this (I think it is the lower corners moving straight up). Tell users to test their machine in that region and adjust until no warnings are received.

I still strongly believe dynamic adjusting is a bad idea. It is accompanied by all sorts of dials and knobs to adjust that add complexity. How large should the dead zone be? How much should we decelerate when the dead zone is exceeded? How quickly should we do this? Do we need to ignore this for some period when starting from a standstill? These things are no more likely to be consistent across machines than the max feed rate is.

Moreover, if max feed is wrong, every segment that exceeds this rate will have some error before the dead zone is hit and we can properly decelerate to catch up. Whereas if we dial in max feed correctly, we won't have this error. Plus if we ever get to a place where the firmware has a concept of alarms, we could stop the cut when we start to see an error, allowing the user to fix the feed rate error and resume or restart the cut.

I created a crude system to dynamically adjust the max feedrate for only G0 moves a long time ago with the thought that the max feed is about 30% higher in the center than on the edges. Increasing the feedrate of non-cutting G0 moves could cut down on the total manufacturing time for a part. Ultimately I threw this out as buggy, bug prone, too complex and not producing a lot of gain. I could be convinced that this is a good idea for G0 moves, but I don't think this should even remotely be a priority.

@davidelang
Copy link
Contributor

davidelang commented Mar 7, 2018 via email

@blurfl
Copy link
Collaborator

blurfl commented Mar 7, 2018

@krkeegan, some further thoughts...

  • it might be useful for the overspeed message to indicate which axis is calling for the overspeed.
  • Though the message is generated in Motor::write(), that gets its values from Motor::additiveWrite(), which bumps up the previous speed by the amount requested by the PID. Is there anything interesting about the incremental amount being requested?
  • _PIDController and MotorGearboxEncoder are both aware of outputLimits (+-255), and PID::Compute() seems to impose those as limits to the additional speed that can be requested.
  • The only check that the resulting speed (previous plus increment) command sent to Motor::Write() is that Motor::write clamps that to 255 if it is beyond the physical limit (PWM duty cycle).
  • There is no feedback up the chain that the limit has been reached. Motor::additiveWrite() is aware of the previous speed and , so additiveWrite() seems a logical source of correction, maybe an axisOverspeed flag for the coordinatedMove() routine?

@krkeegan
Copy link
Collaborator

krkeegan commented Mar 7, 2018

it might be useful for the overspeed message to indicate which axis is calling for the overspeed.

This can be done, but at the level I put this in, the code has no human conveyable notion of which motor it is talking too. It could be added if necessary, but I kinda felt like it wasn't needed as in my testing at least is seems pretty clear which one is the issue.

Though the message is generated in Motor::write(), that gets its values from Motor::additiveWrite(), which bumps up the previous speed by the amount requested by the PID. Is there anything interesting about the incremental amount being requested?

I don't think so.

_PIDController and MotorGearboxEncoder are both aware of outputLimits (+-255), and PID::Compute() seems to impose those as limits to the additional speed that can be requested.

Yeah, we never get anywhere near that. It seemed like a logical restriction to leave in place, but since the velocity controller is additive now, the numbers it outputs are much lower than this. But its output is added to the prior velocity, so we can still see numbers in excess of 255 where the test is.

The only check that the resulting speed (previous plus increment) command sent to Motor::Write() is that Motor::write clamps that to 255 if it is beyond the physical limit (PWM duty cycle).

yes.

There is no feedback up the chain that the limit has been reached. Motor::additiveWrite() is aware of the previous speed and , so additiveWrite() seems a logical source of correction, maybe an axisOverspeed flag for the coordinatedMove() routine?

I think in the long run, the right course of action is to alarm and stop the machine possibly a warning at some lower time threshold before stopping. But that requires a bit more work including solving #407 and #351.

@davidelang
Copy link
Contributor

davidelang commented Mar 7, 2018 via email

@blurfl
Copy link
Collaborator

blurfl commented Mar 7, 2018

@davidelang "that feedback is what we are adding."

Where are you adding this?

Edit - I see, the feedback is all the way to the operator. Now I follow.

@davidelang
Copy link
Contributor

davidelang commented Mar 7, 2018 via email

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

Successfully merging this pull request may close these issues.

4 participants