Fix M909 (set microstepping) and M92 (set steps per mm)

- remove Printer.steps_pr_meter because it's redundant with Stepper.microsteps - remove callback from Stepper to Printer to update steps_pr_meter - remove redundant microstepping bounds check from M909 - the Steppers can do this themselves - add microstepping bounds check to B-series Steppers to match the A-series - update steps per mm in M92 correctly (instead of restarting the path planner, which doesn't help) - add tests for M909 and M92 - revise PathPlanner::setAxisStepsPerMeter to preserve the current location of the printer Validated on the hardware - you can now change microstepping settings without losing printer positioning. If no one objects I'd like to take an IOU on test coverage for the PathPlanner bits - that interface is about to be significantly refactored anyway to make probing more flexible. Fixes #188
intelligent-agent · Aug 24, 2019 · 4b0a83f · 4b0a83f
1 parent d08cc19
commit 4b0a83f
Show file tree

Hide file tree

Showing 10 changed files with 170 additions and 34 deletions.
diff --git a/redeem/PathPlanner.py b/redeem/PathPlanner.py
@@ -137,7 +137,7 @@ def _init_path_planner(self):
 
     self.native_planner.initPRU(fw0, fw1)
 
-    self.native_planner.setAxisStepsPerMeter(tuple(self.printer.steps_pr_meter))
+    self.native_planner.setAxisStepsPerMeter(tuple(self.printer.get_steps_pr_meter()))
     self.native_planner.setMaxSpeeds(tuple(self.printer.max_speeds))
     self.native_planner.setAcceleration(tuple(self.printer.acceleration))
     self.native_planner.setMaxSpeedJumps(tuple(self.printer.max_speed_jumps))
@@ -174,7 +174,7 @@ def restart(self):
 
   def update_steps_pr_meter(self):
     """ Update steps pr meter from the path """
-    self.native_planner.setAxisStepsPerMeter(tuple(self.printer.steps_pr_meter))
+    self.native_planner.setAxisStepsPerMeter(tuple(self.printer.get_steps_pr_meter()))
 
   def update_backlash(self):
     """ Update steps pr meter from the path """
@@ -420,8 +420,8 @@ def probe(self, z, speed, accel):
     start_state = self.native_planner.getState()
 
     # calculate how many steps the requested z movement will require
-    steps = np.ceil(z * self.printer.steps_pr_meter[2])
-    z_dist = steps / self.printer.steps_pr_meter[2]
+    steps = np.ceil(z * self.printer.get_steps_pr_meter()[2])
+    z_dist = steps / self.printer.get_steps_pr_meter()[2]
     logging.debug("Steps total: " + str(steps))
 
     # select the relative end point
@@ -540,10 +540,6 @@ def set_extruder(self, ext_nr):
         """
     if ext_nr in range(Printer.MAX_AXES - 3):
       logging.debug("Selecting " + str(ext_nr))
-      #Printer.steps_pr_meter[3] = self.printer.steppers[
-      #        Printer.index_to_axis(ext_nr+3)
-      #        ].get_steps_pr_meter()
-      #self.native_planner.setExtruder(ext_nr)
 
 
 """

diff --git a/redeem/Printer.py b/redeem/Printer.py
@@ -87,7 +87,6 @@ def __init__(self):
     self.home_speed = np.ones(self.num_axes)
     self.home_backoff_speed = np.ones(self.num_axes)
     self.home_backoff_offset = np.zeros(self.num_axes)
-    self.steps_pr_meter = np.ones(self.num_axes)
     self.backlash_compensation = np.zeros(self.num_axes)
     self.backlash_state = np.zeros(self.num_axes)
     self.soft_min = -np.ones(self.num_axes) * 1000.0
@@ -175,6 +174,12 @@ def set_active_endstops(self):
     # write to shared memory
     PruInterface.set_active_endstops(active)
 
