colinrgodsey / step-daemon

stepd - External planner and stepper for 3d printing
GNU General Public License v3.0
98 stars 8 forks source link

Octoprint TypeError serial crash #12

Closed lav8org closed 3 years ago

lav8org commented 3 years ago

2020-11-30 23:00:02,661 - octoprint.util.comm - INFO - Changing monitoring state from "Offline" to "Opening serial connection"
2020-11-30 23:00:02,663 - octoprint.plugins.stepd - INFO - Starting service: ['/.octoprint/data/stepd/step-daemon','device=/dev/ttyACM0', 'baud=115200', 'config=config.json']
2020-11-30 23:00:02,672 - octoprint.util.comm - INFO - Changing monitoring state from "Opening serial connection" to "Connecting"
2020-11-30 23:00:02,673 - octoprint.util.comm - INFO - M110 detected, setting current line number to 0
2020-11-30 23:00:02,673 - octoprint.util.comm - ERROR - Unexpected error while writing to serial port
Traceback (most recent call last):
File "/workspace/octoprint/venv/lib/python3.7/site-packages/octoprint/util/comm.py", line 4686, in _do_send_without_checksum
result = self._serial.write(to_send)
File "/workspace/octoprint/venv/lib/python3.7/site-packages/octoprint_stepd/StepdService.py", line 40, in write
return self.process.stdin.write(*args, **kwargs)
TypeError: write() argument must be str, not bytes
2020-11-30 23:00:02,674 - octoprint.util.comm - ERROR - Something crashed inside the serial connection loop, please report this in OctoPrint's bug tracker:
Traceback (most recent call last):
File "/workspace/octoprint/venv/lib/python3.7/site-packages/octoprint/util/comm.py", line 2171, in _monitor
line = self._readline()
File "/workspace/octoprint/venv/lib/python3.7/site-packages/octoprint/util/comm.py", line 3848, in _readline
null_pos = ret.find(b"\x00")
TypeError: must be str, not bytes

(edit: how do newline within code tag? 2nd search still didn't resolve, gave up)

o-nix commented 3 years ago

This is mine:

2020-12-16 18:05:53,452 - octoprint.util.comm - INFO - Changing monitoring state from "Offline" to "Opening serial connection"
2020-12-16 18:05:53,474 - octoprint.plugins.stepd - INFO - Starting service: ['/home/pi/.octoprint/data/stepd/step-daemon', 'device=/dev/ttyACM0', 'baud=500000', 'config=config.json']
2020-12-16 18:05:53,522 - octoprint.util.comm - INFO - Changing monitoring state from "Opening serial connection" to "Connecting"
2020-12-16 18:05:53,527 - octoprint.util.comm - INFO - M110 detected, setting current line number to 0
2020-12-16 18:05:53,528 - octoprint.util.comm - ERROR - Unexpected error while writing to serial port
Traceback (most recent call last):
  File "/home/pi/oprint/lib/python3.7/site-packages/octoprint/util/comm.py", line 4686, in _do_send_without_checksum
    result = self._serial.write(to_send)
  File "/home/pi/oprint/lib/python3.7/site-packages/octoprint_stepd/StepdService.py", line 40, in write
    return self.process.stdin.write(*args, **kwargs)
TypeError: write() argument must be str, not bytes
2020-12-16 18:05:53,530 - octoprint.util.comm - ERROR - Something crashed inside the serial connection loop, please report this in OctoPrint's bug tracker:
Traceback (most recent call last):
  File "/home/pi/oprint/lib/python3.7/site-packages/octoprint/util/comm.py", line 2171, in _monitor
    line = self._readline()
  File "/home/pi/oprint/lib/python3.7/site-packages/octoprint/util/comm.py", line 3848, in _readline
    null_pos = ret.find(b"\x00")
TypeError: must be str, not bytes
2020-12-16 18:05:53,536 - octoprint.util.comm - INFO - Changing monitoring state from "Connecting" to "Offline (Error: See octoprint.log for details)"
2020-12-16 18:05:53,544 - octoprint.plugins.stepd - INFO - GO:
2020-12-16 18:05:53,563 - octoprint.plugins.stepd - INFO - service terminated
lav8org commented 3 years ago

I recently found that python3 isn't as flexible/permissive as python2 with io type mismatch, could be that octoprint made the 2to3 transition since the plugin was updated? The fix could be as simple as changing the io init type parameter

o-nix commented 3 years ago

OctoPrint 1.5.2 Python 3.7.3 OctoPi 0.18.0

Exactly, 3rd version by default on new installations.

lav8org commented 3 years ago

Oh it's piping with standard streams, that's a little different, but may be a similar issue. There's no env #! on StepdService.py so only guessing it's a 2to3 issue. Per python3 docs the sys "standard streams are in text mode by default. To write or read binary data to these, use the underlying binary buffer. For example, to write bytes to stdout, use sys.stdout.buffer.write(b'abc')" see https://docs.python.org/3/library/sys.html#sys.stdin

Still researching if/how subp streams differ...

lav8org commented 3 years ago

Haven't found definitive clarification, but gut feeling is that they're the same. Don't have the time to try it myself atm, but you could try changing this line in your local copy of https://github.com/colinrgodsey/step-daemon/blob/da32986cfb787bb92c15114f8e9d57d614d4385b/octoprint-plugin/octoprint_stepd/StepdService.py#L40 from return self.process.stdin.write(*args, **kwargs) to return self.process.stdin.buffer.write(*args, **kwargs) Line 37 just above may require the same treatment, but also octoprint could have new/different expectations... it'd be pretty lucky if that was all it took.

o-nix commented 3 years ago

@lav8org thanks, was digging in the same direction, my update is more lame straightforward

diff --git a/octoprint-plugin/octoprint_stepd/StepdService.py b/octoprint-plugin/octoprint_stepd/StepdService.py
index ea3a1d0..7e28c22 100644
--- a/octoprint-plugin/octoprint_stepd/StepdService.py
+++ b/octoprint-plugin/octoprint_stepd/StepdService.py
@@ -17,7 +17,7 @@ class StepdService():
     self.bin_path = os.path.join(self.base_path, 'step-daemon')

     args = [self.bin_path, 'device='+str(self.device),
-            'baud='+str(self.baudrate), 'config=config.json']
+            'baud='+str(self.baudrate), 'config=' + os.path.join(self.base_path, 'config.json')]

     logger.info("Starting service: " + str(args))

