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.

Sign up for GitHub

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

Jump to bottom

System11 AC Relay Improvements #1628

Merged
merged 5 commits into from
Jan 21, 2022
Merged

System11 AC Relay Improvements #1628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d94840e
Correct platform_driver in timed_enable event
avanwinkle Jan 8, 2022
3217db7
Explicit driver_action for Sys11 timed_enable to avoid accidental ind…
avanwinkle Dec 30, 2021
d6a6a8e
Improve AC Relay transitions and block C queue
avanwinkle Jan 8, 2022
77a9c09
Support for ac_relay_switch for System11 platforms
avanwinkle Jan 8, 2022
cf21e53
Fix Snux test to return wait_ms on mocked pulse()
avanwinkle Jan 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions mpf/config_spec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1857,10 +1857,14 @@ system11:
__valid_in__: machine
__type__: config
ac_relay_delay_ms: single|ms|75ms
ac_relay_debounce_ms: single|ms|0
ac_relay_driver: single|machine(coils)|
ac_relay_switch: single|machine(switches)|None
prefer_a_side_event: single|event_handler|game_ended
prefer_c_side_event: single|event_handler|game_will_start
queue_c_side_while_preferred: single|bool|true
platform: single|str|None
debug: single|bool|false
console_log: single|enum(none,basic,full)|none
file_log: single|enum(none,basic,full)|basic
text_strings:
Expand Down
3 changes: 2 additions & 1 deletion mpf/devices/driver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,8 @@ def timed_enable(self, timed_enable_ms: int = None, hold_power: float = None, pu
pulse_power = self.get_and_verify_pulse_power(pulse_power)
hold_duration = self.get_and_verify_timed_enable_ms(timed_enable_ms)
hold_power = self.get_and_verify_hold_power(hold_power)
wait_ms = self._notify_psu_and_get_wait_ms(pulse_duration, max_wait_ms)
# Let the PSU wait for both the pulse _and_ the timed enable
wait_ms = self._notify_psu_and_get_wait_ms(pulse_duration + hold_duration, max_wait_ms)

# TODO: Detect a NotImplementedError and simulate a timed_enable
# with a software timer and enable+disable
Expand Down
108 changes: 67 additions & 41 deletions mpf/platforms/system11.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ class System11OverlayPlatform(DriverPlatform, SwitchPlatform):

"""Overlay platform to drive system11 machines using a WPC controller."""

__slots__ = ["delay", "platform", "system11_config", "a_side_queue", "c_side_queue",
__slots__ = ["delay", "platform", "system11_config", "a_side_queue", "c_side_queue", "debounce_secs",
"a_side_done_time", "c_side_done_time", "drivers_holding_a_side", "drivers_holding_c_side",
"a_side_enabled", "c_side_enabled", "ac_relay_in_transition", "prefer_a_side", "drivers"]
"a_side_enabled", "c_side_enabled", "ac_relay_in_transition", "prefer_a_side", "drivers",
"relay_switch"]

def __init__(self, machine: MachineController) -> None:
"""Initialise the board."""
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

self.drivers_holding_a_side = set() # type: Set[DriverPlatformInterface]
self.drivers_holding_c_side = set() # type: Set[DriverPlatformInterface]
self.a_side_enabled = True
Expand All @@ -60,25 +62,26 @@ def __init__(self, machine: MachineController) -> None:
def stop(self):
"""Stop the overlay. Nothing to do here because stop is also called on parent platform."""

@property
def a_side_active(self):
"""Return if A side cannot be switched off right away."""
return self.drivers_holding_a_side or self.a_side_done_time + self.debounce_secs > self.machine.clock.get_time()

@property
def a_side_busy(self):
"""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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


@property
def c_side_active(self):
"""Return if C side cannot be switches off right away."""
return self.drivers_holding_c_side or self.c_side_done_time > self.machine.clock.get_time()
"""Return if C side cannot be switched off right away."""
return self.drivers_holding_c_side or self.c_side_done_time + self.debounce_secs > self.machine.clock.get_time()

@property
def c_side_busy(self):
"""Return if C side cannot be switches off right away."""
return self.drivers_holding_c_side or self.c_side_done_time > self.machine.clock.get_time() or self.c_side_queue
"""Return if C side cannot be switched off right away."""
return self.c_side_active or self.c_side_queue

@property
def a_side_active(self):
"""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()

async def initialize(self):
"""Automatically called by the Platform class after all the core modules are loaded."""
Expand All @@ -101,8 +104,15 @@ def _initialize(self, **kwargs):

self.system11_config['ac_relay_driver'].get_and_verify_hold_power(1.0)

self.log.debug("Configuring A/C Select Relay transition delay for "
"%sms", self.system11_config['ac_relay_delay_ms'])
if self.system11_config['ac_relay_switch']:
self.log.debug("Configuring A/C Select Relay switch %s", self.system11_config['ac_relay_switch'])
self.system11_config['ac_relay_switch'].add_handler(state=0, callback=self._a_side_enabled)
self.system11_config['ac_relay_switch'].add_handler(state=1, callback=self._c_side_enabled)

self.debounce_secs = self.system11_config['ac_relay_debounce_ms'] / 1000.0
self.log.debug("Configuring A/C Select Relay transition delay for %sms and debounce for %s",
self.system11_config['ac_relay_delay_ms'],
self.system11_config['ac_relay_debounce_ms'])

self.machine.events.add_handler(self.system11_config['prefer_a_side_event'], self._prefer_a_side)
self.log.info("Configuring System11 driver to prefer A side on event %s",
Expand Down Expand Up @@ -250,7 +260,7 @@ def clear_hw_rule(self, switch, coil):
self.platform.clear_hw_rule(switch, coil)

def driver_action(self, driver, pulse_settings: Optional[PulseSettings], hold_settings: Optional[HoldSettings],
side: str):
side: str, timed_enable: bool = False):
"""Add a driver action for a switched driver to the queue (for either the A-side or C-side queue).

Args:
Expand All @@ -264,20 +274,26 @@ def driver_action(self, driver, pulse_settings: Optional[PulseSettings], hold_se
"""
if self.prefer_a_side:
if side == "A":
self.a_side_queue.add((driver, pulse_settings, hold_settings))
self._service_a_side()
self.a_side_queue.add((driver, pulse_settings, hold_settings, timed_enable))
if not self.ac_relay_in_transition:
self._service_a_side()
elif side == "C":
self.c_side_queue.add((driver, pulse_settings, hold_settings))
self.c_side_queue.add((driver, pulse_settings, hold_settings, timed_enable))
if not self.ac_relay_in_transition and not self.a_side_busy:
self._service_c_side()
else:
raise AssertionError("Invalid side {}".format(side))
else:
if side == "C":
self.c_side_queue.add((driver, pulse_settings, hold_settings))
self._service_c_side()
# Sometimes it doesn't make sense to queue the C side (flashers) and play them after
# switching to the A side (coils) and back. In which case, just ignore this driver action.
if not self.c_side_enabled and not self.system11_config['queue_c_side_while_preferred']:
return
self.c_side_queue.add((driver, pulse_settings, hold_settings, timed_enable))
if not self.ac_relay_in_transition:
self._service_c_side()
elif side == "A":
self.a_side_queue.add((driver, pulse_settings, hold_settings))
self.a_side_queue.add((driver, pulse_settings, hold_settings, timed_enable))
if not self.ac_relay_in_transition and not self.c_side_busy:
self._service_a_side()
else:
Expand All @@ -288,18 +304,24 @@ def _enable_ac_relay(self):
self.ac_relay_in_transition = True
self.a_side_enabled = False
self.c_side_enabled = False
self.delay.add(ms=self.system11_config['ac_relay_delay_ms'],
callback=self._c_side_enabled,
name='enable_ac_relay')
# Without a relay switch, use a delay to wait for the relay to enable
if not self.system11_config['ac_relay_switch']:
self.delay.add(ms=self.system11_config['ac_relay_delay_ms'],
callback=self._c_side_enabled,
name='enable_ac_relay')

def _disable_ac_relay(self):
self.system11_config['ac_relay_driver'].disable()
self.ac_relay_in_transition = True
self.a_side_enabled = False
self.c_side_enabled = False
self.delay.add(ms=self.system11_config['ac_relay_delay_ms'],
callback=self._a_side_enabled,
name='disable_ac_relay')
# Clear out the C side queue if we don't want to hold onto it for later
if not self.system11_config['queue_c_side_while_preferred']:
self.c_side_queue.clear()
if not self.system11_config['ac_relay_switch']:
self.delay.add(ms=self.system11_config['ac_relay_delay_ms'],
callback=self._a_side_enabled,
name='disable_ac_relay')

# -------------------------------- A SIDE ---------------------------------

Expand All @@ -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'],
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

callback=self._enable_a_side,
name='enable_a_side')
return

