fixes #69

[carneeki]

Single Block Mode: Useful for testing a g-code program. To enable

1. Tools -> Options, go to the UGS tab
2. Tick the "Enable Single Step Mode" check box.
3. Tick the "Enable Single Block Mode" check box.
4. Click OK.

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!)

1. How comments will be treated.
2. If pressing CS on Grbl to continue will work.
3. Other controllers (TinyG, Smoothie etc..). I have only tested this on a Grbl board, which didn't have buttons hooked up so I could not test the above line item.

[codecov-io]

Codecov Report

Merging #1222 into master will decrease coverage by 0.16%. The diff coverage is 7.84%.

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

Impacted Files	Coverage Δ	Complexity Δ
...der/universalgcodesender/AbstractCommunicator.java	63.63% <ø> (ø)	21 <0> (ø)	:arrow_down:
...der/uielements/panels/ConnectionSettingsPanel.java	0% <0%> (ø)	0 <0> (ø)	:arrow_down:
...er/universalgcodesender/utils/SettingsFactory.java	8.33% <0%> (-0.11%)	2 <0> (ø)
.../universalgcodesender/utils/GcodeStreamReader.java	65.78% <0%> (-20.42%)	11 <0> (ø)
...inder/universalgcodesender/AbstractController.java	60.38% <0%> (-0.89%)	97 <0> (ø)
...lwinder/universalgcodesender/model/GUIBackend.java	47.5% <100%> (+0.13%)	54 <0> (ø)	:arrow_down:
...illwinder/universalgcodesender/utils/Settings.java	46.15% <16.66%> (-0.74%)	41 <0> (ø)
...der/universalgcodesender/BufferedCommunicator.java	75.51% <9.09%> (-11.69%)	46 <1> (+1)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 82ec70c...f3cdcf9. Read the comment docs.

[winder]

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 dispatchListenerEvents(PAUSED, ""); to synchronize the communicator (more on this at the end).

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:

1. add a streamOneCommand() method to IController (implemented generically in AbstractController). GUIBackend:send would be able to call that instead of beginStreaming. This could get tricky because of the state maintained in the controller that gets initialized during beginStreaming, maybe a initStream would be needed followed by beginStreaming or streamOneCommand.
2. Perform the pause in the commandSent callback. This one is also not perfect because of the real-time pause. Maybe pause could be updated with pause(Boolean realTime). This is probably the least intrusive approach.

In both cases, perhaps GcodeCommand would need to be updated with some sort of "source" or "type" flag (i.e. ad-hoc / streaming command)

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).

[winder]

Curious what you think about these comments @breiler since you've been cleaning up these layers a lot in the last year.

[carneeki]

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, [email protected] wrote:

@winder 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 https://github.com/winder/Universal-G-Code-Sender/pull/1222#pullrequestreview-232166847, or mute the thread https://github.com/notifications/unsubscribe-auth/AADVRKDWUNDGP2OP4L3BAJ3PTBCEBANCNFSM4HJE6NIA .

[breiler]

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 would much rather have the pause before the command is sent. That way it could be possible to add a "step next"-action that would peek for the next command in the queue and somehow set a pause flag before sending the next command.

I'm sorry for not having any concrete ideas on how to achieve this...

[winder]

@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: source website

@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

[codecov-commenter]

:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

:x: Patch coverage is 9.80392% with 46 lines in your changes missing coverage. Please review. :white_check_mark: Project coverage is 36.16%. Comparing base (82ec70c) to head (22d7ede). :warning: Report is 873 commits behind head on master.

Files with missing lines	Patch %	Lines
...der/universalgcodesender/BufferedCommunicator.java	9.09%	19 Missing and 1 partial :warning:
.../universalgcodesender/utils/GcodeStreamReader.java	0.00%	9 Missing :warning:
...inder/universalgcodesender/AbstractController.java	0.00%	6 Missing :warning:
...der/uielements/panels/ConnectionSettingsPanel.java	0.00%	6 Missing :warning:
...illwinder/universalgcodesender/utils/Settings.java	33.33%	4 Missing :warning:
...er/universalgcodesender/utils/SettingsFactory.java	0.00%	1 Missing :warning:
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #1222      +/-   ##
============================================
- Coverage     36.32%   36.16%   -0.16%     
- Complexity     1106     1108       +2     
============================================
  Files           150      150              
  Lines          8587     8638      +51     
  Branches        874      883       +9     
============================================
+ Hits           3119     3124       +5     
- Misses         5156     5201      +45     
- Partials        312      313       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.