+  def get_steps_pr_meter(self):
+    result = np.ones(self.num_axes)
+    for axis in self.steppers.keys():
+      result[self.axis_to_index(axis)] = self.steppers[axis].get_steps_pr_meter()
+    return result
+
   def save_settings(self, filename):
     logging.debug("save_settings: setting stepper parameters")
     for name, stepper in iteritems(self.steppers):

diff --git a/redeem/Redeem.py b/redeem/Redeem.py
@@ -444,7 +444,6 @@ def __init__(self, config_location="/etc/redeem"):
                                                               'home_backoff_speed_' + axis.lower())
       printer.home_backoff_offset[i] = printer.config.getfloat(
           'Homing', 'home_backoff_offset_' + axis.lower())
-      printer.steps_pr_meter[i] = printer.steppers[axis].get_steps_pr_meter()
       printer.backlash_compensation[i] = printer.config.getfloat('Steppers',
                                                                  'backlash_' + axis.lower())
 

diff --git a/redeem/Stepper.py b/redeem/Stepper.py
@@ -36,8 +36,6 @@ class Stepper(object):
 
   printer = None
 
-  all_steppers = list()
-
   def __init__(self, step_pin, dir_pin, fault_key, dac_channel, shiftreg_nr, name):
     """ Init """
     self.dac_channel = dac_channel    # Which channel on the dac is connected to this stepper
@@ -138,7 +136,11 @@ def __init__(self, stepPin, dirPin, faultPin, dac_channel, shiftreg_nr, name):
     self.state = 0    # The initial state of shift register
 
   def set_microstepping(self, value, force_update=False):
-    """ Todo: Find an elegant way for this """
+    if not value in range(9):
+      logging.warning("Tried to set illegal microstepping value: {0} for stepper {1}".format(
+          value, self.name))
+      return
+
     EN_CFG1 = (1 << 7)
     DIS_CFG1 = (0 << 7)
     EN_CFG2 = (1 << 5)
@@ -179,9 +181,6 @@ def set_microstepping(self, value, force_update=False):
     self.shift_reg.set_state(state, 0xF0)
     self.mmPrStep = 1.0 / (self.steps_pr_mm * self.microsteps)
 
-    # update the Printer class with new values
-    stepper_num = self.printer.axis_to_index(self.name)
-    self.printer.steps_pr_meter[stepper_num] = self.get_steps_pr_meter()
     logging.debug("Updated stepper " + self.name + " to microstepping " + str(value) + " = " +
                   str(self.microsteps))
     self.microstepping = value
@@ -432,9 +431,6 @@ def set_microstepping(self, value, force_update=False):
     #self.state = int("0b"+bin(self.state)[2:].rjust(8, '0')[:4]+bin(value)[2:].rjust(3, '0')+bin(self.state)[-1:], 2)
     self.mmPrStep = 1.0 / (self.steps_pr_mm * self.microsteps)
 
-    # update the Printer class with new values
-    stepper_num = self.printer.axis_to_index(self.name)
-    self.printer.steps_pr_meter[stepper_num] = self.get_steps_pr_meter()
     logging.debug("Updated stepper " + self.name + " to microstepping " + str(value) + " = " +
                   str(self.microsteps))
     self.update()

diff --git a/redeem/gcodes/M909.py b/redeem/gcodes/M909.py
@@ -19,21 +19,20 @@ def execute(self, g):
 
     for axis in self.printer.AXES:
       if g.has_letter(axis) and g.has_letter_value(axis):
-        val = g.get_int_by_letter(axis)
-        if val >= 0 and val <= 7:
+        val = g.get_int_by_letter(axis, -1)
+        if val != -1:
           self.printer.steppers[axis].set_microstepping(val)
     self.printer.path_planner.update_steps_pr_meter()
-    logging.debug("Updated steps pr meter to %s", self.printer.steps_pr_meter)
+    logging.debug("Updated steps pr meter to %s", self.printer.get_steps_pr_meter())
 
   def get_description(self):
     return "Set microstepping value"
 
   def get_long_description(self):
