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.

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

Add abort api #194

Merged
merged 7 commits into from
Oct 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions evolver/app/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,16 @@ async def calibrate(name: str, data: dict = None):
return calibrator.run_calibration_procedure(data) # TODO: or **data?


@app.post("/abort")
async def abort():
app.state.evolver.abort()
# Disable commit also in persistent config in case application needs to restart
config = Evolver.Config.load(app_settings.CONFIG_FILE)
config.enable_control = False
Copy link
Member

Choose a reason for hiding this comment

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

FYI I've opened #199 for us to also consider aborting the read.

config.enable_commit = False
config.save(app_settings.CONFIG_FILE)
Copy link
Member

@jamienoss jamienoss Oct 22, 2024

Choose a reason for hiding this comment

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

Does this mean that whilst the user can hit this endpoint to stop the control, they have to use the update endpoint to start/restart it and have to specifically do so by (hopefully) only manually changing these two flags to True every time?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes!

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is a sufficient user interface and have opened #198 to add a "start" endpoint.



app.mount("/html", html_app)


Expand Down
8 changes: 8 additions & 0 deletions evolver/app/tests/test_app.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,14 @@ def test_history(self, app_client, query_params):
else:
assert response.json() == {"data": {}}

def test_abort_endpoint(self, app_client):
assert app.state.evolver.enable_commit
response = app_client.post("/abort")
assert response.status_code == 200
assert not app.state.evolver.enable_commit
saved_config = Evolver.Config.load(app_settings.CONFIG_FILE)
assert not saved_config.enable_commit


def test_app_load_file(app_client):
config = Evolver.Config(
Expand Down
6 changes: 6 additions & 0 deletions evolver/device.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ def loop_once(self):
if self.enable_commit:
self.commit_proposals()

def abort(self):
self.enable_control = False
self.enable_commit = False
for device in self.effectors.values():
device.off()

def __enter__(self):
return self

Expand Down
6 changes: 6 additions & 0 deletions evolver/hardware/demo.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,5 +31,11 @@ def get(self):


class NoOpEffectorDriver(EffectorDriver):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)

def commit(self):
self.committed = copy(self.proposal)

def off(self):
pass
10 changes: 10 additions & 0 deletions evolver/hardware/interface.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,3 +81,13 @@ def set(self, input: Input):
@abstractmethod
def commit(self):
pass

@abstractmethod
def off(self):
"""Immediately turn device into off state.

Used by framework in aborting an experiment. Implementations should
define off condition and implement in such a way that a commit call is
not necessary (i.e. the device turns off upon calling this method).
"""
pass
5 changes: 5 additions & 0 deletions evolver/hardware/standard/led.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,8 @@ def from_brightness(brightness):
with self.serial as comm:
comm.communicate(SerialData(addr=self.addr, data=cmd))
self.committed = inputs

def off(self):
cmd = [b"0"] * self.slots
with self.serial as comm:
comm.communicate(SerialData(addr=self.addr, data=cmd))
Comment on lines +39 to +41
Copy link
Member

@jamienoss jamienoss Oct 22, 2024

Choose a reason for hiding this comment

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

This looks like more than just abort code, as mentioned before, couldn't this go in another abstractmethod named, e.g., "reset", "idle", "off", or "init", etc, that then abort just calls? abort wouldn't have to be abstract.

Looks to me like these should be called upon initialization as a way for controlling a baseline on the hardware's physical state.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the above is what it takes to abort the operation of the device. Do you mean like naming it off since that would look less alarming if called upon initialization (if we do so in the future)?

Copy link
Member

@jamienoss jamienoss Oct 22, 2024

Choose a reason for hiding this comment

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

Not entirely, but kinda yeah, I meant that we both mentioned in #197 it might be desirable to exec these commands upon initial startup, in which case it's not really an "abort" but more of a ground/rest/init/off state.

Technically it's not aborting anything as we have to actually send a commit to command the hardware to some state. Evolver.abort is an "abort" but HardwareDriver.abort isn't.

Either way, I didn't mean anything complicated just something like the following:

class HardwareDriver(...):
  @abstractmethod
  def init(self, *args, **kwargs):  # could be called "reset"
     """ Implement to command hardware to its ground/off state. This could be called upon startup/abort."""
    ...
   
  def abort(self):
    self.init()

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually the device abort does send the command directly, no commit required. I'm not sure yet what we will do at startup, but I can definitely rename the device method to off instead. "abort" speaks to the immediacy of it though...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've renamed driver method to off and added docstring for method expectations. I will defer doing anything with startup in this PR.

Copy link
Member

Choose a reason for hiding this comment

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

Actually the device abort does send the command directly, no commit required.

I only meant that it still has to send a command period and not just block subsequent commands. That these hardware methods don't alter the config, they themselves don't "block" future commanding at all etc, and that they are in fact completely oblivious to this concept (at least they should be). They only alter the hardware to some specific state, a state in this instance that is encoded in the semantics of this function name - which isn't "abort".

"abort" speaks to the immediacy of it though...

Agreed, and evolver.device.Evolver.abort is cool to be named such as it does actually "abort" subsequent control etc, though there's still scope for argument around the fact that it still pings the hardware and reads data so doesn't completely "abort", see #199 #196.

