MaslowCNC / Firmware

Maslow Firmware
GNU General Public License v3.0
261 stars 136 forks source link

No Z axis doesn’t have the motion error checking #466

Open blurfl opened 5 years ago

blurfl commented 5 years ago

The Z axis doesn’t have the motion error checking that the X and Y axes have. As a result, a failed Z axis doesn't stop a gcode program. One place is to check the z axis in the same place that the other two axes are checked. Here are some questions:

    • This would put up the same error dialog that the other axes use, which directs the user to https://github.com/MaslowCNC/Firmware/wiki/Keeping-Up. Would it be sufficient to update that page to address the Z axis case, or does the Z axis need a separate alarm text?
    • The X and Y axes use sysSettings.positionErrorLimit, which defaults to 2mm. That's too large an error to accept for the Z axis, but 0.0 isn't reasonable. One possibility is using sysSettings.positionErrorLimit divided by 10, which would avoid introducing another variable. Changing that variable to silence the alarm for X and Y would silence it for Z as well.

I have a branch ready to submit that uses the above approach, are changes needed?

Note: be careful testing, I think I damaged a driver chip interrupting the Z axis connection.

BarbourSmith commented 5 years ago

I like that approach. I think using the same error message is the right way to go. I think that a divide by 10 approach is good but I'm a little worried that 10 would be too big. I think we might see a .2mm error more often than we want and we wouldn't want to pause the run because of that. How would you feel about a divide by 5? Is there a place that users can set the maximum z-axis feed rate if their setup can't keep up?

I'm going to be dog sitting for my cousins for the next two days so I might not be able to test before the time window so we should make an effort to recruit more testers.

blurfl commented 5 years ago

I’ll wait to open a PR - lots of time before the end of the month!😄 zMaxFeed is calculated from the pitch and maxZRPM, which is reported as $18 but isn’t available to change. I did my testing on the bench, motors with no load. I’ll do some more testing with the sled in service.

davidelang commented 5 years ago

On Sun, 14 Oct 2018, BarbourSmith wrote:

I like that approach. I think using the same error message is the right way to go. I think that a divide by 10 approach is good but I'm a little worried that 10 would be too big. I think we might see a .2mm error more often than we want and we wouldn't want to pause the run because of that. How would you feel about a divide by 5? Is there a place that users can set the maximum z-axis feed rate if their setup can't keep up?

remember, we are measuring the encoder position, not the actual router position, so .2mm is actually quite a bit of motor movement.

David Lang

blurfl commented 5 years ago

measuring the encoder position, not the actual router position

Pretty sure that zAxis.error() is in mm of z axis movement; it's the same calculation that brings left- or rightAxis.error(), returning encoderErr _mmPerRotation

blurfl commented 5 years ago

might not be able to test before the time window

With the release cycle pushed out to the first of each month, maybe we can spread the time window as well - a week (or two...) maybe? That would mean that really exciting changes submitted during the last week would either miss the cut or need special attention, but it would ease the burden of getting PRs tested before the time window shuts.

blurfl commented 5 years ago

I had a chance to test this change with a (much) higher speed z motor which has a lot of trouble keeping up; this needs more work before it's ready for the world. I suppose using the XY positionErrorLimit value or a separate setting for the Z are possibilities.

BarbourSmith commented 5 years ago

Great test!

Would the much higher speed motor have worked OK if there was a user option for setting the z-axis speed with $18?

blurfl commented 5 years ago

After more testing, It looks like the Z axis alarm value should be a separate parameter from the XY value, to accommodate various gear ratios and Z axis mechanisms. I'll wait until after the November release to do a PR.

BarbourSmith commented 5 years ago

Sounds excellent!

blurfl commented 5 years ago

Opinions on what should the default value be? I lean toward choosing a value that would make the alarm inactive, there have been quite a few support threads on the forum caused by the XY alarm from users with uncalibrated or suboptimal machines. This would be a setting for fine tuning after the machine was up and working.

BarbourSmith commented 5 years ago

I think that is a smart way to do things. Having a large threshold to begin with would lead to fewer folks running into issues. Users that want to have a tighter threshold can adjust it. From playing around with it what value seems reasonable to you?

blurfl commented 5 years ago

I'll have to do more testing once I have the separate variable set up.