-    return ("Example: M909 X3 Y5 Z2 E3\n"
-            "Set the microstepping value for each of the steppers. In "
-            "Redeem this is implemented as 2^value, so M909 X2 sets "
-            " microstepping to 2^2 = 4, M909 Y3 sets microstepping to "
-            "2^3 = 8 etc. ")
+    return (
+        "Example: M909 X3 Y5 Z2 E3\n"
+        "Set the microstepping value for each of the steppers. What these values mean depends on your board revision. "
+    )
 
   def is_buffered(self):
     return True
diff --git a/redeem/gcodes/M92.py b/redeem/gcodes/M92.py
@@ -21,11 +21,9 @@ def execute(self, g):
       if value > 0:
         logging.info("Updating steps pr mm on {} to {}".format(axis, value))
         self.printer.steppers[axis].set_steps_pr_mm(value)
-        i = Printer.axis_to_index(axis)
-        self.printer.steps_pr_meter[i] = self.printer.steppers[axis].get_steps_pr_meter()
       else:
-        logging.error('Steps per milimeter must be grater than zero.')
-    self.printer.path_planner.restart()
+        logging.error('Steps per millimeter must be greater than zero.')
+    self.printer.path_planner.update_steps_pr_meter()
 
   def get_description(self):
     return "Set number of steps per millimeters for each steppers"
diff --git a/redeem/path_planner/PathPlannerSetup.cpp b/redeem/path_planner/PathPlannerSetup.cpp
@@ -44,11 +44,12 @@ void PathPlanner::setMaxSpeedJumps(VectorN speedJumps)
     optimizer.setMaxSpeedJumps(speedJumps);
 }
 
-void PathPlanner::setAxisStepsPerMeter(VectorN stepPerM)
+void PathPlanner::setAxisStepsPerMeter(VectorN stepsPerM)
 {
-    axisStepsPerM = stepPerM;
-
+    VectorN stateBefore = getState();
+    axisStepsPerM = stepsPerM;
     recomputeParameters();
+    setState(stateBefore);
 }
 
 // soft endstops

