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

GenericGcodeDriver/Grbl: Fix for flip-setting also translating by bed width/height when startpoint is set #224

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pimsierhuis
Copy link
Contributor

We tried to use the "set startpoint" function to support the use case for jogging to the start point and set work-coordinates to zero. But our Y-coordinate also uses the flip-setting. The problem is, that this setting incorrectly still adds the bed height to the coordinates when flipping when the starting-point is set. This PR fixes this.

I can't imagine a use-case where the translate by bed-height when a startpoint is set is useful. Under this assumption, I consider this PR as backwards compatible.

@mgmax
Copy link
Collaborator

mgmax commented May 29, 2024

Looking at the one driver, this looks good, except when one sets the origin point at exactly (0,0).

Looking at all drivers, I'm a bit concerned that it is a workaround that will cause us extra effort in the future, to remove it again and to replace it with something that really solves the underlying problem.

The actual issue is not in the driver but in applyStartPoint() or however it is called, and in the interface of LaserJob (?) not distinguishing between "origin 0" and "origin default - use absolute positioning".
A longer discussion of the backgrounds can be found in #206 .

I guess that a proper solution would be to

  • have applyStartPoint() receive a default value from the laser driver
  • use origin=NaN instead of origin=0 if the origin is unset

and then we have this fixed for all drivers and even weird future cases where the default origin is in the middle or wherever. I think this shouldn't be too difficult, and these two steps could also be done separately. What's your opinion?

(What I'm writing here is from memory and may be not 100% accurate, please double-check yourself.)

@pimsierhuis
Copy link
Contributor Author

pimsierhuis commented Jun 7, 2024

Sorry for the late response.

So if I understand you correctly, you would prefer to move the flipping logic from the laserdriver and let the driver communicate to LaserJob if flipping is needed?

I would then propose to create a new function as an alternative to applyStartPoint(), lets say applyDriverCoords, where the driver can give it its origin in absolute visicut-coord terms (so that would be 0,0 for left top and 0, for left bottom for example) and two booleans if flipping on x and y axis is necessary. Then a driver can decide to use this generic way for converting from visicut- to driver-coords (by calling the new function) for backwards-compatibility reasons.

This method would then rewrite all coordinates (like applyStartPoint does now), but also respecting the drivers coordinate system. The flip-logic can then be removed from the driver.

Does this sound reasonable? Or did I misunderstood what you meant..

@pimsierhuis
Copy link
Contributor Author

@mgmax Any thoughts?

@mgmax mgmax marked this pull request as draft November 10, 2024 09:51
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.

2 participants