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

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

Open
blurfl opened this issue Oct 14, 2018 · 12 comments
Open

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

blurfl opened this issue Oct 14, 2018 · 12 comments

Comments

@blurfl
Copy link
Collaborator

blurfl commented Oct 14, 2018

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
Copy link
Member

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
Copy link
Collaborator Author

blurfl commented Oct 15, 2018

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
Copy link
Contributor

davidelang commented Oct 15, 2018 via email

@blurfl
Copy link
Collaborator Author

blurfl commented Oct 15, 2018

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
Copy link
Collaborator Author

blurfl commented Oct 16, 2018

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
Copy link
Collaborator Author

blurfl commented Oct 17, 2018

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
Copy link
Member

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
Copy link
Collaborator Author

blurfl commented Oct 28, 2018

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
Copy link
Member

Sounds excellent!

@blurfl
Copy link
Collaborator Author

blurfl commented Oct 30, 2018

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
Copy link
Member

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
Copy link
Collaborator Author

blurfl commented Oct 30, 2018

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

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

3 participants