diff --git a/tests/gcode/MockPrinter.py b/tests/gcode/MockPrinter.py
@@ -57,6 +57,12 @@ class MockPrinter(unittest.TestCase):
   above, creates a mock Redeem instance. The mock instance has only what is
   needed for our tests and does not access any BBB hardware IOs.
   """
+  """
+  handy conversion from microstep config to microstep multiplier
+  this is inherently tied to the stepper class the MockPrinter is using - right now it's 00A4
+  for the B series that use TMC2100s, this is [1, 2, 2, 4, 16, 4, 16, 4, 16]
+  """
+  microstep_config_to_multiplier = [1, 2, 4, 8, 16, 32]
 
   @classmethod
   def setUpPatch(cls):

diff --git a/tests/gcode/test_M909.py b/tests/gcode/test_M909.py
@@ -0,0 +1,75 @@
+from __future__ import absolute_import
+
+from .MockPrinter import MockPrinter
+from numpy.testing import assert_array_equal
+import numpy
+import mock
+
+
+class M909_Tests(MockPrinter):
+  def setUp(self):
+    self.steps_pr_mm = {}
+    self.microstep_configs = {}
+
+    for axis, stepper in self.printer.steppers.items():
+      self.steps_pr_mm[axis] = stepper.steps_pr_mm
+      self.microstep_configs[axis] = stepper.microstepping
+
+    self.printer.path_planner.update_steps_pr_meter = mock.Mock()
+
+  def _check_microsteps_pr_meter(self):
+    # first we check just the microstep configs because this makes failures easier to diagnose
+    printer_microstep_configs = numpy.ones(self.printer.num_axes)
+    expected_microstep_configs = numpy.ones(self.printer.num_axes)
+    for axis, stepper in self.printer.steppers.items():
+      index = self.printer.axis_to_index(axis)
+      printer_microstep_configs[index] = stepper.microstepping
+      expected_microstep_configs[index] = self.microstep_configs[axis]
+
+    assert_array_equal(expected_microstep_configs, printer_microstep_configs)
+
+    # next check the final multiplied microsteps per mm
+    printer_microsteps = [val / 1000.0 for val in self.printer.get_steps_pr_meter()]
+    expected_microsteps = []
+
+    for axis_num in range(self.printer.num_axes):
+      axis = self.printer.index_to_axis(axis_num)
+      expected_microsteps.append(self.steps_pr_mm[axis] *
+                                 self.microstep_config_to_multiplier[self.microstep_configs[axis]])
+
+    assert_array_equal(expected_microsteps, printer_microsteps)
+
+  def test_gcodes_M909_noop(self):
+    self.printer.path_planner.update_steps_pr_meter.assert_not_called()
+    self._check_microsteps_pr_meter()
+    self.execute_gcode("M909")
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()
+
+  def test_gcodes_M909_X0_Y1_E2(self):
+    self.execute_gcode("M909 X0 Y1 E2")
+    self.microstep_configs['X'] = 0
+    self.microstep_configs['Y'] = 1
+    self.microstep_configs['E'] = 2
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()
+
+  def test_gcodes_M909_H5_E4_Z2(self):
+    self.execute_gcode("M909 H5 E4 Z2")
+    self.microstep_configs['H'] = 5
+    self.microstep_configs['E'] = 4
+    self.microstep_configs['Z'] = 2
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()
+
+  def test_gcodes_M909_X20(self):
+    self.execute_gcode("M909 X20")
+    # should be a no-op
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()
+
+  def test_gcodes_M909_Xneg(self):
+    self.execute_gcode("M909 X-1")
+    # should be a no-op
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()
diff --git a/tests/gcode/test_M92.py b/tests/gcode/test_M92.py
@@ -0,0 +1,61 @@
+from __future__ import absolute_import
+
+from .MockPrinter import MockPrinter
+from numpy.testing import assert_array_equal
+import mock
+
+
+class M92_Tests(MockPrinter):
+  def setUp(self):
+    self.steps_pr_mm = {}
+
+    for axis, stepper in self.printer.steppers.items():
+      self.steps_pr_mm[axis] = stepper.steps_pr_mm
+      stepper.set_microstepping(0)
+
+    self.printer.path_planner.update_steps_pr_meter = mock.Mock()
+
+  def _check_microsteps_pr_meter(self):
+    # to make the test output easier to read, we actually convert from meters to millimeters
+    printer_microsteps = [val / 1000.0 for val in self.printer.get_steps_pr_meter()]
+    expected_microsteps = [
+        self.steps_pr_mm[self.printer.index_to_axis(axis_num)]
+        for axis_num in range(self.printer.num_axes)
+    ]
+
+    assert_array_equal(expected_microsteps, printer_microsteps)
+
+  def test_gcodes_M92_noop(self):
+    self.printer.path_planner.update_steps_pr_meter.assert_not_called()
+    self._check_microsteps_pr_meter()
+    self.execute_gcode("M92")
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()
+
+  def test_gcodes_M92_X5_Z200_H50000(self):
+    self.execute_gcode("M92 X5 Z200.0 H50000.0000")
+    self.steps_pr_mm['X'] = 5.0
+    self.steps_pr_mm['Z'] = 200.0
+    self.steps_pr_mm['H'] = 50000.0
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()
+
+  def test_gcodes_M92_E0_001_H0_0050(self):
+    self.execute_gcode("M92 E0.001 H0.0050")
+    self.steps_pr_mm['E'] = 0.001
+    self.steps_pr_mm['H'] = 0.0050
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()
+
+  def test_gcodes_M92_Yneg_Zneg(self):
+    self.execute_gcode("M92 Y-0.05 Z-2")
+    # should be a no-op
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()
+
+  def test_gcodes_M92_Xneg_Y8_44(self):
+    self.execute_gcode("M92 X-22 Y8.44")
+    # negative X is ignored
+    self.steps_pr_mm['Y'] = 8.44
+    self._check_microsteps_pr_meter()
+    self.printer.path_planner.update_steps_pr_meter.assert_called_once()