@@ -34,10 +34,16 @@ class StepdService():
   ##~~ mock Serial methods

   def readline(self, *args, **kwargs):
-    return self.process.stdout.readline(*args, **kwargs)
+    line = self.process.stdout.readline(*args, **kwargs)
+    return line.encode('ascii')

   def write(self, *args, **kwargs):
-    return self.process.stdin.write(*args, **kwargs)
+    text_args = []
+
+    for arg in args:
+        text_args.append(arg.decode('ascii'))
+
+    return self.process.stdin.write(*text_args, **kwargs)

   def close(self):
     self.process.kill()

It allows connecting, but the binary still drops the connection after a couple of seconds with panic: Failed to load device settings error...

lav8org commented 3 years ago

My impression is that the master branch is a rewrite that is as yet incomplete, but was uploaded so that others could help? Could be wrong... can't help but wonder because it certainly wouldn't have taken much effort to drop any sort of warning at the top of the readme, considering that then people might actually try to help instead of burning time assuming it should work. Also because I missed the part about the compatible Marlin branch being out of sync with the master (tho once noticed I checked the diff; a bit fuzzy here 2 weeks later but don't recall there being differences that would obviously prevent the panic). Regardless, maybe the "legacy" branch will run? I gathered it was unreliable, but not unrunnable.

o-nix commented 3 years ago

@colinrgodsey is master an unstable rewrite? 👆

colinrgodsey commented 3 years ago

The master branch was never really finalized for release. At that stage I was looking for some devices to test with and I didn't really get anywhere.

The outstanding issues right now, judging by the recent feedback, is that the OctoPrint plugin has some compatibility issues with newer octoprint, and there are some compatibility issues with syncing the device settings from Marlin (either due to new Marlin changes, or different device configurations).

The octoprint issue should be easy to fix, and it's something I can tackle when I get more bandwidth (hopefully soon). The configuration sync issues are a bit more troublesome. One of the goals of the Marlin implementation was to add as little extra g-code as possible, and use as much of the existing functionality as we could. The m503 "report settings" code causes a lot of issues here. It ultimately needs to be re-written to just look for the few responses it needs from there, and unlock the pipeline as soon as it gets those (instead of waiting for the "end" of the output, which is apparently not stable).

There was still several changes I wanted to make here, including on the settings sync, that I wanted to tackle before I officially "released" step-d. I'm hoping I can get to those soon, but my availability has been wonky for the last few months. I need to write up some issues here for the last few items to help guide any contributions, but I'm also open to any fixes you guys come up with until I can get back into full swing here.

colinrgodsey commented 3 years ago

Sorry for the delay all. Finally got around to cleaning up some stuff in the main codebase and fixing support for octoprint on python 2/3. There's a new build of the plugin (required) that was tested on the latest OctoPrint (python 2 and 3) and latest Marlin 2.0.x.

You should be able to grab the new OctoPrint plugin here: https://raw.githubusercontent.com/colinrgodsey/maven/master/step-daemon/octoprint-plugin/latest.zip and install it over the existing plugin and restart OctoPrint.

colinrgodsey commented 3 years ago

Should be fixed now. Closing this out, feel free to reopen if it somehow is still happening.