if self.c_side_enabled:
Expand Down Expand Up @@ -354,12 +373,17 @@ def _service_a_side(self):
return

while self.a_side_queue:
driver, pulse_settings, hold_settings = self.a_side_queue.pop()
driver, pulse_settings, hold_settings, timed_enable = self.a_side_queue.pop()

if hold_settings is None and pulse_settings:
driver.pulse(pulse_settings)
self.a_side_done_time = max(self.a_side_done_time,
self.machine.clock.get_time() + (pulse_settings.duration / 1000.0))
if timed_enable:
wait_ms = driver.timed_enable(pulse_settings, hold_settings) or 0
wait_secs = (pulse_settings.duration + hold_settings.duration + wait_ms) / 1000.0
self.a_side_done_time = max(self.a_side_done_time, self.machine.clock.get_time() + wait_secs)

elif hold_settings is None and pulse_settings:
wait_ms = driver.pulse(pulse_settings) or 0
wait_secs = (pulse_settings.duration + wait_ms) / 1000.0
self.a_side_done_time = max(self.a_side_done_time, self.machine.clock.get_time() + wait_secs)

elif hold_settings and pulse_settings:
driver.enable(pulse_settings, hold_settings)
Expand All @@ -381,9 +405,6 @@ def _enable_c_side(self):
if self.a_side_active:
self._disable_all_a_side_drivers()
self._enable_ac_relay()
self.delay.add(ms=self.system11_config['ac_relay_delay_ms'],
callback=self._enable_c_side,
name='enable_c_side')
return

if self.a_side_enabled:
Expand Down Expand Up @@ -429,9 +450,14 @@ def _service_c_side(self):
return

while self.c_side_queue:
driver, pulse_settings, hold_settings = self.c_side_queue.pop()
driver, pulse_settings, hold_settings, timed_enable = self.c_side_queue.pop()

if timed_enable:
driver.timed_enable(pulse_settings, hold_settings)
self.c_side_done_time = max(self.c_side_done_time,
self.machine.clock.get_time() + (pulse_settings.duration / 1000.0))

if hold_settings is None and pulse_settings:
elif hold_settings is None and pulse_settings:
driver.pulse(pulse_settings)
self.c_side_done_time = max(self.c_side_done_time,
self.machine.clock.get_time() + (pulse_settings.duration / 1000.0))
Expand Down Expand Up @@ -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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

4 changes: 2 additions & 2 deletions mpf/tests/test_Snux.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

driver_11.enable = MagicMock()
driver_11.disable = MagicMock()
driver_12.pulse = MagicMock()
driver_12.pulse = MagicMock(return_value=0)
driver_12.enable = MagicMock()
driver_12.disable = MagicMock()

Expand Down