fixes #69 #1222
fixes #69 #1222
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1222 +/- ##
============================================
- Coverage 36.32% 36.15% -0.17%
- Complexity 1106 1107 +1
============================================
Files 150 150
Lines 8587 8638 +51
Branches 874 883 +9
============================================
+ Hits 3119 3123 +4
- Misses 5156 5202 +46
- Partials 312 313 +1
Continue to review full report at Codecov.
|
ugs-core/src/com/willwinder/universalgcodesender/utils/Settings.java
Outdated
Show resolved
Hide resolved
ugs-core/src/com/willwinder/universalgcodesender/AbstractCommunicator.java
Show resolved
Hide resolved
|
First of all, nice job diving into the guts of UGS! Getting through all of the abstraction layers to plumb in a new feature like this is a big accomplishment. I think this is working because you've accidentally tied single-step mode and single-block mode to the same parameter, without that real-time pauses would interrupt the stream until the send buffer was full. Related to that, I suspect that the machine doesn't actually do anything until 2 commands are sent (probably not noticeable because the first commands are generally setting up state). In any case, implicitly enabling single-step mode when single-block mode is set is probably the right move. Or maybe I'm mistaken about this because you are pausing at such a low level. I also think that you might need to send I do have some larger concerns specifically because this is cutting across all of the core abstraction layers. In general I've been trying to keep features like this as high level as possible. With that in mind I have two alternative implementations to consider:
In both cases, perhaps I'm also nervous about adding in implicit pauses at this low level. In general most of the core UGS bugs are related to synchronizing state between communicator/controller/backend, so adding features using the approach you're using is sort of a last resort (and one of the major motivators for the UGS platform). |
|
Curious what you think about these comments @breiler since you've been cleaning up these layers a lot in the last year. |
| !peekNextCommand().getCommandString().isEmpty() ) { | ||
|
|
||
| logger.log(Level.INFO, "singleBlockMode: pausing"); | ||
| pauseSend(); |
There was a problem hiding this comment.
I think dispatchListenerEvents(PAUSED, ""); is needed here.
|
Hi Will,
I appreciate the feedback! I was looking at this wondering if a strategy
pattern might be helpful for handling the different controllers, but unsure
yet.
I just experimented with adding a `dispatchListenerEvents(PAUSED, "some
message");` to the end of the `streamCommands()` while loop, and it did
give some nice feedback to the DRO which I was wondering how to do, it also
paused prematurely and sent a feed hold to Grbl which was far from intended
behaviour (I merely wanted to prevent more gcode from streaming; this way
the machine could be jogged / whatever else if necessary). I just ran
through my test ngc code after correcting the silly singleStepMode typo,
and it worked as intended (without the two commands issue you suspected
that might happen) - my test ngc was purely G0 and G1 commands to move X
then "peck drill" a hole and move X again, no setups or comments, I suspect
some G/M codes for setting metric/inch etc would just send and pause almost
instantly, though EEPROM commands that change to singleStepMode still need
to be tested for completeness.
Concerning the two approaches, I think I prefer doing the pause in the
`commandSent` callback, but I'll need to look at it with fresh eyes
tomorrow before I can say for sure.
I also considered adding a `source` field to GCodeCommand, this might have
some merit, particularly in combination with the next thought: Perhaps a
single priority queue of GcodeCommands with a source field is a cleaner
approach?
No matter what we do, some further documentation for `queuedCommands`,
`activeCommands`, and `streamCommands` (or a single, well-documented,
priority queue object) would go a long way to improving the long term
maintenance. A priority queue with a `Comparator` [which would take the new
`GcodeCommand.source` field into account], which can decide on the ordering
is neat, reusable (if needed), and standard `push()`, `pop()` and `peek()`
methods common to `Collections` are a nice result.
Thanks again for the feedback! Looking forward to what @breiler thinks of
the strategies you proposed as well.
…On Tue., 30 Apr. 2019, 23:25 Will Winder, ***@***.***> wrote:
***@***.**** commented on this pull request.
While it looks like this would work, the relationship between these
components is a bit more delicate than you may realize. Let's consider the
alternatives before merging this in.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1222 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADVRKDWUNDGP2OP4L3BAJ3PTBCEBANCNFSM4HJE6NIA>
.
|
|
It's a lot harder to be on the reviewing side than I imagined... Regarding the comments, I don't think they reflect the function as you've described yet. I'm thinking that for this method to work, both singleStep and singleBlock needs to be activated and if I were to implement a new Communicator I would probably not get it right based on the comments. Intuitively I would probably expect that the setSingleStepMode would function as a way to enable single stepping a gcode program. So we might have other issues with the naming of some existing methods. 😬 I think you have a good point @winder about these functions being a bit too low level. I have been thinking about adding a function for creating breakpoints on specific lines, but don't have any ideas on strategies on how to get there. But it sounds like this would be possible with the second option. I'm sorry for not having any concrete ideas on how to achieve this... |
|
@carneeki I think that updating "pause" would be the simplest way to implement this. I'm not sure about allowing jogging during a stream though, seems like that would be hard to get right and potentially dangerous. There is some documentation about all of this, but it is not detailed or complete: @breiler breakpoints would be pretty cool. The gcode stream's line number is the original line number for that reason (the gcode editor / line highlighter uses it in this way to map the original source line to the expanded lines), so even if an arc is expanded to a series of lines each of the segments would be tagged with the original line number. This package does breakpoints well, and UGS almost has all the pieces available where it could have something similar: https://github.com/duembeg/gsat |
Single Block Mode: Useful for testing a g-code program.
To enable
Behaviour
Each line of the file will require pressing play to continue to the next line.
This mode will NOT change the Z position of the tool. For testing programs, I recommend taking the cutter out, and then testing your program.
Untested (please help!)