"abort" speaks to the immediacy of it though...

At the same time, however, this isn't entirely true due to the significant (1-2s) latency per call and the fact that these are turned off in sequence rather than proposals being set and committed all at once.

11 changes: 11 additions & 0 deletions evolver/hardware/standard/pump.py
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,14 @@ def commit(self):
comm.communicate(SerialData(addr=self.addr, data=cmd))
self.committed = inputs

def off(self):
cmd = [b"0"] * self.slots
for pump in self.ipp_pumps:
for solenoid in range(3):
cmd[pump * 3 + solenoid] = f"0|{pump}|{solenoid+1}".encode()
with self.serial as comm:
comm.communicate(SerialData(addr=self.addr, data=cmd))


class VialIEPumpCalibrator(GenericPumpCalibrator):
def run_calibration_procedure(self, *args, **kwargs):
Expand Down Expand Up @@ -155,3 +163,6 @@ def commit(self):
)
self._generic_pump.commit()
self.committed = copy(self.proposal)

def off(self):
self._generic_pump.off()
5 changes: 5 additions & 0 deletions evolver/hardware/standard/stir.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,8 @@ def commit(self):
with self.serial as comm:
comm.communicate(SerialData(addr=self.addr, data=cmd))
self.committed = inputs

def off(self):
cmd = [b"0"] * self.slots
with self.serial as comm:
comm.communicate(SerialData(addr=self.addr, data=cmd))
5 changes: 5 additions & 0 deletions evolver/hardware/standard/temperature.py
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,8 @@ def read(self):

def commit(self):
self._do_serial(from_proposal=True)

def off(self):
cmd = [self.HEAT_OFF] * self.slots
with self.serial as comm:
comm.communicate(SerialData(addr=self.addr, data=cmd))
4 changes: 4 additions & 0 deletions evolver/hardware/standard/tests/test_standard.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ def test_default_calibrator(self):
)
class TestTempEffectorMode(SerialVialEffectorHardwareTestSuite):
driver = Temperature
abort_command = ({"addr": "temp", "slots": 2}, b"tempr,4095,4095,_!")


@pytest.mark.parametrize(
Expand All @@ -107,6 +108,7 @@ class TestTempEffectorMode(SerialVialEffectorHardwareTestSuite):
)
class TestLED(SerialVialEffectorHardwareTestSuite):
driver = LED
abort_command = ({"addr": "od_led", "slots": 2}, b"od_ledr,0,0,_!")


@pytest.mark.parametrize(
Expand All @@ -126,6 +128,7 @@ class TestLED(SerialVialEffectorHardwareTestSuite):
)
class TestStir(SerialVialEffectorHardwareTestSuite):
driver = Stir
abort_command = ({"addr": "stir", "slots": 2}, b"stirr,0,0,_!")


@pytest.mark.parametrize(
Expand Down Expand Up @@ -155,3 +158,4 @@ class TestStir(SerialVialEffectorHardwareTestSuite):
)
class TestPump(SerialVialEffectorHardwareTestSuite):
driver = VialIEPump
abort_command = ({"addr": "pump", "slots": 2, "ipp_pumps": [1]}, b"pumpr,0,0,0,0|1|1,0|1|2,0|1|3,_!")
9 changes: 9 additions & 0 deletions evolver/hardware/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class TestMyEffector(SerialVialEffectorHardwareTestSuite):
"""

driver: HardwareDriver = None
abort_command = None

def _load_and_check(self, hw, values, serial_out):
expected_committed = {}
Expand All @@ -118,3 +119,11 @@ def test_from_config_descriptor(self, config_params, values, serial_out):
def test_from_evolver_config(self, config_params, values, serial_out):
evolver = _from_evolver_conf(self.driver, config_params, {})
self._load_and_check(evolver.hardware["testhw"], values, serial_out)

def test_abort(self, config_params, values, serial_out):
if self.abort_command == "none_expected":
return
config, command = self.abort_command
hw = _from_config_desc(self.driver, config, {})
hw.off()
assert hw.evolver.serial.backend.hits_map[command] == 1
18 changes: 17 additions & 1 deletion evolver/tests/test_device.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from evolver.connection.interface import Connection
from evolver.controller.interface import Controller
from evolver.device import DEFAULT_HISTORY, DEFAULT_SERIAL, Evolver
from evolver.hardware.demo import NoOpSensorDriver
from evolver.hardware.demo import NoOpEffectorDriver, NoOpSensorDriver
from evolver.hardware.interface import HardwareDriver
from evolver.history.interface import History

Expand Down Expand Up @@ -144,3 +144,19 @@ def test_calibration_status(self, demo_evolver):
assert isinstance(status["testsensor"].input_transformer, Status)
assert isinstance(status["testsensor"].output_transformer, Status)
assert status["testsensor"].ok

def test_abort(self, demo_evolver):
class AbortEffector(NoOpEffectorDriver):
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
self.aborted = False

def off(self):
super().off()
self.aborted = True

demo_evolver.hardware["testeffector"] = AbortEffector()
demo_evolver.enable_commit = True
demo_evolver.abort()
assert not demo_evolver.enable_commit
assert demo_evolver.hardware["testeffector"].aborted
Loading