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

Send now fix #924

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Send now fix #924

wants to merge 3 commits into from

Conversation

kliment
Copy link
Owner

@kliment kliment commented Mar 25, 2018

This adds checksums and line numbers to manual commands - needs to be tested with manual commands interleaved with prints

@hroncok
Copy link
Collaborator

hroncok commented Mar 25, 2018

Should this block the next rc?

@kliment
Copy link
Owner Author

kliment commented Mar 26, 2018

Should this block the next rc?

No. This needs extensive testing and has the potential to break core functionality, and we are otherwise release-ready in my opinion so we can merge this after 2.0 is out once it's been tested some more.

@rockstorm101
Copy link
Collaborator

Just some thoughts here. If needed, the stable branch could be recovered, to hold the preparation for the 2.0.0 release. And this PR could be merged into the current development branch (a.k.a. "2.x") so it can suffer proper testing. And perhaps current 2.x should be made the default branch, and leave current master for 1.x fixes.

@kliment
Copy link
Owner Author

kliment commented Mar 26, 2018

I like the idea of stable but because we're so close to releasing 2.0.0 now I think this can wait until after release. @VanessaE has promised to test this, as it fixes an issue she reported, so I expect we'll hear pretty soon if something is seriously broken.

@kliment
Copy link
Owner Author

kliment commented Mar 26, 2018

That said, @rockstorm101 I think we should merge 2.x into master after branching current master into 1.x branch. It will merge cleanly after reverting all the fixes I backported from 2.x to 1.x. What do you think?

@hroncok
Copy link
Collaborator

hroncok commented Mar 26, 2018

And perhaps current 2.x should be made the default branch, and leave current master for 1.x fixes.

Please don't. That had always bitten me. If 1.x needs to have it's own branch for fixes, let it have a branch. But keep master for where development happens (or is being actively merged into), as god intended.

@kliment
Copy link
Owner Author

kliment commented Mar 26, 2018

Heh, @hroncok I beat you to it by 42 seconds :)

@hroncok
Copy link
Collaborator

hroncok commented Mar 26, 2018

Also, we don't need to revert anything: see for example https://github.com/hroncok/Printrun/tree/_master

Once we decide to merge 2.x into master, I can do it in a similar fashion.

@kliment
Copy link
Owner Author

kliment commented Mar 26, 2018

Yeah, either way works for me. Incidentally, I'm happy to have 2.x merged into master right now, as long as a 1.x branch is made first. Do you want to do it @hroncok ?

@rockstorm101
Copy link
Collaborator

If 1.x needs to have it's own branch for fixes, let it have a branch. But keep master for where development happens (or is being actively merged into), as god intended.

I'm not sure I understand what you mean, sorry. I believe we want the same branch layout:

  • branch "master" (default) which holds the development and accepts new features and whatnot.
  • branch "stable" which branches from master at certain points and "freezes" it for release preparation.
  • branch "1.x" which holds the old 1.x Printrun, and accepts fixes for it.

@kliment
Copy link
Owner Author

kliment commented Mar 26, 2018

@rockstorm101 we both understood what you said as suggesting that 2.x be set to default, rather than merged into master. But good to see we're all on the same page.

@hroncok
Copy link
Collaborator

hroncok commented Mar 26, 2018

branch "master" (default) which holds the development and accepts new features and whatnot.

OK.

branch "stable" which branches from master at certain points and "freezes" it for release preparation.

I'm not strongly for or against this. Was it ever truly necessary?

branch "1.x" which holds the old 1.x Printrun, and accepts fixes for it.

done. https://github.com/kliment/Printrun/tree/1.x

@rockstorm101
Copy link
Collaborator

Damm, you two are way faster than my writing XD.

@hroncok hroncok closed this Mar 26, 2018
@hroncok hroncok changed the base branch from 2.x to master March 26, 2018 20:32
@hroncok
Copy link
Collaborator

