Correct angle calculation

[blurfl]

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]

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]

Way to fix that quickly!! Awesome. 👍👍

[blurfl]

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]

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]

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

[MaslowCommunityGardenRobot]

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

[BarbourSmith]

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]

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 ***@***.***> 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 <#477 (comment)>, 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]

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]

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? ---