Correct angle calculation

blurfl commented 6 years ago

This PR addresses Issue #476: "Motion:arc() bug - G2 gcode cuts counterclockwise instead of clockwise"

The variable theta used in the incremental motion calculation is defined to contain the value direction indicated by G2/G3. This PR removes a second reference to direction in that calculation to correct the issue that arises when a negative direction multiplied twice (G2 instance) cancels the direction. The effect was that G2 gcodes cut in the positive direction (as G3 does) instead of the correct direction.

Here is a cut that demonstrates the error and the fix. It should run a 1" counterclockwise circle (G3) to the right of home, with Z beginning at 0 and ending at 0.5, then a clockwise circle (G2) to the left of home with Z finishing at 0. Without the patch, the G2 circle runs counterclockwise.

G20 G90
G0 X0 Y0 Z0
G3 X.0 Y0.001 Z-0.5 I1.0 J0.0 F100
G0 X0 Y0
G2 X.0 Y0.001 Z0.0 I-1.0 J0.0 F100
G0 X0 Y0
M2

MaslowCommunityGardenRobot commented 6 years ago

Congratulations on the pull request @blurfl

Now we need to decide as a community if we want to integrate these changes. You should vote by giving this comment a thumbs up or a thumbs down. Votes are counted in 48 hours. Ties will not be merged.

I'm just a robot, but I love to see people contributing so I'm going vote thumbs up (but my vote won't count...)!

BarbourSmith commented 6 years ago

Way to fix that quickly!! Awesome. 👍👍

blurfl commented 6 years ago

I introduced a pretty major flaw in the previous release - G2 gcodes will spoil a user's expensive material. This PR corrects that, and looks likely to merge. Is there a way to avoid 30 days of damage before the next release?

BarbourSmith commented 6 years ago

I think we could do an early release no problem. Should I just merge this right away and we could do a release today? We could also wait a few days if we want more time for testing.

blurfl commented 6 years ago

The user who identified the issue says he will test it this week. Let’s wait to hear from him.

MaslowCommunityGardenRobot commented 6 years ago

Time is up and we're ready to merge this pull request. Great work!

BarbourSmith commented 6 years ago

I just realized that I only increased the merge time to one week for Ground Control and the firmware is still 48hrs. Do we want one week on both or does 48hrs seem like enough time?

servant74 commented 6 years ago

If we are really paying attention, 48 hrs is enough. If not, the week is best. If it is contriversial (lots of input) then 48 is probably best.

We will defer to your judgement Bar.

On Wed, Nov 7, 2018 at 5:34 PM BarbourSmith notifications@github.com wrote:

I just realized that I only increased the merge time to one week for Ground Control and the firmware is still 48hrs. Do we want one week on both or does 48hrs seem like enough time?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/MaslowCNC/Firmware/pull/477#issuecomment-436817229, or mute the thread https://github.com/notifications/unsubscribe-auth/AAlLkqxzQMEAcLWMeCgspbn-n8-Brhriks5us24LgaJpZM4YPXjv .

--

<> ... Jack

If you are not paying for something, you are not a consumer, you are the product. - Chamath Palihapitiya

"Tell me and I forget. Teach me and I remember. Involve me and I learn." - Ben Franklin

davidelang commented 6 years ago

On Wed, 7 Nov 2018, BarbourSmith wrote:

I just realized that I only increased the merge time to one week for Ground Control and the firmware is still 48hrs. Do we want one week on both or does 48hrs seem like enough time?

I think the time delay is more important for firmware than for GC, so I would say we want it for both.

blurfl commented 6 years ago

The reason for the delay is to have more people test and report. Two weeks would be my vote, but one week is better than 48 hours. --- Did anyone besides me test this PR? ---

MaslowCNC / Firmware

Correct angle calculation #477