hroncok commented Mar 26, 2018

Got closed automatically when 2.x was deleted.

@hroncok hroncok reopened this Mar 26, 2018
@hroncok
Copy link
Collaborator

hroncok commented Mar 26, 2018

I'm happy to have 2.x merged into master right now, as long as a 1.x branch is made first. Do you want to do it @hroncok ?

Done.

@rockstorm101
Copy link
Collaborator

rockstorm101 commented Mar 26, 2018

Was it ever truly necessary?

Honestly, I never thought it would be necessary for Printrun.

@VanessaE
Copy link
Contributor

VanessaE commented Mar 26, 2018

Initial results (master branch, at commit 6bf59b6):

Long macros appear to be executed properly and completely during active printing, but when the print continues after the macro, it does not put X/Y/Z back at their proper locations, so the print continues in mid-air since the last moves in the macro leave the nozzle at {100,100,200}. No warnings or errors.

If I try a long macro while the print is paused, the entire thing is not executed. No warnings/errors.

If I clear off the print bed and click "restart" (NOT "resume") after the failed macro, the print seems to start and behave normally. No warnings/errors, naturally.

If I use the manual command input while actively printing, the entire contents appear to be executed, but I get "Error:Line Number is not Last Line Number+1, Last Line: xxx" warnings after it. When the print continues, it's left printing in mid-air, as with the macro method. No other warnings/errors.

If I use the manual command input while paused, the entire contents of the macro are not executed. Usually no errors or warnings, but occasionally I might see something like echo:Unknown command: "GM105" (so temperature read commands aren't being properly queued?).

The test gcode (for both the macro test and manual commands) ```gcode M201 T2000 g1 f18000 g1 x0 g1 y0 g1 z1 g1 x200 g1 y200 g1 z200 g1 x0 g1 y0 g1 z1 g1 x200 g1 y200 g1 z200 g1 x0 g1 y0 g1 z1 g1 x200 g1 y200 g1 z200 g1 x0 g1 y0 g1 z1 g1 x200 g1 y200 g1 z200 g1 x0 g1 y0 g1 z1 g1 x200 g1 y200 g1 z200 g1 x100 y100 ```


EDIT: I did these tests without applying this pull request, but in retrospect I figure these results may be useful anyway.

@VanessaE
Copy link
Contributor

VanessaE commented Mar 27, 2018

AFTER applying this pull request, sending a long macro while printing was an epic fail (same gcode as in my previous comment):

[...]
G1 X200
G1 Y200
G1 Z200
G1 X100 Y100
Error:No Checksum with line number, Last Line: 89
Error:No Checksum with line number, Last Line: 89

Resend: 90
echo:Unknown command: "00"
Print paused at: 20:02:37 [<------ I paused the print out of habit, trying to stop the macro from finishing]
Error:Line Number is not Last Line Number+1, Last Line: 89
Error:Line Number is not Last Line Number+1, Last Line: 89

Resend: 90

It ran the macro's commands far slower than they should be (so I guess the "g1 f" command was ignored or corrupted), and in any case it did not execute the entire macro. After it finished, it threw this error:

Exception in thread Thread-8:
Traceback (most recent call last):
  File "/usr/lib/python3.5/threading.py", line 914, in _bootstrap_inner
    self.run()
  File "/usr/lib/python3.5/threading.py", line 862, in run
    self._target(*self._args, **self._kwargs)
  File "/home/vanessa/RepRap/Printrun-2.x/printrun/printcore.py", line 399, in _sender
    self._sendnext(now=True)
  File "/home/vanessa/RepRap/Printrun-2.x/printrun/printcore.py", line 583, in _sendnext
    self._send(self.sentlines[self.resendfrom], self.resendfrom, False)
KeyError: 90

After that, Pronterface stopped responding to the jog controls and further manual commands were ignored. I did not continue testing at this point, and git reset --hard HEAD to get back to normal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants