System11 AC Relay Improvements #1628

avanwinkle · 2022-01-08T20:16:58Z

This PR adds some improvements and fixes some logical gaps in the System11 AC Relay behavior.

New Features

New config option `ac_relay_debounce_ms`

The AC Relay configuration already has an option ac_relay_delay_ms, which is the desired delay between changing the relay state and pulsing the coils. However there is no option for the inverse: a desired delay after the coils are pulsed before the relay changes back.

The new ac_relay_debounce_ms config option is a time duration that is added to the calculated coil completion time, which provides a safety buffer to ensure that the relay is not switched over while a coil still has power.

New config option `ac_relay_switch`

Later System11 machines include a switch that reflects the real-time state of the AC relay. Using this switch provides a much more accurate assessment of when it is safe to fire relayed coils. The existing method, using ac_relay_delay_ms, is vulnerable to queue and serial bus delays because the delay is calculated from when the relay coil is scheduled, which may be some time before the actual command is sent.

With a long platform queue, such as during a ball start, it's even possible for the AC relay switch over, coil fire, and switch back events to be sent to the platform within 5-10ms of each other if the queue is longer than the ac_relay_delay_ms.

This new config option updates the System11 platform to use a switch instead of a delay, if the ac_relay_switch value is provided. Instead of a delay timeout, the platform will listen to switch state changes and trigger the appropriate A/C side behavior.

New config option `queue_c_side_while_preferred`

Most of the bugs and logic improvements (listed below) are caused by competing coil events on both the A and C sides. Even with the below fixes, certain game designs may still drive excessive relay back-and-forth if the preferred C side (flashers) is trying to fire repeatedly while the A side (coils) has a series of coils to fire.

Example: a spinner has a corresponding flasher on the C side that pulses with each spinner hit.

The player hits the spinner on the way to a drain with ball save, wherein the outhole coil needs to fire to the trough and the trough coil needs to fire to the plunger.

The relay switches to the A side to fire the outhole, while the inertia from the spinner stacks up a bunch of flashes on the C side queue.

The outhole fires and the relay switches back to the C side, and the A side is then queued for the trough eject.

The flasher flashes repeatedly (because it has many hits stacked)

The relay switches back to the A side to fire the trough, while the spinner stacks up a few more flashes on the C side queue

The trough fires and the relay switches back to the C side

The flasher flashes repeatedly to clear the queue, even though the spinner has stopped.

This new configuration option queue_c_side_while_preferred is for games where the designer does not care about queueing C side events during gameplay. Because the C side is typically used for flashers and is typically "preferred" during gameplay, this option ignores all C-side triggers while the A side is active. The result is immediate firing of A side coils—even multiple in a row—and no lagging flashes after.

Logic Improvements and Bug Fixes

Prioritize switched side queue over preferred

A bug exists in the System11 logic where during relay switches from the preferred side to the other side, any triggers sent to the preferred side will immediately be serviced, causing a switch back to the preferred side and never getting around to firing what the relay originally switched over to fire.

This PR adds a safety check to check for the relay being in transition before attempting to service a trigger, therefore allowing the switchover to complete and the non-preferred side to fire before restoring the preferred side.

Include hold/timed_enable duration in coil timing calculation

A bug exists where the time calculation for how long to hold the relay on a non-preferred side are based exclusively on the pulse time, which can lead to the relay switching over while a coil is still powered for a hold or timed_enable.

This PR updates the time calculation to include both pulse_settings.duration and hold_settings.duration, to help prevent relay switches while switched coils are hot.

Explicit `timed_enable` argument for pulse actions

A bug exists with the recent timed_enable implementation based on how System11 platforms determine their behavior. A simple boolean check for hold_settings triggered either pulse() (if no hold settings) or enable() (if yes hold settings). This bug could lead to unintended enabling of coils intended for a timed enable.

This PR updates adds an explicit timed_enable argument that takes priority over the true/false presence of hold_settings, allowing a timed enable to be performed instead of a hold.

…efinite enable

sonarqubecloud · 2022-01-08T20:18:02Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

avanwinkle · 2022-01-08T20:18:53Z

mpf/platforms/system11.py

@@ -44,6 +45,7 @@ def __init__(self, machine: MachineController) -> None:

        self.a_side_done_time = 0
        self.c_side_done_time = 0
+        self.debounce_secs = 0


A number of other values are always pulled by lookup (e.g. self.system11_config['ac_relay_delay_ms']), but the debounce time is used every tick during a switchover and I thought it'd be nice to spare all those lookups with a stored value.

avanwinkle · 2022-01-08T20:19:50Z

mpf/platforms/system11.py

-        """Return if A side cannot be switches off right away."""
-        return self.drivers_holding_a_side or self.a_side_done_time > self.machine.clock.get_time() or self.a_side_queue
+        """Return if A side cannot be switched off right away."""
+        return self.a_side_active or self.a_side_queue


This block (and the C side) originally duplicated the check in both a_side_active and a_side_busy, so I cleaned the latter up to just check the former.

avanwinkle · 2022-01-08T20:27:25Z

mpf/platforms/system11.py

@@ -310,9 +332,6 @@ def _enable_a_side(self):
                if self.c_side_active:
                    self._disable_all_c_side_drivers()
                    self._disable_ac_relay()
-                    self.delay.add(ms=self.system11_config['ac_relay_delay_ms'],


This delay call seems redundant to me, because self._disable_ac_relay() also creates a delay for ac_relay_delay_ms that uses self._a_side_enabled as a callback.

This block here uses the same delay to call self._enable_a_side, which would also call self._a_side_enabled. I've noticed no issues removing the redundant call, but please let me know if I'm overlooking something.

avanwinkle · 2022-01-08T20:28:10Z

mpf/platforms/system11.py

@@ -510,4 +536,4 @@ def disable(self):

    def timed_enable(self, pulse_settings: PulseSettings, hold_settings: HoldSettings):
        """Pulse and enable the coil for an explicit duration."""
-        self.overlay.driver_action(self.platform.driver, pulse_settings, hold_settings, self.side)
+        self.overlay.driver_action(self.platform_driver, pulse_settings, hold_settings, self.side, True)


This self.platform.driver instead of self.platform_driver is a mistake on my part from the last PR!

avanwinkle · 2022-01-08T20:28:40Z

mpf/tests/test_Snux.py

@@ -52,10 +52,10 @@ def test_ac_switch_and_pulse(self):
        c_ac_relay.enable = MagicMock()
        c_ac_relay.disable = MagicMock()

-        driver_11.pulse = MagicMock()
+        driver_11.pulse = MagicMock(return_value=0)


Now that the code path uses the wait_ms response from pulse() to calculate switchover delays, need to ensure that a value is returned.

avanwinkle added 5 commits January 8, 2022 11:26

Correct platform_driver in timed_enable event

d94840e

Explicit driver_action for Sys11 timed_enable to avoid accidental ind…

3217db7

…efinite enable

Improve AC Relay transitions and block C queue

d6a6a8e

Support for ac_relay_switch for System11 platforms

77a9c09

Fix Snux test to return wait_ms on mocked pulse()

cf21e53

avanwinkle requested review from jabdoa2 and toomanybrians January 8, 2022 20:17

avanwinkle commented Jan 8, 2022

View reviewed changes

avanwinkle merged commit 1035c3f into missionpinball:dev Jan 21, 2022

avanwinkle deleted the system11-ac-relay-improvements branch December 4, 2023 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

System11 AC Relay Improvements #1628

System11 AC Relay Improvements #1628

avanwinkle commented Jan 8, 2022 •

edited

Loading

sonarqubecloud bot commented Jan 8, 2022

avanwinkle Jan 8, 2022

avanwinkle Jan 8, 2022

avanwinkle Jan 8, 2022

avanwinkle Jan 8, 2022

avanwinkle Jan 8, 2022

System11 AC Relay Improvements #1628

System11 AC Relay Improvements #1628

Conversation

avanwinkle commented Jan 8, 2022 • edited Loading

New Features

New config option ac_relay_debounce_ms

New config option ac_relay_switch

New config option queue_c_side_while_preferred

Logic Improvements and Bug Fixes

Prioritize switched side queue over preferred

Include hold/timed_enable duration in coil timing calculation

Explicit timed_enable argument for pulse actions

sonarqubecloud bot commented Jan 8, 2022

avanwinkle Jan 8, 2022

Choose a reason for hiding this comment

avanwinkle Jan 8, 2022

Choose a reason for hiding this comment

avanwinkle Jan 8, 2022

Choose a reason for hiding this comment

avanwinkle Jan 8, 2022

Choose a reason for hiding this comment

avanwinkle Jan 8, 2022

Choose a reason for hiding this comment

avanwinkle commented Jan 8, 2022 •

edited

Loading

New config option `ac_relay_debounce_ms`

New config option `ac_relay_switch`

New config option `queue_c_side_while_preferred`

Explicit `timed_enable` argument for pulse actions