Repository: missionpinball/mpf

Issue:
coil_player action 'off' crashes out
Using 'off' as an action in a coil_player generates (at run time) an error that the action is not valid (**_Invalid action off_**).  Easily worked-around by using 'disabled' instead, but either the code needs fixing or the documentation updating.

So this works fine when the ball_cleared_ramp event comes in..

```
# Will activate a divertor depending on which lock is our target.  
coil_player:
   route_to_lock1:
      c_lowerDivertor:
         action: pulse
         pulse_ms: 3000
   route_to_lock2:
      c_upperDivertor:
         action: pulse
         pulse_ms: 3000
   ball_cleared_ramp: # Divertor did it's job, so can be switched off
      c_upperDivertor:
         action: disable
      c_lowerDivertor:
         action: disable
```

However with this config instead..

```
# Will activate a divertor depending on which lock is our target. 
coil_player:
   route_to_lock1:
      c_lowerDivertor:
         action: pulse
         pulse_ms: 3000
   route_to_lock2:
      c_upperDivertor:
         action: pulse
         pulse_ms: 3000
   ball_cleared_ramp: # Divertor did it's job, so can be switched off
      c_upperDivertor:
         action: off
      c_lowerDivertor:
         action: off
```

This is the error from the log..

Traceback (most recent call last):
  File "c:\users\mark\appdata\local\programs\python\python36\lib\site-packages\mpf\core\events.py", line 657, in _run_handlers
    result = handler.callback(**merged_kwargs)
  File "c:\users\mark\appdata\local\programs\python\python36\lib\site-packages\mpf\core\config_player.py", line 336, in config_play_callback
    **kwargs)
  File "c:\users\mark\appdata\local\programs\python\python36\lib\site-packages\mpf\config_players\coil_player.py", line 32, in play
    self.raise_config_error("Invalid action {}".format(action), 1, context=context)
  File "c:\users\mark\appdata\local\programs\python\python36\lib\site-packages\mpf\core\logging.py", line 139, in raise_config_error
    raise ConfigFileError(msg, error_no, self.log.name, context)
mpf.exceptions.ConfigFileError.ConfigFileError: Config File Error in coil_player: Invalid action off Error Code: CFE-coil_player-1 Context: lock_handler




Discussion:

Patch:
diff --git a/mpf/config_players/coil_player.py b/mpf/config_players/coil_player.py
index 9fe7fc5d1..ee67b8552 100644
--- a/mpf/config_players/coil_player.py
+++ b/mpf/config_players/coil_player.py
@@ -27,16 +27,22 @@ class CoilPlayer(DeviceConfigPlayer):
             if not isinstance(coil, Driver):
                 self.raise_config_error("Invalid coil name {}".format(coil), 2, context=context)
             action = s.pop('action')
-            coil_action = getattr(coil, action, None)
-            if not coil_action:
-                self.raise_config_error("Invalid action {}".format(action), 1, context=context)
 
-            if action in ("disable", "off") and coil.name in instance_dict:
+            # delete coil from dict
+            try:
                 del instance_dict[coil.name]
+            except KeyError:
+                pass
+
+            if action in ("disable", "off"):
+                coil.disable(**s)
             elif action in ("on", "enable"):
                 instance_dict[coil.name] = coil
-
-            coil_action(**s)
+                coil.enable(**s)
+            elif action == "pulse":
+                coil.pulse(**s)
+            else:
+                self.raise_config_error("Invalid action {}".format(action), 1, context=context)
 
     def clear_context(self, context):
         """Disable enabled coils."""
diff --git a/mpf/config_spec.yaml b/mpf/config_spec.yaml
index da85e2565..afe7a653e 100644
--- a/mpf/config_spec.yaml
+++ b/mpf/config_spec.yaml
@@ -266,7 +266,7 @@ fast_coils:
     recycle_ms: single|ms|None
 coil_player:
     __valid_in__: machine, mode, show
-    action: single|lstr|pulse
+    action: single|enum(pulse,on,off,enable,disable)|pulse
     hold_power: single|float|None
     pulse_power: single|float|None
     pulse_ms: single|int|None


Test patch:
diff --git a/mpf/tests/machine_files/coil_player/config/coil_player.yaml b/mpf/tests/machine_files/coil_player/config/coil_player.yaml
index 0e0cbf253..b749a1335 100644
--- a/mpf/tests/machine_files/coil_player/config/coil_player.yaml
+++ b/mpf/tests/machine_files/coil_player/config/coil_player.yaml
@@ -41,6 +41,12 @@ coil_player:
     coil_3: disable
   event9:
     coil_3: 30
+  event10:
+    coil_1:
+      action: on
+  event11:
+    coil_1:
+      action: off
   pulse_1_100:
     coil_1:
       action: pulse
@@ -49,4 +55,4 @@ coil_player:
     coil_1:
       action: pulse
       pulse_ms: 50
-      max_wait_ms: 100
\ No newline at end of file
+      max_wait_ms: 100
diff --git a/mpf/tests/test_CoilPlayer.py b/mpf/tests/test_CoilPlayer.py
index a079f556d..d5f2c4707 100644
--- a/mpf/tests/test_CoilPlayer.py
+++ b/mpf/tests/test_CoilPlayer.py
@@ -127,6 +127,22 @@ class TestCoilPlayer(MpfTestCase):
 
         coil.hw_driver.disable.assert_called_with()
 
+        # same again but use on and off instead of enable and disable
+        coil.hw_driver.enable = MagicMock()
+        coil.hw_driver.pulse = MagicMock()
+        coil.hw_driver.disable = MagicMock()
+
+        self.machine.events.post('event10')
+        self.advance_time_and_run()
+
+        coil.hw_driver.enable.assert_called_with(PulseSettings(power=1.0, duration=10), HoldSettings(power=1.0))
+        assert not coil.hw_driver.pulse.called
+
+        self.machine.events.post('event11')
+        self.advance_time_and_run()
+
+        coil.hw_driver.disable.assert_called_with()
+
     def test_coil_player_in_mode(self):
         coil = self.machine.coils['coil_3']
         coil.hw_driver.enable = MagicMock()
@@ -158,4 +174,4 @@ class TestCoilPlayer(MpfTestCase):
         self.assertEqual("pulsed_100", coil.hw_driver.state)
         # after pulse end
         self.advance_time_and_run(.06)
-        self.assertEqual("pulsed_50", coil.hw_driver.state)
\ No newline at end of file
+        self.assertEqual("pulsed_50", coil.hw_driver.state)