From d890052a89a9fd311196b2b247ea4de82e1dca47 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sat, 28 Dec 2024 08:51:58 -0800
Subject: [PATCH 01/12] Update modbus variables

---
 python/lvmecp/etc/lvmecp.yml | 197 ++++++++++++++++++++++++++++-------
 1 file changed, 160 insertions(+), 37 deletions(-)

diff --git a/python/lvmecp/etc/lvmecp.yml b/python/lvmecp/etc/lvmecp.yml
index aa1d34f..ac3ac40 100644
--- a/python/lvmecp/etc/lvmecp.yml
+++ b/python/lvmecp/etc/lvmecp.yml
@@ -1,172 +1,263 @@
 modbus:
   host: 10.8.38.51
   port: 502
-  cache_timeout: 0.5
+  cache_timeout: 1
   registers:
     door_locked:
       address: 0
       group: safety
+      mode: coil
+      readonly: true
     door_closed:
       address: 1
       group: safety
+      mode: coil
+      readonly: true
     local:
       address: 2
       group: safety
+      mode: coil
+      readonly: true
     e_status:
-      address: 200
+      address: 199
       group: safety
+      mode: coil
+      readonly: true
     e_stop:
-      address: 199
+      address: 200
       group: safety
+      mode: coil
+      readonly: false
     e_relay_reset:
       address: 201
       group: safety
+      mode: coil
+      readonly: false
     cr_new:
       address: 234
       group: lights
+      mode: coil
+      readonly: false
     cr_status:
       address: 334
       group: lights
+      mode: coil
+      readonly: true
     ur_new:
       address: 235
       group: lights
+      mode: coil
+      readonly: false
     ur_status:
       address: 335
       group: lights
+      mode: coil
+      readonly: true
     sr_new:
       address: 236
       group: lights
+      mode: coil
+      readonly: false
     sr_status:
       address: 336
       group: lights
+      mode: coil
+      readonly: true
     uma_new:
       address: 237
       group: lights
+      mode: coil
+      readonly: false
     uma_status:
       address: 337
       group: lights
+      mode: coil
+      readonly: true
     tb_new:
       address: 238
       group: lights
+      mode: coil
+      readonly: false
     tb_status:
       address: 338
       group: lights
+      mode: coil
+      readonly: true
     tr_new:
       address: 239
       group: lights
+      mode: coil
+      readonly: false
     tr_status:
       address: 339
       group: lights
+      mode: coil
+      readonly: true
     drive_enabled:
       address: 99
       group: dome
-    drive_state:
-      address: 100
-      group: dome
+      mode: coil
+      readonly: false
     motor_direction:
       address: 101
       group: dome
-    drive_brake:
-      address: 102
-      group: dome
+      mode: coil
+      readonly: false
     ne_limit:
       address: 104
       group: dome
+      mode: coil
+      readonly: true
     se_limit:
       address: 105
       group: dome
+      mode: coil
+      readonly: true
     nw_limit:
       address: 106
       group: dome
+      mode: coil
+      readonly: true
     sw_limit:
       address: 107
       group: dome
+      mode: coil
+      readonly: true
     dome_closed:
       address: 108
       group: dome
+      mode: coil
+      readonly: true
     dome_open:
       address: 109
       group: dome
-    # overcurrent:
-    #   address: 109
-    #   group: dome
-    drive_velocity1:
-      address: 103
-      mode: holding_register
+      mode: coil
+      readonly: true
+    dome_lockout:
+      address: 110
       group: dome
-    drive_velocity2:
-      address: 104
-      mode: holding_register
+      mode: coil
+      readonly: true
+    dome_error:
+      address: 111
       group: dome
-    open_timeout:
-      address: 119
-      mode: holding_register
+      mode: coil
+      readonly: true
+    dome_error_reset:
+      address: 112
       group: dome
-    close_timeout:
-      address: 120
+      mode: coil
+      readonly: false
+    dome_counter:
+      address: 149
       mode: holding_register
       group: dome
-    drive_current:
-      address: 129
+      readonly: true
+    dome_position:
+      address: 150
       mode: holding_register
       group: dome
-    dome_counter:
-      address: 149
+      readonly: true
+    dome_speed:
+      address: 150
       mode: holding_register
       group: dome
+      readonly: true
     dome_status1:
       address: 410
       mode: holding_register
       group: dome
+      readonly: true
     dome_status2:
       address: 411
       mode: holding_register
       group: dome
-    rolloff_lockout:
-      address: 110
+      readonly: true
+    dome_set_frequency:
+      address: 412
+      mode: holding_register
       group: dome
-    rolloff_error:
-      address: 111
+      readonly: true
+    dome_output_frequency:
+      address: 413
+      mode: holding_register
       group: dome
-    rolloff_error_reset:
-      address: 112
+      readonly: true
+    dome_output_current:
+      address: 129
+      mode: holding_register
+      group: dome
+      readonly: true
+    dome_output_voltage:
+      address: 416
+      mode: holding_register
       group: dome
+      readonly: true
+    dome_motor_current_rpm:
+      address: 417
+      mode: holding_register
+      group: dome
+      readonly: true
+    dome_present_fault_record:
+      address: 399
+      mode: holding_register
+      group: dome
+      readonly: true
     oxygen_read_utilities_room:
       address: 599
       mode: holding_register
       group: safety
+      readonly: true
     oxygen_read_spectrograph_room:
       address: 600
       mode: holding_register
       group: safety
-    oxygen_mode_utilities_room:
+      readonly: true
+    oxygen_error_code_utilities_room:
       address: 601
       mode: holding_register
       group: safety
-    oxygen_mode_spectrograph_room:
+      readonly: true
+    oxygen_error_code_spectrograph_room:
       address: 602
       mode: holding_register
       group: safety
+      readonly: true
     hb_set:
       address: 599
       mode: coil
+      readonly: false
       group: safety
     hb_ack:
       address: 600
       mode: coil
+      readonly: true
       group: safety
     hb_error:
       address: 601
       mode: coil
+      readonly: true
       group: safety
     rain_sensor_alarm:
       address: 699
       mode: coil
+      readonly: true
       group: safety
-    rain_sensor_counter:
-      address: 699
+    rain_sensor_countdown:
+      address: 700
       mode: holding_register
+      readonly: true
       group: safety
+    engineering_mode_hardware:
+      address: 899
+      mode: coil
+      readonly: false
+      group: engineering_mode
+    engineering_mode_software:
+      address: 900
+      mode: coil
+      readonly: false
+      group: engineering_mode
 
 safety:
   override_local_mode: False
@@ -187,135 +278,167 @@ hvac:
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_flowmeter_ahu_spectrograph:
       address: 2
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_exterior_humidity:
       address: 4
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_humidity_utilities_room:
       address: 6
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_humidity_spectrograph_room:
       address: 8
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_humidity_telescope_platform:
       address: 10
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_water_pressure_inlet_circuit_1:
       address: 12
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_water_pressure_inlet_circuit_2:
       address: 14
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_exterior_temperature:
       address: 16
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_utilities_room_temperature:
       address: 18
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_spectrograph_room_temperature:
       address: 20
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_telescope_platform_temperature:
       address: 22
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_chiller_in_water_temperature:
       address: 24
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_ahu_in_water_temperature:
       address: 26
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_ahu_injection_temperature:
       address: 28
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_ahu_return_temperature:
       address: 30
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_chiller_out_temperature:
       address: 32
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_ahu_out_temperature:
       address: 34
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_ahu_cold_valve:
       address: 36
       mode: holding_register
       count: 2
       decoder: float_32bit
+      readonly: true
     hvac_system_status:
       address: 0
       mode: coil
+      readonly: true
     hvac_ahu_filter_status:
       address: 1
       mode: coil
+      readonly: true
     hvac_vin_1_air_pressure:
       address: 2
       mode: coil
+      readonly: true
     hvac_vin_2_air_pressure:
       address: 3
       mode: coil
+      readonly: true
     hvac_roll_off_roof_position:
       address: 4
       mode: coil
+      readonly: true
     hvac_status_damper_1:
       address: 5
       mode: coil
+      readonly: true
     hvac_status_damper_2:
       address: 6
       mode: coil
+      readonly: true
     hvac_ahu_heater_1:
       address: 7
       mode: coil
+      readonly: true
     hvac_start_stop_chiller:
       address: 8
       mode: coil
+      readonly: true
     hvac_start_stop_vin_1:
       address: 9
       mode: coil
+      readonly: true
     hvac_start_stop_vin_2:
       address: 10
       mode: coil
+      readonly: true
     hvac_start_stop_water_pump:
       address: 11
       mode: coil
+      readonly: true
     hvac_start_stop_ahu:
       address: 12
       mode: coil
+      readonly: true
 
 engineering_mode:
   default_duration: 300

From d6b7f76217a390a910c609c6b2a64d8f2f54823b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sun, 29 Dec 2024 02:40:11 +0000
Subject: [PATCH 02/12] Update config

---
 python/lvmecp/etc/lvmecp.yml | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/python/lvmecp/etc/lvmecp.yml b/python/lvmecp/etc/lvmecp.yml
index ac3ac40..0928c9b 100644
--- a/python/lvmecp/etc/lvmecp.yml
+++ b/python/lvmecp/etc/lvmecp.yml
@@ -159,7 +159,7 @@ modbus:
       group: dome
       readonly: true
     dome_speed:
-      address: 150
+      address: 151
       mode: holding_register
       group: dome
       readonly: true
@@ -243,6 +243,11 @@ modbus:
       mode: coil
       readonly: true
       group: safety
+    rain_sensor_count:
+      address: 699
+      mode: holding_register
+      readonly: true
+      group: safety
     rain_sensor_countdown:
       address: 700
       mode: holding_register
@@ -259,15 +264,6 @@ modbus:
       readonly: false
       group: engineering_mode
 
-safety:
-  override_local_mode: False
-  o2_threshold: 19.5
-
-dome:
-  daytime_allowed: false
-  daytime_tolerance: 600
-  anti_flap_tolerance: [3, 600]
-
 hvac:
   host: 10.8.38.49
   port: 502
@@ -443,8 +439,17 @@ hvac:
 engineering_mode:
   default_duration: 300
 
+safety:
+  o2_threshold: 19.5
+
+dome:
+  daytime_allowed: false
+  daytime_tolerance: 600
+  anti_flap_tolerance: [3, 600]
+  full_open_mm: 9480
+
 actor:
-  name: lvmecp
+  name: lvmecp-dev
   host: localhost
   port: 5672
   log_dir: /data/logs/lvmecp

From b2e483d64f9859bfe3ca2026a4cbba9b0836fb8a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sun, 29 Dec 2024 02:41:50 +0000
Subject: [PATCH 03/12] Improve performance of PLC reading

---
 python/lvmecp/actor/actor.py           |  10 +-
 python/lvmecp/actor/commands/status.py |  18 +-
 python/lvmecp/dome.py                  |  44 ++--
 python/lvmecp/etc/schema.json          |  21 +-
 python/lvmecp/hvac.py                  |   4 +-
 python/lvmecp/lights.py                |   2 +-
 python/lvmecp/maskbits.py              |   3 +
 python/lvmecp/modbus.py                | 332 +++++++++++++++----------
 python/lvmecp/module.py                |  33 ++-
 python/lvmecp/plc.py                   |  22 +-
 python/lvmecp/safety.py                |  25 +-
 python/lvmecp/tools.py                 |  50 +++-
 12 files changed, 373 insertions(+), 191 deletions(-)

diff --git a/python/lvmecp/actor/actor.py b/python/lvmecp/actor/actor.py
index 10047ff..1a89997 100644
--- a/python/lvmecp/actor/actor.py
+++ b/python/lvmecp/actor/actor.py
@@ -60,8 +60,6 @@ def __init__(
         else:
             self.plc = plc
 
-        self.semaphore = asyncio.Semaphore(5)
-
         self._emit_status_task: asyncio.Task | None = None
         self._monitor_dome_task: asyncio.Task | None = None
 
@@ -71,8 +69,6 @@ def __init__(
         self._engineering_mode_duration: float | None = None
         self._engineering_mode_task: asyncio.Task | None = None
 
-        self._last_heartbeat: float | None = None
-
         self.running: bool = False
 
     async def start(self, **kwargs):
@@ -81,6 +77,8 @@ async def start(self, **kwargs):
         await super().start(**kwargs)
         self.running = True
 
+        await self.plc.read_all_registers(use_cache=False)
+
         # Start PLC modules now that the actor is running. This prevents the modules
         # trying to broadcast messages before the actor is ready.
         await self.plc.start_modules()
@@ -185,9 +183,7 @@ async def emit_heartbeat(self):
         """Emits a heartbeat to the PLC."""
 
         self.log.debug("Emitting heartbeat to the PLC.")
-        await self.plc.modbus["hb_set"].set(True)
-
-        self._last_heartbeat = time.time()
+        await self.plc.modbus["hb_set"].write(True)
 
     async def _check_internal(self):
         return await super()._check_internal()
diff --git a/python/lvmecp/actor/commands/status.py b/python/lvmecp/actor/commands/status.py
index 2f771cf..b4f1173 100644
--- a/python/lvmecp/actor/commands/status.py
+++ b/python/lvmecp/actor/commands/status.py
@@ -26,19 +26,27 @@
 
 @parser.command()
 @click.option("--no-registers", is_flag=True, help="Does not output registers.")
-async def status(command: ECPCommand, no_registers: bool = False):
+@click.option("--no-cache", is_flag=True, help="Ignores the internal cache.")
+async def status(
+    command: ECPCommand,
+    no_registers: bool = False,
+    no_cache: bool = False,
+):
     """Returns the enclosure status."""
 
     plc = command.actor.plc
 
     if no_registers is False:
-        async with command.actor.semaphore:
-            command.info(registers=(await plc.read_all_registers(use_cache=False)))
+        command.info(registers=(await plc.read_all_registers(use_cache=not no_cache)))
 
     modules: list[PLCModule] = [plc.dome, plc.safety, plc.lights]
     await asyncio.gather(
         *[
-            module.update(force_output=True, command=command, use_cache=True)
+            module.update(
+                force_output=True,
+                command=command,
+                use_cache=True,
+            )
             for module in modules
         ]
     )
@@ -48,6 +56,6 @@ async def status(command: ECPCommand, no_registers: bool = False):
         o2_percent_spectrograph=plc.safety.o2_level_spectrograph,
     )
 
-    command.info(last_heartbeat_set=timestamp_to_iso(command.actor._last_heartbeat))
+    command.info(heartbeat_ack=timestamp_to_iso(plc.safety.last_heartbeat_ack))
 
     return command.finish()
diff --git a/python/lvmecp/dome.py b/python/lvmecp/dome.py
index 29c006b..bf95e10 100644
--- a/python/lvmecp/dome.py
+++ b/python/lvmecp/dome.py
@@ -13,6 +13,7 @@
 from time import time
 from types import SimpleNamespace
 
+import numpy
 from astropy.time import Time
 from lvmopstools.ephemeris import get_ephemeris_summary
 
@@ -42,10 +43,9 @@ async def _update_internal(self, use_cache: bool = True, **kwargs):
         assert self.flag
         new_status = self.flag(0)
 
-        if dome_status.drive_state:
-            new_status |= self.flag.DRIVE_AVAILABLE
-        else:
-            new_status |= self.flag.NODRIVE
+        # The variable that would determine if the drive is available is not
+        # does not exist anymore, so we assume it is.
+        new_status |= self.flag.DRIVE_AVAILABLE
 
         if dome_status.drive_enabled:
             new_status |= self.flag.DRIVE_ENABLED
@@ -57,12 +57,6 @@ async def _update_internal(self, use_cache: bool = True, **kwargs):
             else:
                 new_status |= self.flag.MOTOR_CLOSING
 
-        if dome_status.drive_brake:
-            new_status |= self.flag.BRAKE_ENABLED
-
-        # if dome_status.overcurrent:
-        #     new_status |= self.flag.OVERCURRENT
-
         if dome_status.dome_open is True:
             new_status |= self.flag.OPEN
         elif dome_status.dome_closed is True:
@@ -73,12 +67,24 @@ async def _update_internal(self, use_cache: bool = True, **kwargs):
         if new_status.value == 0:
             new_status = self.flag(self.flag.__unknown__)
 
-        return new_status
+        if dome_status.dome_open:
+            percent_open = 1
+        elif dome_status.dome_closed:
+            percent_open = 0
+        else:
+            full_open = config["dome.full_open_mm"]
+            percent_open = numpy.clip(dome_status.dome_position / full_open, 0, 1)
+
+        extra_info = {
+            "dome_percent_open": round(float(percent_open) * 100, 1),
+        }
+
+        return new_status, extra_info
 
     async def set_direction(self, open: bool):
         """Sets the motor direction (`True` means open, `False` close)."""
 
-        await self.modbus["drive_direction"].set(open)
+        await self.modbus["drive_direction"].write(open)
         await self.update(use_cache=False)
 
     async def _move(self, open: bool, force: bool = False):
@@ -117,12 +123,12 @@ async def _move(self, open: bool, force: bool = False):
                 warnings.warn("Dome already at position, but forcing.", ECPWarning)
 
         log.debug("Setting motor_direction.")
-        await self.modbus["motor_direction"].set(open)
+        await self.modbus["motor_direction"].write(open)
 
         await asyncio.sleep(0.5)
 
         log.debug("Setting drive_enabled.")
-        await self.modbus["drive_enabled"].set(True)
+        await self.modbus["drive_enabled"].write(True)
 
         await asyncio.sleep(0.5)
 
@@ -131,8 +137,10 @@ async def _move(self, open: bool, force: bool = False):
             # Still moving.
             await asyncio.sleep(2)
 
-            drive_enabled = await self.modbus["drive_enabled"].get()
-            move_done = await self.modbus["dome_open" if open else "dome_closed"].get()
+            drive_enabled = await self.modbus["drive_enabled"].read(use_cache=False)
+
+            move_done_register = self.modbus["dome_open" if open else "dome_closed"]
+            move_done = await move_done_register.read(use_cache=False)
 
             if drive_enabled:
                 last_enabled = time()
@@ -174,14 +182,14 @@ async def stop(self):
         if not drive_enabled:
             return
 
-        await self.plc.modbus["drive_enabled"].set(False)
+        await self.plc.modbus["drive_enabled"].write(False)
 
         await self.update(use_cache=False)
 
     async def reset(self):
         """Resets the roll-off error state."""
 
-        await self.modbus["rolloff_error_reset"].set(1)
+        await self.modbus["rolloff_error_reset"].write(True)
         await asyncio.sleep(1)
 
     def is_allowed(self):
diff --git a/python/lvmecp/etc/schema.json b/python/lvmecp/etc/schema.json
index 742403d..bc1d288 100644
--- a/python/lvmecp/etc/schema.json
+++ b/python/lvmecp/etc/schema.json
@@ -23,9 +23,24 @@
     },
     "o2_percent_utilities": { "type": "number" },
     "o2_percent_spectrograph": { "type": "number" },
-    "last_heartbeat_set": {
+    "heartbeat_ack": {
       "oneOf": [{ "type": "string" }, { "type": "null" }]
     },
+    "register": {
+      "type": "object",
+      "properties": {
+        "name": { "type": "string" },
+        "address": { "type": "number" },
+        "value": {
+          "oneOf": [
+            { "type": "boolean" },
+            { "type": "number" },
+            { "type": "null" }
+          ]
+        }
+      },
+      "required": ["name", "value"]
+    },
     "engineering_mode": {
       "type": "object",
       "properties": {
@@ -35,7 +50,9 @@
         },
         "ends_at": {
           "oneOf": [{ "type": "string" }, { "type": "null" }]
-        }
+        },
+        "software_override": { "type": "boolean" },
+        "hardware_override": { "type": "boolean" }
       },
       "required": ["enabled", "started_at", "ends_at"]
     }
diff --git a/python/lvmecp/hvac.py b/python/lvmecp/hvac.py
index 42e6331..8559555 100644
--- a/python/lvmecp/hvac.py
+++ b/python/lvmecp/hvac.py
@@ -23,9 +23,9 @@ class HVACController(PLCModule):
     def __init__(self, *args, **kwargs):
         super().__init__(*args, **kwargs)
 
-        self.status: dict[str, float | bool | None] = {}
+        self.status: dict[str, int | bool] = {}
 
     async def _update_internal(self, **kwargs):
         """Update status."""
 
-        self.status = await self.modbus.get_all()
+        self.status = await self.modbus.read_all()
diff --git a/python/lvmecp/lights.py b/python/lvmecp/lights.py
index ff22082..a72810f 100644
--- a/python/lvmecp/lights.py
+++ b/python/lvmecp/lights.py
@@ -123,7 +123,7 @@ async def toggle(self, light: str):
         code = self.get_code(light)
 
         log.debug(f"Toggling light {code}.")
-        await self.modbus[f"{code}_new"].set(True)
+        await self.modbus[f"{code}_new"].write(True)
 
         await asyncio.sleep(0.5)
         await self.update(use_cache=False)
diff --git a/python/lvmecp/maskbits.py b/python/lvmecp/maskbits.py
index 967cf8f..4d226dd 100644
--- a/python/lvmecp/maskbits.py
+++ b/python/lvmecp/maskbits.py
@@ -54,6 +54,9 @@ class SafetyStatus(Maskbit):
     O2_SENSOR_SR_ALARM = 0x400  # Spec room
     O2_SENSOR_SR_FAULT = 0x800
     RAIN_SENSOR_ALARM = 0x1000
+    E_STOP = 0x2000
+    DOME_LOCKED = 0x4000
+    DOME_ERROR = 0x8000
     UNKNOWN = 0x100000
 
 
diff --git a/python/lvmecp/modbus.py b/python/lvmecp/modbus.py
index 3c7c94e..e0dff4e 100644
--- a/python/lvmecp/modbus.py
+++ b/python/lvmecp/modbus.py
@@ -12,8 +12,9 @@
 import pathlib
 from time import time
 
-from typing import cast
+from typing import Literal, Sequence
 
+from lvmopstools.retrier import Retrier
 from pymodbus.client.tcp import AsyncModbusTcpClient
 from pymodbus.constants import Endian
 from pymodbus.payload import BinaryPayloadDecoder
@@ -24,10 +25,15 @@
 from lvmecp import config as lvmecp_config
 from lvmecp import log
 from lvmecp.exceptions import ECPError
+from lvmecp.tools import TimedCacheDict
 
 
 MAX_RETRIES = 3
-TIMEOUT = 10.0
+MAX_COUNT_HR = 100
+CONNECTION_TIMEOUT = 10.0
+
+
+RegisterModes = Literal["coil", "holding_regiser", "discrete_input", "input_register"]
 
 
 class ModbusRegister:
@@ -47,6 +53,8 @@ class ModbusRegister:
         or ``input_register``.
     group
         A grouping key for registers.
+    readonly
+        Whether the register is read-only.
 
     """
 
@@ -55,10 +63,11 @@ def __init__(
         modbus: Modbus,
         name: str,
         address: int,
-        mode: str = "coil",
+        mode: RegisterModes = "coil",
         count: int = 1,
         group: str | None = None,
         decoder: str | None = None,
+        readonly: bool = True,
     ):
         self.modbus = modbus
         self.client = modbus.client
@@ -69,18 +78,11 @@ def __init__(
         self.count = count
         self.group = group
         self.decoder = decoder
+        self.readonly = readonly
 
-        self._last_value: int | float = 0
-        self._last_seen: float = 0
-
-    async def _get_internal(self, use_cache: bool = True):
+    async def _read_internal(self):
         """Return the value of the modbus register."""
 
-        cache_timeout = self.modbus.cache_timeout
-        last_seen_interval = time() - self._last_seen
-        if use_cache and last_seen_interval < cache_timeout:
-            return self._last_value
-
         if self.mode == "coil":
             func = self.client.read_coils
         elif self.mode == "holding_register":
@@ -92,19 +94,12 @@ async def _get_internal(self, use_cache: bool = True):
         else:
             raise ValueError(f"Invalid block mode {self.mode!r}.")
 
-        if self.client.connected:
+        async with self.modbus:
             resp = await func(
                 self.address,
                 count=self.count,
                 slave=self.modbus.slave,
-            )  # type: ignore
-        else:
-            async with self.modbus:
-                resp = await func(
-                    self.address,
-                    count=self.count,
-                    slave=self.modbus.slave,
-                )  # type: ignore
+            )
 
         if resp.function_code > 0x80:
             raise ValueError(
@@ -119,87 +114,84 @@ async def _get_internal(self, use_cache: bool = True):
             registers = resp.registers
             value = registers[0 : self.count] if self.count > 1 else registers[0]
 
-            if self.decoder is not None:
-                if self.decoder == "float_32bit":
-                    bin_payload = BinaryPayloadDecoder.fromRegisters(
-                        value,
-                        byteorder=Endian.BIG,
-                        wordorder=Endian.LITTLE,
-                    )
-                    value = round(bin_payload.decode_32bit_float(), 3)
-                else:
-                    raise ValueError(f"Unknown decoder {self.decoder}")
+        value = self.decode(value)
 
         if not isinstance(value, (int, float)):
             raise ValueError(f"Invalid type for {self.name!r} response.")
 
-        self._last_value = value
-        self._last_seen = time()
+        return value
+
+    def decode(self, value: int | bool | list[int | bool]):
+        """Decodes the raw value from the register."""
+
+        if self.decoder is not None:
+            if self.decoder == "float_32bit":
+                bin_payload = BinaryPayloadDecoder.fromRegisters(
+                    value,
+                    byteorder=Endian.BIG,
+                    wordorder=Endian.LITTLE,
+                )
+                value = round(bin_payload.decode_32bit_float(), 3)
+            else:
+                raise ValueError(f"Unknown decoder {self.decoder}")
 
         return value
 
-    async def get(self, open_connection: bool = True, use_cache: bool = True):
-        """Return the value of the modbus register. Implements retry."""
+    @Retrier(max_attempts=MAX_RETRIES, delay=0.5, max_delay=2.0)
+    async def read(self, use_cache: bool = True):
+        """Return the value of the modbus register.
 
-        for ntries in range(1, MAX_RETRIES + 1):
-            # If we need to open the connection, use the Modbus context
-            # and call ourselves recursively with open_connection=False
-            # (at that point it will be open).
-            if open_connection:
-                await self.modbus.connect()
+        Parameters
+        ----------
+        use_cache
+            Whether to use the cache to retrieve the value. If the cache is not
+            available, or the value is not in the cache, the register will be read.
+            This function does not set the cache after reading the register.
 
-            if not self.modbus.client or not self.modbus.client.connected:
-                raise ConnectionError("Not connected to modbus server.")
+        """
 
-            try:
-                return await self._get_internal(use_cache=use_cache)
-            except Exception:
-                if ntries >= MAX_RETRIES:
-                    raise
+        if use_cache:
+            cache = self.modbus.register_cache
+            if self.name in cache and (value := cache[self.name]) is not None:
+                return value
 
-                await asyncio.sleep(0.5)
-            finally:
-                if open_connection:
-                    await self.modbus.disconnect()
+        return await self._read_internal()
 
-    async def set(self, value: int | bool):
+    @Retrier(max_attempts=MAX_RETRIES, delay=0.5, max_delay=2.0)
+    async def write(self, value: int | bool):
         """Sets the value of the register."""
 
-        for ntries in range(1, MAX_RETRIES + 1):
-            # Always open the connection.
-            async with self.modbus:
-                if self.mode == "coil":
-                    func = self.client.write_coil
-                elif self.mode == "holding_register":
-                    func = self.client.write_register
-                elif self.mode == "discrete_input" or self.mode == "input_register":
-                    raise ValueError(f"Block of mode {self.mode!r} is read-only.")
-                else:
-                    raise ValueError(f"Invalid block mode {self.mode!r}.")
+        if self.readonly:
+            raise ECPError(f"Register {self.name!r} is read-only.")
 
-                try:
-                    if self.client.connected:
-                        resp = await func(self.address, value)  # type: ignore
-                    else:
-                        async with self.modbus:
-                            resp = await func(self.address, value)  # type: ignore
-
-                    if resp.function_code > 0x80:
-                        raise ECPError(
-                            f"Invalid response for element "
-                            f"{self.name!r}: 0x{resp.function_code:02X}."
-                        )
-                    else:
-                        self._last_value = int(value)
-                        self._last_seen = time()
+        # Always open the connection.
+        async with self.modbus:
+            if self.mode == "coil":
+                func = self.client.write_coil
+            elif self.mode == "holding_register":
+                func = self.client.write_register
+            elif self.mode == "discrete_input" or self.mode == "input_register":
+                raise ValueError(f"Block of mode {self.mode!r} is read-only.")
+            else:
+                raise ValueError(f"Invalid block mode {self.mode!r}.")
 
-                        return
+            try:
+                if self.client.connected:
+                    resp = await func(self.address, value)  # type: ignore
+                else:
+                    async with self.modbus:
+                        resp = await func(self.address, value)  # type: ignore
 
-                except Exception as err:
-                    if ntries >= MAX_RETRIES:
-                        raise ECPError(f"Failed setting {self.name!r}: {err}")
+                if resp.function_code > 0x80:
+                    raise ECPError(
+                        f"Invalid response for element "
+                        f"{self.name!r}: 0x{resp.function_code:02X}."
+                    )
+                else:
+                    self.modbus.register_cache[self.name] = value
 
-                await asyncio.sleep(0.5)
+            except Exception as err:
+                raise ECPError(f"Failed setting {self.name!r}: {err}")
 
 
 class Modbus(dict[str, ModbusRegister]):
@@ -232,40 +224,42 @@ def __init__(self, config: dict | pathlib.Path | str | None = None):
         self.port = self.config["port"]
         self.slave = self.config.get("slave", 0)
 
-        # Cache results so that very close calls to get_all() don't need to
-        # open a connection and read the registers.
-        self.cache_timeout = self.config.get("cache_timeout", 0.5)
-        self._register_cache: dict[str, int | float | None] = {}
-        self._register_last_seen: float = 0
+        # Cache results so that very close calls to get_all()
+        # don't need to open a connection and read the registers.
+        self.cache_timeout = self.config.get("cache_timeout", 1)
+        self.register_cache = TimedCacheDict(self.cache_timeout, mode="null")
 
+        # Modbus client.
         self.client = AsyncModbusTcpClient(self.host, port=self.port)
 
-        self.lock = asyncio.Lock()
-        self._lock_release_task: asyncio.Task | None = None
+        # Semaphore to allow up to 5 concurrent connections.
+        self.semaphore = asyncio.Semaphore(1)
+        self._semaphore_release_task: asyncio.Task | None = None
 
-        register_data = self.config["registers"]
+        # Create the internal dictionary of registers
         registers = {
             name: ModbusRegister(
                 self,
                 name,
-                elem["address"],
-                mode=elem.get("mode", "coil"),
-                group=elem.get("group", None),
-                count=elem.get("count", 1),
-                decoder=elem.get("decoder", None),
+                register["address"],
+                mode=register.get("mode", "coil"),
+                group=register.get("group", None),
+                count=register.get("count", 1),
+                decoder=register.get("decoder", None),
+                readonly=register.get("readonly", True),
             )
-            for name, elem in register_data.items()
+            for name, register in self.config["registers"].items()
         }
 
         dict.__init__(self, registers)
-        for name, elem in registers.items():
-            setattr(self, name, elem)
+        for name, register in registers.items():
+            setattr(self, name, register)
 
     async def connect(self):
         """Connects to the client."""
 
         try:
-            await asyncio.wait_for(self.lock.acquire(), TIMEOUT)
+            await asyncio.wait_for(self.semaphore.acquire(), CONNECTION_TIMEOUT)
         except asyncio.TimeoutError:
             raise RuntimeError("Timed out waiting for lock to be released.")
 
@@ -282,15 +276,15 @@ async def connect(self):
         except Exception as err:
             raise ConnectionError(f"Failed connecting to server at {hp}: {err}.")
         finally:
-            if not did_connect and self.lock.locked():
-                self.lock.release()
+            if not did_connect:
+                self.semaphore.release()
 
         log.debug(f"Connected to {hp}.")
 
         # Schedule a task to release the lock after 5 seconds. This is a safeguard
         # in case something fails and the connection is never closed and the lock
         # not released.
-        self._lock_release_task = asyncio.create_task(self.unlock_on_timeout())
+        self._semaphore_release_task = asyncio.create_task(self.unlock_on_timeout())
 
     async def disconnect(self):
         """Disconnects the client."""
@@ -301,10 +295,9 @@ async def disconnect(self):
                 log.debug(f"Disonnected from {self.host}:{self.port}.")
 
         finally:
-            if self.lock.locked():
-                self.lock.release()
+            self.semaphore.release()
 
-            await cancel_task(self._lock_release_task)
+            await cancel_task(self._semaphore_release_task)
 
     async def __aenter__(self):
         """Initialises the connection to the server."""
@@ -319,50 +312,97 @@ async def __aexit__(self, exc_type, exc, tb):
     async def unlock_on_timeout(self):
         """Removes the lock after an amount of time."""
 
-        await asyncio.sleep(TIMEOUT)
-        if self.lock.locked():
-            self.lock.release()
+        await asyncio.sleep(CONNECTION_TIMEOUT)
+        self.semaphore.release()
 
-    async def get_all(self, use_cache: bool = True):
-        """Returns a dictionary with all the registers."""
+    async def read_all(self, use_cache: bool = True) -> dict[str, int | bool]:
+        """Returns a dictionary with all the registers and sets the cache."""
 
-        if use_cache and time() - self._register_last_seen < self.cache_timeout:
-            if None not in self._register_cache.values():
-                return self._register_cache
-
-        names = results = []
+        if use_cache:
+            oldest_cache = min(self.register_cache._cache_time.values())
+            if time() - oldest_cache < self.cache_timeout:
+                return self.register_cache.freeze()
 
+        # With this PLC it is more efficient to read the entire coil and
+        # holding register blocks in one go, and then parse the results, as
+        # opposed to reading each register individually.
         async with self:
-            names = [name for name in self]
-            tasks = [
-                elem.get(open_connection=False, use_cache=False)
-                for elem in self.values()
-            ]
+            for mode in ["coil", "holding_register"]:
+                mode_count = max(
+                    register.address + register.count
+                    for register in self.values()
+                    if register.mode == mode
+                )
+
+                if mode == "coil":
+                    func = self.client.read_coils
+                elif mode == "holding_register":
+                    func = self.client.read_holding_registers
+                else:
+                    raise ValueError(f"Invalid mode {mode!r}.")
+
+                data: list[int | bool] = []
+
+                if mode == "coil":
+                    # For coils we can read the entire block.
+                    resp = await func(0, count=1023, slave=self.slave)
+
+                    if resp.isError():
+                        raise ValueError(
+                            f"Invalid response for block {mode!r}: "
+                            f"0x{resp.function_code:02X}."
+                        )
+
+                    data += resp.bits
+
+                else:
+                    # For holding registers we are limited to reading MAX_COUNT_HR
+                    # at once. We iterate to get all the data.
+                    n_reads = mode_count // MAX_COUNT_HR + 1
+
+                    for nn in range(n_reads):
+                        resp = await func(
+                            nn * MAX_COUNT_HR,
+                            count=MAX_COUNT_HR,
+                            slave=self.slave,
+                        )
 
-            results = await asyncio.gather(*tasks, return_exceptions=True)
+                        if resp.isError():
+                            raise ValueError(
+                                f"Invalid response for block {mode!r}: "
+                                f"0x{resp.function_code:02X}."
+                            )
 
-        if any([isinstance(result, Exception) for result in results]):
-            for ii, result in enumerate(results):
-                if isinstance(result, Exception):
-                    log.warning(f"Failed retrieving value for {names[ii]!r}")
-                    results[ii] = None
+                        data += resp.registers
 
-        registers = cast(
-            dict[str, int | float | None],
-            {names[ii]: results[ii] for ii in range(len(names))},
-        )
+                for name, register in self.items():
+                    if register.mode != mode:
+                        continue
 
-        self._register_cache = registers
-        self._register_last_seen = time()
+                    value = data[register.address : register.address + register.count]
+                    value = register.decode(value)
+
+                    if isinstance(value, Sequence) and len(value) == 1:
+                        value = value[0]
+
+                    self.register_cache[name] = value
+
+        registers = self.register_cache.freeze()
+
+        # Just to double check that none of the values have expired, we loop over the
+        # dictionary and if necessary we read the register again.
+        for name, value in registers.items():
+            if value is None:
+                registers[name] = await self[name].read(use_cache=False)
 
         return registers
 
     async def read_group(self, group: str, use_cache: bool = True):
         """Returns a dictionary of all read registers that match a ``group``."""
 
-        registers = await self.get_all(use_cache=use_cache)
+        registers = await self.read_all(use_cache=use_cache)
 
-        group_registers = {}
+        group_registers: dict[str, int | bool] = {}
         for name in self:
             register = self[name]
             if register.group is not None and register.group == group:
@@ -372,3 +412,23 @@ async def read_group(self, group: str, use_cache: bool = True):
                 group_registers[name] = registers[name]
 
         return group_registers
+
+    async def write_register(self, register: str | int, value: int | bool):
+        """Writes a value to a register."""
+
+        if isinstance(register, int):
+            for name, reg in self.items():
+                if reg.address == register:
+                    register = name
+                    found = True
+                    break
+
+            if not found:
+                raise ValueError(f"Register with address {register!r} not found.")
+
+        assert isinstance(register, str)
+
+        if register not in self:
+            raise ValueError(f"Register {register!r} not found.")
+
+        await self[register].write(value)
diff --git a/python/lvmecp/module.py b/python/lvmecp/module.py
index e80e307..4408f8c 100644
--- a/python/lvmecp/module.py
+++ b/python/lvmecp/module.py
@@ -11,7 +11,16 @@
 import abc
 import asyncio
 
-from typing import TYPE_CHECKING, Callable, Coroutine, Generic, Type, TypeVar
+from typing import (
+    TYPE_CHECKING,
+    Any,
+    Callable,
+    Coroutine,
+    Generic,
+    Sequence,
+    Type,
+    TypeVar,
+)
 
 from lvmecp import log
 from lvmecp.tools import cancel_tasks_by_name
@@ -80,7 +89,10 @@ async def _status_loop(self):
             await asyncio.sleep(self._interval)
 
     @abc.abstractmethod
-    async def _update_internal(self, **kwargs) -> Flag_co:
+    async def _update_internal(
+        self,
+        **kwargs,
+    ) -> Flag_co | tuple[Flag_co, dict[str, Any]]:
         """Determines the new module flag status."""
 
         pass
@@ -94,14 +106,22 @@ async def update(
         """Refreshes the module status."""
 
         try:
-            new_status = await self._update_internal(use_cache=use_cache)
+            internal_output = await self._update_internal(use_cache=use_cache)
+            if isinstance(internal_output, Sequence):
+                new_status, extra_info = internal_output
+            else:
+                new_status, extra_info = internal_output, {}
         except Exception as err:
             log.warning(f"{self.name}: failed updating status: {err}")
             new_status = self.flag(self.flag.__unknown__) if self.flag else None
 
         # Only notify if the status has changed.
-        if new_status != self.status or force_output:
-            await self.notify_status(new_status, **notifier_kwargs)
+        if (new_status != self.status and not extra_info) or force_output:
+            await self.notify_status(
+                new_status,
+                extra_keywords=extra_info,
+                **notifier_kwargs,
+            )
 
         self.status = new_status
 
@@ -110,6 +130,7 @@ async def update(
     async def notify_status(
         self,
         status: Flag_co | None = None,
+        extra_keywords: dict[str, Any] = {},
         wait: bool = False,
         **kwargs,
     ):
@@ -123,7 +144,7 @@ async def notify_status(
             return
 
         if asyncio.iscoroutinefunction(self.notifier):
-            coro = self.notifier(status.value, str(status), **kwargs)
+            coro = self.notifier(status.value, str(status), extra_keywords, **kwargs)
             if wait:
                 await coro
             else:
diff --git a/python/lvmecp/plc.py b/python/lvmecp/plc.py
index 36e4ad1..9a4c4e9 100644
--- a/python/lvmecp/plc.py
+++ b/python/lvmecp/plc.py
@@ -10,7 +10,7 @@
 
 import asyncio
 
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Any
 
 from lvmecp.hvac import HVACController
 from lvmecp.modbus import Modbus
@@ -32,15 +32,23 @@ def create_actor_notifier(
     use_hex: bool = True,
     labels_suffix="_labels",
     level="d",
+    allow_broadcasts: bool = False,
 ):
     """Generate a notifier function for a keyword."""
 
-    async def notifier(value: int, labels: str, command: Command | None = None):
+    async def notifier(
+        value: int,
+        labels: str,
+        extra_keywords: dict[str, Any] = {},
+        command: Command | None = None,
+    ):
         message = {
             keyword: value if use_hex is False else hex(value),
             f"{keyword}{labels_suffix}": labels,
         }
-        if command is None and actor:
+        message.update(extra_keywords)
+
+        if command is None and actor and allow_broadcasts:
             # Allow for 3 seconds for broadcast. This is needed because the PLC
             # starts before the actor and for the first message the exchange is
             # not yet available.
@@ -114,7 +122,9 @@ async def start_modules(self):
     async def read_all_registers(self, use_cache: bool = True):
         """Reads all the connected registers and returns a dictionary."""
 
-        registers = await self.modbus.get_all(use_cache=use_cache)
-        registers.update(await self.hvac_modbus.get_all(use_cache=use_cache))
+        registers_plc, registers_hvac = await asyncio.gather(
+            self.modbus.read_all(use_cache=use_cache),
+            self.hvac_modbus.read_all(use_cache=use_cache),
+        )
 
-        return registers
+        return registers_plc | registers_hvac
diff --git a/python/lvmecp/safety.py b/python/lvmecp/safety.py
index 37137f7..9a35147 100644
--- a/python/lvmecp/safety.py
+++ b/python/lvmecp/safety.py
@@ -9,6 +9,7 @@
 from __future__ import annotations
 
 import math
+import time
 from types import SimpleNamespace
 
 from lvmecp.maskbits import SafetyStatus
@@ -27,6 +28,8 @@ def __init__(self, *args, **kwargs):
         self.o2_level_utilities: float = math.nan
         self.o2_level_spectrograph: float = math.nan
 
+        self.last_heartbeat_ack: float | None = None
+
     async def _update_internal(self, use_cache: bool = True, **kwargs):
         assert self.flag is not None
 
@@ -51,33 +54,41 @@ async def _update_internal(self, use_cache: bool = True, **kwargs):
         self.o2_level_utilities = safety_status.oxygen_read_utilities_room / 10.0
         if self.o2_level_utilities < self.plc.config["safety"]["o2_threshold"]:
             new_status |= self.flag.O2_SENSOR_UR_ALARM
-        if safety_status.oxygen_mode_utilities_room == 8:
+        if safety_status.oxygen_error_code_utilities_room == 8:
             new_status |= self.flag.O2_SENSOR_UR_FAULT
 
         # Spectrograph room O2 sensor
         self.o2_level_spectrograph = safety_status.oxygen_read_spectrograph_room / 10.0
         if self.o2_level_spectrograph < self.plc.config["safety"]["o2_threshold"]:
             new_status |= self.flag.O2_SENSOR_SR_ALARM
-        if safety_status.oxygen_mode_spectrograph_room == 8:
+        if safety_status.oxygen_error_code_spectrograph_room == 8:
             new_status |= self.flag.O2_SENSOR_SR_FAULT
 
         # Rain sensor
         if safety_status.rain_sensor_alarm:
             new_status |= self.flag.RAIN_SENSOR_ALARM
 
+        # E-stop
+        if safety_status.e_status:
+            new_status |= self.flag.E_STOP
+
+        # Dome lockout and error
+        if await self.plc.modbus["dome_lockout"].read():
+            new_status |= self.flag.DOME_LOCKED
+        if await self.plc.modbus["dome_error"].read():
+            new_status |= self.flag.DOME_ERROR
+
         if new_status.value == 0:
             new_status = self.flag(self.flag.__unknown__)
 
+        if await self.plc.modbus["hb_ack"].read():
+            self.last_heartbeat_ack = time.time()
+
         return new_status
 
     async def is_remote(self):
         """Returns `True` if NOT in local mode (i.e., safe to operate remotely)."""
 
-        safety_config = self.plc.config.get("safety", {})
-        override_local = safety_config.get("override_local_mode", False)
-        if override_local:
-            return True
-
         await self.update()
         assert self.status is not None and self.flag is not None
 
diff --git a/python/lvmecp/tools.py b/python/lvmecp/tools.py
index a8512c4..04d5b9e 100644
--- a/python/lvmecp/tools.py
+++ b/python/lvmecp/tools.py
@@ -9,10 +9,11 @@
 from __future__ import annotations
 
 import asyncio
+import time
 from contextlib import suppress
 from datetime import datetime, timezone
 
-from typing import Any, Callable, Coroutine
+from typing import Any, Callable, Coroutine, Literal
 
 
 __all__ = ["loop_coro", "cancel_tasks_by_name", "timestamp_to_iso"]
@@ -60,3 +61,50 @@ def timestamp_to_iso(ts: float | None, timespec: str = "seconds") -> str | None:
         .isoformat(timespec=timespec)
         .replace("+00:00", "Z")
     )
+
+
+class TimedCacheDict(dict):
+    """A dictionary that caches values for a certain amount of time.
+
+    Parameters
+    ----------
+    timeout
+        The timeout in seconds for the cache.
+    mode
+        The mode for the cache. If ``delete``, the key will be deleted
+        after the timeout. If ``null``, the value will be set to ``None``.
+
+    """
+
+    def __init__(self, timeout: float, mode: Literal["delete", "null"] = "delete"):
+        self.timeout = timeout
+        self.mode = mode
+
+        self._cache_time: dict[str, float] = {}
+
+        super().__init__()
+
+    def freeze(self):
+        """Returns a non-cached version of the dictionary."""
+
+        return dict(self)
+
+    def __getitem__(self, key: str):
+        try:
+            if time.time() - self._cache_time[key] > self.timeout:
+                if self.mode == "delete":
+                    del self[key]
+                else:
+                    self[key] = None
+        except KeyError:
+            pass
+
+        return super().__getitem__(key)
+
+    def __setitem__(self, key: str, value):
+        self._cache_time[key] = time.time()
+        super().__setitem__(key, value)
+
+    def __delitem__(self, key: str):
+        del self._cache_time[key]
+        super().__delitem__(key)

From 3becdd790e95a183e4c21eec8b429b7d309b6dd2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sun, 29 Dec 2024 02:42:13 +0000
Subject: [PATCH 04/12] Report/sec software/hardware overrides in engineering
 mode

---
 python/lvmecp/actor/commands/engineering.py | 68 +++++++++++++++------
 1 file changed, 48 insertions(+), 20 deletions(-)

diff --git a/python/lvmecp/actor/commands/engineering.py b/python/lvmecp/actor/commands/engineering.py
index 200f485..e598c9d 100644
--- a/python/lvmecp/actor/commands/engineering.py
+++ b/python/lvmecp/actor/commands/engineering.py
@@ -18,9 +18,29 @@
 
 
 if TYPE_CHECKING:
-    from lvmecp.actor import ECPCommand
+    from lvmecp.actor import ECPActor, ECPCommand
 
 
+async def get_eng_mode_status(actor: ECPActor) -> dict:
+    enabled = actor.is_engineering_mode_enabled()
+    started_at = actor._engineering_mode_started_at
+    duration = actor._engineering_mode_duration
+
+    registers = await actor.plc.read_all_registers(use_cache=True)
+
+    if duration is None or started_at is None:
+        ends_at = None
+    else:
+        ends_at = started_at + duration
+
+    return {
+            "enabled": enabled,
+            "started_at": timestamp_to_iso(started_at),
+            "ends_at": timestamp_to_iso(ends_at),
+            "software_override": registers["engineering_mode_software"],
+            "hardware_override": registers["engineering_mode_hardware"],
+        }
+
 @parser.group(name="engineering-mode")
 def engineering_mode():
     """Enable/disable the engineering mode."""
@@ -36,12 +56,32 @@ def engineering_mode():
     help="Timeout for the engineering mode. "
     "If not passed, the default timeout is used.",
 )
-async def enable(command: ECPCommand, timeout: float | None = None):
+@click.option(
+    "--hardware-override",
+    is_flag=True,
+    help="Sets the hardware override flag.",
+)
+@click.option(
+    "--software-override",
+    is_flag=True,
+    help="Sets the software override flag.",
+)
+async def enable(
+    command: ECPCommand,
+    timeout: float | None = None,
+    hardware_override: bool = False,
+    software_override: bool = False,
+):
     """Enables the engineering mode."""
 
     await command.actor.engineering_mode(True, timeout=timeout)
 
-    return command.finish(engineering_mode=True)
+    if hardware_override:
+        await command.actor.plc.modbus.write_register("engineering_mode_hardware", True)
+    if software_override:
+        await command.actor.plc.modbus.write_register("engineering_mode_software", True)
+
+    return command.finish(engineering_mode=await get_eng_mode_status(command.actor))
 
 
 @engineering_mode.command()
@@ -50,26 +90,14 @@ async def disable(command: ECPCommand):
 
     await command.actor.engineering_mode(False)
 
-    return command.finish(engineering_mode=False)
+    await command.actor.plc.modbus.write_register("engineering_mode_hardware", False)
+    await command.actor.plc.modbus.write_register("engineering_mode_software", False)
+
+    return command.finish(engineering_mode=await get_eng_mode_status(command.actor))
 
 
 @engineering_mode.command()
 async def status(command: ECPCommand):
     """Returns the status of the engineering mode."""
 
-    enabled = command.actor.is_engineering_mode_enabled()
-    started_at = command.actor._engineering_mode_started_at
-    duration = command.actor._engineering_mode_duration
-
-    if duration is None or started_at is None:
-        ends_at = None
-    else:
-        ends_at = started_at + duration
-
-    return command.finish(
-        engineering_mode={
-            "enabled": enabled,
-            "started_at": timestamp_to_iso(started_at),
-            "ends_at": timestamp_to_iso(ends_at),
-        }
-    )
+    return command.finish(engineering_mode=await get_eng_mode_status(command.actor))

From d29a1449052cfc25c7a62b2173247c9fd1a6a2de Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sun, 29 Dec 2024 02:42:33 +0000
Subject: [PATCH 05/12] Add low-level modbus command

---
 python/lvmecp/actor/commands/modbus.py | 106 +++++++++++++++++++++++++
 1 file changed, 106 insertions(+)
 create mode 100644 python/lvmecp/actor/commands/modbus.py

diff --git a/python/lvmecp/actor/commands/modbus.py b/python/lvmecp/actor/commands/modbus.py
new file mode 100644
index 0000000..eea8cd4
--- /dev/null
+++ b/python/lvmecp/actor/commands/modbus.py
@@ -0,0 +1,106 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+#
+# @Author: José Sánchez-Gallego (gallegoj@uw.edu)
+# @Date: 2024-12-28
+# @Filename: modbus.py
+# @License: BSD 3-clause (http://www.opensource.org/licenses/BSD-3-Clause)
+
+from __future__ import annotations
+
+import asyncio
+
+from typing import TYPE_CHECKING
+
+import click
+
+from . import parser
+
+
+if TYPE_CHECKING:
+    from lvmecp.actor import ECPCommand
+    from lvmecp.modbus import ModbusRegister
+
+
+def get_register(command: ECPCommand, address_or_name: str):
+    """Returns a register from an address or name."""
+
+    plc = command.actor.plc
+
+    register: ModbusRegister | None = None
+    try:
+        address = int(address_or_name)  # type: ignore
+        for _, reg in plc.modbus.items():
+            if reg.address == address:
+                register = reg
+                break
+    except ValueError:
+        register = plc.modbus.get(address_or_name)
+
+    if register is None:
+        command.fail(f"Register {address!r} not found.")
+        return False
+
+    return register
+
+
+@parser.group()
+def modbus():
+    """Low-level access to the PLC Modbus variables."""
+
+    pass
+
+
+@modbus.command()
+@click.argument("address", metavar="ADDRESS|NAME")
+async def read(command: ECPCommand, address: str):
+    """Reads a Modbus register."""
+
+    if not (register := get_register(command, address)):
+        return False
+
+    value = await register.read(use_cache=False)
+
+    return command.finish(
+        register={
+            "name": register.name,
+            "address": register.address,
+            "value": value,
+        }
+    )
+
+
+@modbus.command()
+@click.argument("address", metavar="ADDRESS|NAME")
+@click.argument("value", type=int)
+async def write(command: ECPCommand, address: str, value: int):
+    """Writes a value to a Modbus register."""
+
+    if not (register := get_register(command, address)):
+        return False
+
+    name = register.name
+
+    if register.readonly:
+        return command.fail(f"Register {name!r} is read-only.")
+
+    if register.mode == "coil":
+        try:
+            value = bool(int(value))
+        except Exception:
+            return command.fail(f"Invalid value for coil register {name!r}: {value!r}")
+        else:
+            value = int(value)
+
+    await register.write(value)
+
+    await asyncio.sleep(0.5)
+    new_value = await register.read(use_cache=False)
+
+    return command.finish(
+        register={
+            "name": name,
+            "address": register.address,
+            "value": new_value,
+        }
+    )

From 0b5b27ad4aacdd13c2131479708b47608d2410b9 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sat, 28 Dec 2024 21:36:09 -0800
Subject: [PATCH 06/12] Improve the simulator to handle events and register
 overrides

---
 python/lvmecp/etc/lvmecp.yml    |  63 +++++++++++++++
 python/lvmecp/lights.py         |   2 +-
 python/lvmecp/modbus.py         |   3 +-
 python/lvmecp/simulator.py      | 139 ++++++++++++++++++--------------
 tests/conftest.py               |   2 +-
 tests/test_command_heartbeat.py |   4 +-
 6 files changed, 148 insertions(+), 65 deletions(-)

diff --git a/python/lvmecp/etc/lvmecp.yml b/python/lvmecp/etc/lvmecp.yml
index 0928c9b..9d6529c 100644
--- a/python/lvmecp/etc/lvmecp.yml
+++ b/python/lvmecp/etc/lvmecp.yml
@@ -436,6 +436,69 @@ hvac:
       mode: coil
       readonly: true
 
+simulator:
+  host: 127.0.0.1
+  port: 5020
+  overrides:
+    door_locked: true
+    door_closed: true
+    dome_closed: true
+  events:
+    ur_new:
+      on_value: 1
+      then:
+        register: ur_status
+        action: toggle
+        reset_trigger: true
+    cr_new:
+      on_value: 1
+      then:
+        register: cr_status
+        action: toggle
+        reset_trigger: true
+    sr_new:
+      on_value: 1
+      then:
+        register: sr_status
+        action: toggle
+        reset_trigger: true
+    uma_new:
+      on_value: 1
+      then:
+        register: uma_status
+        action: toggle
+        reset_trigger: true
+    tb_new:
+      on_value: 1
+      then:
+        register: tb_status
+        action: toggle
+        reset_trigger: true
+    tr_new:
+      on_value: 1
+      then:
+        register: tr_status
+        action: toggle
+        reset_trigger: true
+    hb_set:
+      on_value: 1
+      then:
+        register: hb_ack
+        action: set
+        reset_trigger: true
+    e_stop:
+      on_value: 1
+      then:
+        register: e_status
+        action: set
+        reset_trigger: true
+    e_relay_reset:
+      on_value: 1
+      then:
+        register: e_status
+        action: reset
+        reset_trigger: true
+
 engineering_mode:
   default_duration: 300
 
diff --git a/python/lvmecp/lights.py b/python/lvmecp/lights.py
index a72810f..5ecc37a 100644
--- a/python/lvmecp/lights.py
+++ b/python/lvmecp/lights.py
@@ -43,7 +43,7 @@ class LightsController(PLCModule):
     flag = LightStatus
     interval = 30.0
 
-    async def _update_internal(self, use_cache: bool = True):
+    async def _update_internal(self, use_cache: bool = True, **kwargs):
         """Update status."""
 
         assert self.flag is not None
diff --git a/python/lvmecp/modbus.py b/python/lvmecp/modbus.py
index e0dff4e..f285440 100644
--- a/python/lvmecp/modbus.py
+++ b/python/lvmecp/modbus.py
@@ -319,7 +319,8 @@ async def read_all(self, use_cache: bool = True) -> dict[str, int | bool]:
         """Returns a dictionary with all the registers and sets the cache."""
 
         if use_cache:
-            oldest_cache = min(self.register_cache._cache_time.values())
+            cache_times = self.register_cache._cache_time.values()
+            oldest_cache = min(cache_times) if len(cache_times) > 0 else 0
             if time() - oldest_cache < self.cache_timeout:
                 return self.register_cache.freeze()
 
diff --git a/python/lvmecp/simulator.py b/python/lvmecp/simulator.py
index 85e5000..f8f1995 100644
--- a/python/lvmecp/simulator.py
+++ b/python/lvmecp/simulator.py
@@ -9,10 +9,9 @@
 from __future__ import annotations
 
 import asyncio
-from contextlib import suppress
 from copy import deepcopy
 
-from typing import ClassVar, cast
+from typing import Any, cast
 
 from pymodbus.datastore import (
     ModbusServerContext,
@@ -21,6 +20,8 @@
 )
 from pymodbus.server import ServerAsyncStop, StartAsyncTcpServer
 
+from sdsstools.utils import cancel_task
+
 from lvmecp import config
 
 
@@ -30,24 +31,21 @@
 class Simulator:
     """A modbus simulator for a PLC controller."""
 
-    OVERRIDES: ClassVar[dict[str, int]] = {}
-
     def __init__(
         self,
         registers: dict,
-        address: str = "127.0.0.1",
+        host: str = "127.0.0.1",
         port: int = 5020,
-        overrides={},
+        overrides: dict[str, int | bool] = {},
+        events: dict[str, dict[str, Any]] = {},
     ):
-        self.address = address
+        self.host = host
         self.port = port
 
         self.registers = deepcopy(registers)
 
-        self.overrides = Simulator.OVERRIDES.copy()
-        self.overrides.update(overrides)
-
-        self.current_values: dict[str, list[int]] = {}
+        self.overrides = overrides
+        self.events = events
 
         self.context: ModbusServerContext | None = None
         self.slave_context: ModbusSlaveContext
@@ -58,10 +56,10 @@ def __init__(
         self.__task: asyncio.Task | None = None
 
     def reset(self):
-        di = {}
-        co = {}
-        hr = {}
-        ir = {}
+        di = {address: 0 for address in range(0, 1024)}
+        co = {address: 0 for address in range(0, 1024)}
+        hr = {address: 0 for address in range(0, 1024)}
+        ir = {address: 0 for address in range(0, 1024)}
 
         for register in self.registers:
             mode = self.registers[register].get("mode", "coil")
@@ -79,13 +77,6 @@ def reset(self):
             else:
                 raise ValueError(f"Invalid mode {mode!r} for register {register!r}.")
 
-            code = 1 if mode == "coil" else 3
-            self.current_values[register.lower()] = [
-                address,
-                code,
-                value,
-            ]
-
         self.slave_context = ModbusSlaveContext(
             di=ModbusSparseDataBlock(di),
             co=ModbusSparseDataBlock(co),
@@ -102,69 +93,97 @@ async def start(self, monitor_interval: float = 0.01):
 
         self.__task = asyncio.create_task(self._monitor_context(monitor_interval))
 
-        await StartAsyncTcpServer(
-            self.context,
-            address=(self.address, self.port),
-        )
+        await StartAsyncTcpServer(self.context, address=(self.host, self.port))
 
     async def stop(self):
         """Stops the simulator."""
 
         await ServerAsyncStop()
 
-        if self.__task:
-            self.__task.cancel()
-            with suppress(asyncio.CancelledError):
-                await self.__task
-
-        self.__task = None
+        self.__task = await cancel_task(self.__task)
 
     def __del__(self):
         if self.__task:
             self.__task.cancel()
 
-    async def _monitor_context(self, interval: float):
-        """Monitor the context."""
+    def get_register_data(self, register: str):
+        """Returns the data for a register."""
+
+        register_data = self.registers[register]
+
+        address = register_data["address"]
+        mode = register_data["mode"]
 
-        async def set_value(register: str, new_value: int, delay: float = 0):
-            if delay > 0:
-                await asyncio.sleep(delay)
+        if mode == "coil":
+            code = 1
+        elif mode == "holding_register":
+            code = 3
+        else:
+            raise ValueError(f"Invalid mode {mode!r} for register {register!r}.")
 
-            address, code, current_value = self.current_values[register]
+        return {
+            "name": register,
+            "address": address,
+            "mode": mode,
+            "code": code,
+            "value": int(self.slave_context.getValues(code, address, 1)[0]),
+        }
 
-            context.setValues(code, address, [new_value])
-            self.current_values[register][2] = new_value
+    async def _monitor_context(self, interval: float):
+        """Monitor the context."""
 
         assert self.context
         context = cast(ModbusSlaveContext, self.context[0])
 
         while True:
-            for register in self.current_values:
-                address, code, current_value = self.current_values[register]
-                new_value = int(context.getValues(code, address, count=1)[0])
-
-                if new_value == current_value:
+            for trigger_register, event_data in self.events.items():
+                on_value: int | bool | None = event_data.get("on_value", None)
+                if on_value is None:
                     continue
 
-                self.current_values[register][2] = new_value
+                trigger_data = self.get_register_data(trigger_register)
 
-                if register.endswith("_new"):
-                    # For lights. When we change the value of the XX_new
-                    # register the light is switched and XX_status changes value.
-                    status_name = register.replace("_new", "_status")
-                    asyncio.create_task(set_value(status_name, new_value, 0.0))
+                if trigger_data["value"] != on_value:
+                    continue
 
-                elif register == "e_stop":
-                    if new_value == 1:
-                        asyncio.create_task(set_value("e_status", 1, 0.0))
-                        asyncio.create_task(set_value("e_reset", 0, 0.0))
+                then = event_data["then"]
+
+                then_register_data = self.get_register_data(then["register"])
+                if then["action"] == "toggle":
+                    context.setValues(
+                        then_register_data["code"],
+                        then_register_data["address"],
+                        [not then_register_data["value"]],
+                    )
+                elif then["action"] == "set":
+                    context.setValues(
+                        then_register_data["code"],
+                        then_register_data["address"],
+                        [1],
+                    )
+                elif then["action"] == "reset":
+                    context.setValues(
+                        then_register_data["code"],
+                        then_register_data["address"],
+                        [0],
+                    )
+                else:
+                    continue
 
-                elif register == "e_reset":
-                    if new_value == 1:
-                        asyncio.create_task(set_value("e_status", 0, 0.0))
-                        asyncio.create_task(set_value("e_stop", 0, 0.0))
+                if then.get("reset_trigger", True):
+                    context.setValues(
+                        trigger_data["code"],
+                        trigger_data["address"],
+                        [0],
+                    )
 
             await asyncio.sleep(interval)
 
 
-plc_simulator = Simulator(config["modbus"]["registers"])
+plc_simulator = Simulator(
+    config["modbus"]["registers"],
+    host=config["simulator"]["host"],
+    port=config["simulator"]["port"],
+    overrides=config["simulator"]["overrides"],
+    events=config["simulator"]["events"],
+)
diff --git a/tests/conftest.py b/tests/conftest.py
index cc09be9..4ffc2fa 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -51,7 +51,7 @@ async def actor(simulator: Simulator, mocker):
 
     _actor = ECPActor.from_config(ecp_config)
 
-    mocker.patch.object(_actor.plc.hvac.modbus, "get_all", return_value={})
+    mocker.patch.object(_actor.plc.hvac.modbus, "read_all", return_value={})
 
     _actor = await setup_test_actor(_actor)  # type: ignore
     _actor.connection.connection = mocker.MagicMock(spec={"is_closed": False})
diff --git a/tests/test_command_heartbeat.py b/tests/test_command_heartbeat.py
index 43e1ea5..2626724 100644
--- a/tests/test_command_heartbeat.py
+++ b/tests/test_command_heartbeat.py
@@ -18,7 +18,7 @@
 
 
 async def test_command_heartbeat(actor: ECPActor, mocker: MockerFixture):
-    hb_set_mock = mocker.patch.object(actor.plc.modbus["hb_set"], "set")
+    hb_set_mock = mocker.patch.object(actor.plc.modbus["hb_set"], "write")
 
     cmd = await actor.invoke_mock_command("heartbeat")
     await cmd
@@ -29,7 +29,7 @@ async def test_command_heartbeat(actor: ECPActor, mocker: MockerFixture):
 
 
 async def test_command_heartbeat_fails(actor: ECPActor, mocker: MockerFixture):
-    mocker.patch.object(actor.plc.modbus["hb_set"], "set", side_effect=Exception)
+    mocker.patch.object(actor.plc.modbus["hb_set"], "write", side_effect=Exception)
 
     cmd = await actor.invoke_mock_command("heartbeat")
     await cmd

From 488b4412f7b4228740c0ea1fa6ad6d36e1b62adb Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sat, 28 Dec 2024 22:44:21 -0800
Subject: [PATCH 07/12] Improve test coverage and fix some issues

---
 python/lvmecp/actor/commands/dome.py   | 15 +++--
 python/lvmecp/actor/commands/modbus.py | 50 +++++++++++----
 python/lvmecp/dome.py                  |  4 +-
 python/lvmecp/etc/lvmecp.yml           |  8 ++-
 python/lvmecp/etc/schema.json          |  8 +--
 python/lvmecp/modbus.py                |  1 +
 python/lvmecp/module.py                |  1 +
 tests/conftest.py                      | 10 +++
 tests/test_command_dome.py             | 67 ++++++++++++++++++++
 tests/test_command_modbus.py           | 88 ++++++++++++++++++++++++++
 10 files changed, 225 insertions(+), 27 deletions(-)
 create mode 100644 tests/test_command_modbus.py

diff --git a/python/lvmecp/actor/commands/dome.py b/python/lvmecp/actor/commands/dome.py
index 63242d3..d036aca 100644
--- a/python/lvmecp/actor/commands/dome.py
+++ b/python/lvmecp/actor/commands/dome.py
@@ -74,7 +74,12 @@ async def close(command: ECPCommand, force=False):
 async def status(command: ECPCommand):
     """Returns the status of the dome."""
 
-    status = await command.actor.plc.dome.update(use_cache=False)
+    status = await command.actor.plc.dome.update(
+        use_cache=False,
+        force_output=True,
+        command=command,
+    )
+
     if status is None:
         return command.fail("Failed retrieving dome status.")
 
@@ -84,7 +89,7 @@ async def status(command: ECPCommand):
     if status & DomeStatus.POSITION_UNKNOWN:
         command.warning("Dome position is unknown!!!")
 
-    return command.finish(dome_open=bool(status & DomeStatus.OPEN))
+    return command.finish()
 
 
 @dome.command()
@@ -101,9 +106,7 @@ async def stop(command: ECPCommand):
 async def reset(command: ECPCommand, force=False):
     """Resets dome error state."""
 
-    try:
-        await command.actor.plc.dome.reset()
-    except DomeError as err:
-        return command.fail(err)
+    command.warning("Resetting dome error state.")
+    await command.actor.plc.dome.reset()
 
     return command.finish()
diff --git a/python/lvmecp/actor/commands/modbus.py b/python/lvmecp/actor/commands/modbus.py
index eea8cd4..dc45fc1 100644
--- a/python/lvmecp/actor/commands/modbus.py
+++ b/python/lvmecp/actor/commands/modbus.py
@@ -22,7 +22,7 @@
     from lvmecp.modbus import ModbusRegister
 
 
-def get_register(command: ECPCommand, address_or_name: str):
+def get_register(command: ECPCommand, address_or_name: str, register_type: str | None):
     """Returns a register from an address or name."""
 
     plc = command.actor.plc
@@ -30,6 +30,11 @@ def get_register(command: ECPCommand, address_or_name: str):
     register: ModbusRegister | None = None
     try:
         address = int(address_or_name)  # type: ignore
+
+        if isinstance(address, int) and not register_type:
+            command.fail("When passing an address, --register-type must be specified.")
+            return False
+
         for _, reg in plc.modbus.items():
             if reg.address == address:
                 register = reg
@@ -38,7 +43,7 @@ def get_register(command: ECPCommand, address_or_name: str):
         register = plc.modbus.get(address_or_name)
 
     if register is None:
-        command.fail(f"Register {address!r} not found.")
+        command.fail(f"Register {address_or_name!r} not found.")
         return False
 
     return register
@@ -53,10 +58,16 @@ def modbus():
 
 @modbus.command()
 @click.argument("address", metavar="ADDRESS|NAME")
-async def read(command: ECPCommand, address: str):
+@click.option(
+    "--register-type",
+    type=click.Choice(["coil", "holding_register"]),
+    default=None,
+    help="The type of register to read. Required if an address is passed.",
+)
+async def read(command: ECPCommand, address: str, register_type: str | None = None):
     """Reads a Modbus register."""
 
-    if not (register := get_register(command, address)):
+    if not (register := get_register(command, address, register_type)):
         return False
 
     value = await register.read(use_cache=False)
@@ -73,10 +84,21 @@ async def read(command: ECPCommand, address: str):
 @modbus.command()
 @click.argument("address", metavar="ADDRESS|NAME")
 @click.argument("value", type=int)
-async def write(command: ECPCommand, address: str, value: int):
+@click.option(
+    "--register-type",
+    type=click.Choice(["coil", "holding_register"]),
+    default=None,
+    help="The type of register to read. Required if an address is passed.",
+)
+async def write(
+    command: ECPCommand,
+    address: str,
+    value: int,
+    register_type: str | None = None,
+):
     """Writes a value to a Modbus register."""
 
-    if not (register := get_register(command, address)):
+    if not (register := get_register(command, address, register_type)):
         return False
 
     name = register.name
@@ -85,14 +107,14 @@ async def write(command: ECPCommand, address: str, value: int):
         return command.fail(f"Register {name!r} is read-only.")
 
     if register.mode == "coil":
-        try:
-            value = bool(int(value))
-        except Exception:
-            return command.fail(f"Invalid value for coil register {name!r}: {value!r}")
-        else:
-            value = int(value)
-
-    await register.write(value)
+        value = bool(int(value))
+    else:
+        value = int(value)
+
+    try:
+        await register.write(value)
+    except Exception as err:
+        return command.fail(f"Error writing to register {name!r}: {err!r}")
 
     await asyncio.sleep(0.5)
     new_value = await register.read(use_cache=False)
diff --git a/python/lvmecp/dome.py b/python/lvmecp/dome.py
index bf95e10..e87c4d8 100644
--- a/python/lvmecp/dome.py
+++ b/python/lvmecp/dome.py
@@ -189,8 +189,8 @@ async def stop(self):
     async def reset(self):
         """Resets the roll-off error state."""
 
-        await self.modbus["rolloff_error_reset"].write(True)
-        await asyncio.sleep(1)
+        await self.modbus["dome_error_reset"].write(True)
+        await asyncio.sleep(0.5)
 
     def is_allowed(self):
         """Returns whether the dome is allowed to move."""
diff --git a/python/lvmecp/etc/lvmecp.yml b/python/lvmecp/etc/lvmecp.yml
index 9d6529c..3fa4c78 100644
--- a/python/lvmecp/etc/lvmecp.yml
+++ b/python/lvmecp/etc/lvmecp.yml
@@ -498,6 +498,12 @@ simulator:
         register: e_status
         action: reset
         reset_trigger: true
+    dome_error_reset:
+      on_value: 1
+      then:
+        register: dome_error
+        action: reset
+        reset_trigger: true
 
 engineering_mode:
   default_duration: 300
@@ -512,7 +518,7 @@ dome:
   full_open_mm: 9480
 
 actor:
-  name: lvmecp-dev
+  name: lvmecp
   host: localhost
   port: 5672
   log_dir: /data/logs/lvmecp
diff --git a/python/lvmecp/etc/schema.json b/python/lvmecp/etc/schema.json
index bc1d288..76d4af8 100644
--- a/python/lvmecp/etc/schema.json
+++ b/python/lvmecp/etc/schema.json
@@ -18,9 +18,9 @@
         }
       }
     },
-    "lights": {
-      "type": "string"
-    },
+    "lights": { "type": "string" },
+    "lights_labels": { "type": "string" },
+    "dome_percent_open": { "type": "number" },
     "o2_percent_utilities": { "type": "number" },
     "o2_percent_spectrograph": { "type": "number" },
     "heartbeat_ack": {
@@ -57,5 +57,5 @@
       "required": ["enabled", "started_at", "ends_at"]
     }
   },
-  "additionalProperties": true
+  "additionalProperties": false
 }
diff --git a/python/lvmecp/modbus.py b/python/lvmecp/modbus.py
index f285440..a1e5401 100644
--- a/python/lvmecp/modbus.py
+++ b/python/lvmecp/modbus.py
@@ -418,6 +418,7 @@ async def write_register(self, register: str | int, value: int | bool):
         """Writes a value to a register."""
 
         if isinstance(register, int):
+            found: bool = False
             for name, reg in self.items():
                 if reg.address == register:
                     register = name
diff --git a/python/lvmecp/module.py b/python/lvmecp/module.py
index 4408f8c..3bab9fe 100644
--- a/python/lvmecp/module.py
+++ b/python/lvmecp/module.py
@@ -114,6 +114,7 @@ async def update(
         except Exception as err:
             log.warning(f"{self.name}: failed updating status: {err}")
             new_status = self.flag(self.flag.__unknown__) if self.flag else None
+            extra_info = {}
 
         # Only notify if the status has changed.
         if (new_status != self.status and not extra_info) or force_output:
diff --git a/tests/conftest.py b/tests/conftest.py
index 4ffc2fa..01a9d41 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -13,7 +13,10 @@
 from contextlib import suppress
 from copy import deepcopy
 
+from typing import cast
+
 import pytest
+from pymodbus.datastore import ModbusSlaveContext
 
 from clu.testing import setup_test_actor
 
@@ -37,6 +40,13 @@ async def simulator():
         await simulator_task
 
 
+@pytest.fixture()
+def context(simulator: Simulator) -> ModbusSlaveContext:
+    assert simulator.context
+
+    return cast(ModbusSlaveContext, simulator.context[0])
+
+
 @pytest.fixture()
 async def actor(simulator: Simulator, mocker):
     ecp_config = deepcopy(config)
diff --git a/tests/test_command_dome.py b/tests/test_command_dome.py
index 7d74211..da22210 100644
--- a/tests/test_command_dome.py
+++ b/tests/test_command_dome.py
@@ -21,11 +21,78 @@
 
 
 if TYPE_CHECKING:
+    from pymodbus.datastore import ModbusSlaveContext
     from pytest_mock import MockerFixture
 
     from lvmecp.actor import ECPActor
 
 
+@pytest.mark.parametrize("open", [True, False])
+async def test_command_dome_status(
+    context: ModbusSlaveContext, actor: ECPActor, open: bool
+):
+    if open:
+        address = actor.plc.modbus["dome_open"].address
+    else:
+        address = actor.plc.modbus["dome_closed"].address
+
+    context.setValues(1, address, [1])
+
+    cmd = await actor.invoke_mock_command("dome status")
+    await cmd
+
+    assert cmd.status.did_succeed
+
+
+async def test_command_dome_moving(context: ModbusSlaveContext, actor: ECPActor):
+    address = actor.plc.modbus["drive_enabled"].address
+
+    context.setValues(1, address, [1])
+
+    cmd = await actor.invoke_mock_command("dome status")
+    await cmd
+
+    assert cmd.status.did_succeed
+    text = cmd.replies.get("text")
+    assert text == "Dome is moving!!!"
+
+
+async def test_command_dome_position_unknown(
+    context: ModbusSlaveContext,
+    actor: ECPActor,
+):
+    context.setValues(1, actor.plc.modbus["dome_closed"].address, [0])
+
+    cmd = await actor.invoke_mock_command("dome status")
+    await cmd
+
+    assert cmd.status.did_succeed
+    text = cmd.replies.get("text")
+    assert text == "Dome position is unknown!!!"
+
+
+async def test_command_dome_stop(actor: ECPActor):
+    await actor.plc.modbus["drive_enabled"].write(1)
+
+    cmd = await actor.invoke_mock_command("dome stop")
+    await cmd
+
+    assert cmd.status.did_succeed
+
+    assert (await actor.plc.modbus["drive_enabled"].read(use_cache=False)) == 0
+
+
+async def test_command_dome_reset(context: ModbusSlaveContext, actor: ECPActor):
+    context.setValues(1, actor.plc.modbus["dome_error"].address, [1])
+
+    cmd = await actor.invoke_mock_command("dome reset")
+    await cmd
+
+    assert cmd.status.did_succeed
+
+    assert (await actor.plc.modbus["dome_error"].read(use_cache=False)) == 0
+
+
 async def test_command_dome_open(actor: ECPActor, mocker: MockerFixture):
     mocker.patch.object(actor.plc.dome, "is_daytime", return_value=False)
     mocker.patch.object(actor.plc.dome, "_move", return_value=True)
diff --git a/tests/test_command_modbus.py b/tests/test_command_modbus.py
new file mode 100644
index 0000000..9dc0b25
--- /dev/null
+++ b/tests/test_command_modbus.py
@@ -0,0 +1,88 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+#
+# @Author: José Sánchez-Gallego (gallegoj@uw.edu)
+# @Date: 2024-12-28
+# @Filename: test_command_modbus.py
+# @License: BSD 3-clause (http://www.opensource.org/licenses/BSD-3-Clause)
+
+from __future__ import annotations
+
+from typing import TYPE_CHECKING
+
+from pytest_mock import MockerFixture
+
+
+if TYPE_CHECKING:
+    from lvmecp.actor import ECPActor
+
+
+async def test_command_modbus_read(actor: ECPActor):
+    read_cmd = await actor.invoke_mock_command("modbus read door_locked")
+    await read_cmd
+
+    assert read_cmd.status.did_succeed
+    assert read_cmd.replies.get("register")["value"]
+
+
+async def test_command_modbus_read_address(actor: ECPActor):
+    read_cmd = await actor.invoke_mock_command("modbus read --register-type coil 1")
+    await read_cmd
+
+    assert read_cmd.status.did_succeed
+
+    assert read_cmd.replies.get("register")["name"] == "door_closed"
+    assert read_cmd.replies.get("register")["value"]
+
+
+async def test_command_modbus_read_address_no_register_type(actor: ECPActor):
+    read_cmd = await actor.invoke_mock_command("modbus read 1")
+    await read_cmd
+
+    assert read_cmd.status.did_fail
+
+    assert "--register-type must be specified" in read_cmd.replies.get("error")
+
+
+async def test_command_modbus_read_bad_register(actor: ECPActor):
+    read_cmd = await actor.invoke_mock_command("modbus read bad_register")
+    await read_cmd
+
+    assert read_cmd.status.did_fail
+
+    assert "not found" in read_cmd.replies.get("error")
+
+
+async def test_modbus_write_register(actor: ECPActor):
+    pre_value = await actor.plc.modbus["motor_direction"].read(use_cache=False)
+    assert pre_value == 0
+
+    write_cmd = await actor.invoke_mock_command("modbus write motor_direction 1")
+    await write_cmd
+
+    assert write_cmd.status.did_succeed
+
+    new_value = await actor.plc.modbus["motor_direction"].read(use_cache=False)
+    assert new_value == 1
+
+
+async def test_modbus_write_register_readonly(actor: ECPActor):
+    write_cmd = await actor.invoke_mock_command("modbus write door_locked 1")
+    await write_cmd
+
+    assert write_cmd.status.did_fail
+    assert "is read-only" in write_cmd.replies.get("error")
+
+
+async def test_modbus_write_register_fails(actor: ECPActor, mocker: MockerFixture):
+    mocker.patch.object(
+        actor.plc.modbus["motor_direction"],
+        "write",
+        side_effect=ValueError("cannot write"),
+    )
+
+    write_cmd = await actor.invoke_mock_command("modbus write motor_direction 1")
+    await write_cmd
+
+    assert write_cmd.status.did_fail
+    assert "cannot write" in write_cmd.replies.get("error")

From de4552873c3c9baae7535b0edd17f908b45b02f0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sat, 28 Dec 2024 22:57:20 -0800
Subject: [PATCH 08/12] Allow reading/writing to unknown registers

---
 python/lvmecp/actor/commands/modbus.py | 64 +++++++++++++++++++++++---
 python/lvmecp/modbus.py                |  2 +-
 tests/test_command_modbus.py           | 17 +++++++
 3 files changed, 75 insertions(+), 8 deletions(-)

diff --git a/python/lvmecp/actor/commands/modbus.py b/python/lvmecp/actor/commands/modbus.py
index dc45fc1..abbb631 100644
--- a/python/lvmecp/actor/commands/modbus.py
+++ b/python/lvmecp/actor/commands/modbus.py
@@ -10,24 +10,33 @@
 
 import asyncio
 
-from typing import TYPE_CHECKING
+from typing import TYPE_CHECKING, Literal
 
 import click
 
+from lvmecp.modbus import ModbusRegister
+
 from . import parser
 
 
 if TYPE_CHECKING:
     from lvmecp.actor import ECPCommand
-    from lvmecp.modbus import ModbusRegister
+    from lvmecp.modbus import RegisterModes
 
 
-def get_register(command: ECPCommand, address_or_name: str, register_type: str | None):
+def get_register(
+    command: ECPCommand,
+    address_or_name: str,
+    register_type: RegisterModes | None = None,
+    allow_unknown: bool = False,
+) -> ModbusRegister | Literal[False]:
     """Returns a register from an address or name."""
 
     plc = command.actor.plc
 
     register: ModbusRegister | None = None
+    address: int | None = None
+
     try:
         address = int(address_or_name)  # type: ignore
 
@@ -41,8 +50,19 @@ def get_register(command: ECPCommand, address_or_name: str, register_type: str |
                 break
     except ValueError:
         register = plc.modbus.get(address_or_name)
+        address = register.address if register else None
 
     if register is None:
+        if allow_unknown and address is not None and register_type:
+            return ModbusRegister(
+                command.actor.plc.modbus,
+                name=f"{register_type}_{address}",
+                address=address,
+                mode=register_type,
+                count=1,
+                readonly=False,
+            )
+
         command.fail(f"Register {address_or_name!r} not found.")
         return False
 
@@ -64,10 +84,27 @@ def modbus():
     default=None,
     help="The type of register to read. Required if an address is passed.",
 )
-async def read(command: ECPCommand, address: str, register_type: str | None = None):
+@click.option(
+    "--allow-unknown",
+    is_flag=True,
+    help="Allow unknown registers. Requires specifying an address.",
+)
+async def read(
+    command: ECPCommand,
+    address: str,
+    register_type: Literal["coil", "holding_register"] | None = None,
+    allow_unknown: bool = False,
+):
     """Reads a Modbus register."""
 
-    if not (register := get_register(command, address, register_type)):
+    if not (
+        register := get_register(
+            command,
+            address,
+            register_type=register_type,
+            allow_unknown=allow_unknown,
+        )
+    ):
         return False
 
     value = await register.read(use_cache=False)
@@ -90,15 +127,28 @@ async def read(command: ECPCommand, address: str, register_type: str | None = No
     default=None,
     help="The type of register to read. Required if an address is passed.",
 )
+@click.option(
+    "--allow-unknown",
+    is_flag=True,
+    help="Allow unknown registers. Requires specifying an address.",
+)
 async def write(
     command: ECPCommand,
     address: str,
     value: int,
-    register_type: str | None = None,
+    register_type: Literal["coil", "holding_register"] | None = None,
+    allow_unknown: bool = False,
 ):
     """Writes a value to a Modbus register."""
 
-    if not (register := get_register(command, address, register_type)):
+    if not (
+        register := get_register(
+            command,
+            address,
+            register_type=register_type,
+            allow_unknown=allow_unknown,
+        )
+    ):
         return False
 
     name = register.name
diff --git a/python/lvmecp/modbus.py b/python/lvmecp/modbus.py
index a1e5401..89094dd 100644
--- a/python/lvmecp/modbus.py
+++ b/python/lvmecp/modbus.py
@@ -33,7 +33,7 @@
 CONNECTION_TIMEOUT = 10.0
 
 
-RegisterModes = Literal["coil", "holding_regiser", "discrete_input", "input_register"]
+RegisterModes = Literal["coil", "holding_register", "discrete_input", "input_register"]
 
 
 class ModbusRegister:
diff --git a/tests/test_command_modbus.py b/tests/test_command_modbus.py
index 9dc0b25..824d052 100644
--- a/tests/test_command_modbus.py
+++ b/tests/test_command_modbus.py
@@ -14,6 +14,8 @@
 
 
 if TYPE_CHECKING:
+    from pymodbus.datastore import ModbusSlaveContext
+
     from lvmecp.actor import ECPActor
 
 
@@ -86,3 +88,18 @@ async def test_modbus_write_register_fails(actor: ECPActor, mocker: MockerFixtur
 
     assert write_cmd.status.did_fail
     assert "cannot write" in write_cmd.replies.get("error")
+
+
+async def test_modbus_write_unknown_register(
+    context: ModbusSlaveContext,
+    actor: ECPActor,
+):
+    cmd = await actor.invoke_mock_command(
+        "modbus write --register-type coil --allow-unknown 999 1"
+    )
+    await cmd
+
+    assert cmd.status.did_succeed
+
+    register = cmd.replies.get("register")
+    assert register == {"name": "coil_999", "address": 999, "value": True}

From f888b59b429132150e506b5dca652ae687c3c9ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sat, 28 Dec 2024 22:59:51 -0800
Subject: [PATCH 09/12] Fix linting

---
 python/lvmecp/actor/commands/engineering.py | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/python/lvmecp/actor/commands/engineering.py b/python/lvmecp/actor/commands/engineering.py
index e598c9d..4d321b0 100644
--- a/python/lvmecp/actor/commands/engineering.py
+++ b/python/lvmecp/actor/commands/engineering.py
@@ -34,12 +34,13 @@ async def get_eng_mode_status(actor: ECPActor) -> dict:
         ends_at = started_at + duration
 
     return {
-            "enabled": enabled,
-            "started_at": timestamp_to_iso(started_at),
-            "ends_at": timestamp_to_iso(ends_at),
-            "software_override": registers["engineering_mode_software"],
-            "hardware_override": registers["engineering_mode_hardware"],
-        }
+        "enabled": enabled,
+        "started_at": timestamp_to_iso(started_at),
+        "ends_at": timestamp_to_iso(ends_at),
+        "software_override": registers["engineering_mode_software"],
+        "hardware_override": registers["engineering_mode_hardware"],
+    }
+
 
 @parser.group(name="engineering-mode")
 def engineering_mode():

From 3d96e8894b982db711bdd3b9cdd522def439027f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sat, 28 Dec 2024 23:07:57 -0800
Subject: [PATCH 10/12] Update changelog

---
 CHANGELOG.md | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 9855518..c73ed55 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,5 +1,12 @@
 # Changelog
 
+## Next version
+
+### ✨ Improved
+
+* [#32](https://github.com/sdss/lvmecp/pull/32) Major refactor of the `Modbus` and `ModbusRegister` classes. The main change is that the performance has been greatly improved, with `Modbus.get_all()` going from taking ~0.6 seconds to under 0.1. The register and coil blocks are now read completely, in chunks as large as the device will accept, as opposed to before, when we would read each variable with one read command (although the connection was not closed in between). Note that several methods and variables have been renamed; see the PR for details.
+
+
 ## 1.0.2 - December 27, 2024
 
 ### ✨ Improved

From f61b5aa4fa50760de5dd0f03aec87f5015ffea06 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sun, 29 Dec 2024 07:29:00 -0800
Subject: [PATCH 11/12] Add test coverage for modbus.py

---
 python/lvmecp/modbus.py |  84 ++++++++++------------
 python/lvmecp/module.py |   3 +-
 tests/conftest.py       |  22 +++++-
 tests/test_modbus.py    | 153 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 212 insertions(+), 50 deletions(-)
 create mode 100644 tests/test_modbus.py

diff --git a/python/lvmecp/modbus.py b/python/lvmecp/modbus.py
index 89094dd..c2e0b63 100644
--- a/python/lvmecp/modbus.py
+++ b/python/lvmecp/modbus.py
@@ -101,7 +101,7 @@ async def _read_internal(self):
                 slave=self.modbus.slave,
             )
 
-        if resp.function_code > 0x80:
+        if resp.isError():
             raise ValueError(
                 f"Invalid response for element "
                 f"{self.name!r}: 0x{resp.function_code:02X}."
@@ -164,34 +164,26 @@ async def write(self, value: int | bool):
         if self.readonly:
             raise ECPError(f"Register {self.name!r} is read-only.")
 
+        if self.mode == "coil":
+            func = self.client.write_coil
+        elif self.mode == "holding_register":
+            func = self.client.write_register
+        elif self.mode == "discrete_input" or self.mode == "input_register":
+            raise ValueError(f"Block of mode {self.mode!r} is read-only.")
+        else:
+            raise ValueError(f"Invalid block mode {self.mode!r}.")
+
         # Always open the connection.
         async with self.modbus:
-            if self.mode == "coil":
-                func = self.client.write_coil
-            elif self.mode == "holding_register":
-                func = self.client.write_register
-            elif self.mode == "discrete_input" or self.mode == "input_register":
-                raise ValueError(f"Block of mode {self.mode!r} is read-only.")
-            else:
-                raise ValueError(f"Invalid block mode {self.mode!r}.")
-
-            try:
-                if self.client.connected:
-                    resp = await func(self.address, value)  # type: ignore
-                else:
-                    async with self.modbus:
-                        resp = await func(self.address, value)  # type: ignore
-
-                if resp.function_code > 0x80:
-                    raise ECPError(
-                        f"Invalid response for element "
-                        f"{self.name!r}: 0x{resp.function_code:02X}."
-                    )
-                else:
-                    self.modbus.register_cache[self.name] = value
+            resp = await func(self.address, value)  # type: ignore
 
-            except Exception as err:
-                raise ECPError(f"Failed setting {self.name!r}: {err}")
+            if resp.isError():
+                raise ECPError(
+                    f"Invalid response for element "
+                    f"{self.name!r}: 0x{resp.function_code:02X}."
+                )
+            else:
+                self.modbus.register_cache[self.name] = value
 
 
 class Modbus(dict[str, ModbusRegister]):
@@ -232,9 +224,9 @@ def __init__(self, config: dict | pathlib.Path | str | None = None):
         # Modbus client.
         self.client = AsyncModbusTcpClient(self.host, port=self.port)
 
-        # Semaphore to allow up to 5 concurrent connections.
-        self.semaphore = asyncio.Semaphore(1)
-        self._semaphore_release_task: asyncio.Task | None = None
+        # Lock to allow up to 5 concurrent connections.
+        self.lock = asyncio.Lock()
+        self._lock_release_task: asyncio.Task | None = None
 
         # Create the internal dictionary of registers
         registers = {
@@ -259,7 +251,7 @@ async def connect(self):
         """Connects to the client."""
 
         try:
-            await asyncio.wait_for(self.semaphore.acquire(), CONNECTION_TIMEOUT)
+            await asyncio.wait_for(self.lock.acquire(), CONNECTION_TIMEOUT)
         except asyncio.TimeoutError:
             raise RuntimeError("Timed out waiting for lock to be released.")
 
@@ -276,15 +268,15 @@ async def connect(self):
         except Exception as err:
             raise ConnectionError(f"Failed connecting to server at {hp}: {err}.")
         finally:
-            if not did_connect:
-                self.semaphore.release()
+            if not did_connect and self.lock.locked():
+                self.lock.release()
 
         log.debug(f"Connected to {hp}.")
 
         # Schedule a task to release the lock after 5 seconds. This is a safeguard
         # in case something fails and the connection is never closed and the lock
         # not released.
-        self._semaphore_release_task = asyncio.create_task(self.unlock_on_timeout())
+        self._lock_release_task = asyncio.create_task(self.unlock_on_timeout())
 
     async def disconnect(self):
         """Disconnects the client."""
@@ -295,9 +287,10 @@ async def disconnect(self):
                 log.debug(f"Disonnected from {self.host}:{self.port}.")
 
         finally:
-            self.semaphore.release()
+            if self.lock.locked():
+                self.lock.release()
 
-            await cancel_task(self._semaphore_release_task)
+            await cancel_task(self._lock_release_task)
 
     async def __aenter__(self):
         """Initialises the connection to the server."""
@@ -313,7 +306,7 @@ async def unlock_on_timeout(self):
         """Removes the lock after an amount of time."""
 
         await asyncio.sleep(CONNECTION_TIMEOUT)
-        self.semaphore.release()
+        await self.disconnect()
 
     async def read_all(self, use_cache: bool = True) -> dict[str, int | bool]:
         """Returns a dictionary with all the registers and sets the cache."""
@@ -414,19 +407,16 @@ async def read_group(self, group: str, use_cache: bool = True):
 
         return group_registers
 
-    async def write_register(self, register: str | int, value: int | bool):
-        """Writes a value to a register."""
+    async def read_register(self, register: str, use_cache: bool = True) -> int | bool:
+        """Reads a register."""
 
-        if isinstance(register, int):
-            found: bool = False
-            for name, reg in self.items():
-                if reg.address == register:
-                    register = name
-                    found = True
-                    break
+        if register not in self:
+            raise ValueError(f"Register {register!r} not found.")
 
-            if not found:
-                raise ValueError(f"Register with address {register!r} not found.")
+        return await self[register].read(use_cache=use_cache)
+
+    async def write_register(self, register: str, value: int | bool):
+        """Writes a value to a register."""
 
         assert isinstance(register, str)
 
diff --git a/python/lvmecp/module.py b/python/lvmecp/module.py
index 3bab9fe..0f3a82e 100644
--- a/python/lvmecp/module.py
+++ b/python/lvmecp/module.py
@@ -47,7 +47,7 @@ def __init__(
         modbus: Modbus | None = None,
         interval: float | None = None,
         start: bool = True,
-        notifier: Callable[[int, str], Callable | Coroutine] | None = None,
+        notifier: Callable[[int, str, dict], Callable | Coroutine] | None = None,
     ):
         self.name = name
         self.plc = plc
@@ -159,5 +159,6 @@ async def notify_status(
                 self.notifier,
                 status.value,
                 str(status),
+                extra_keywords,
                 **kwargs,
             )
diff --git a/tests/conftest.py b/tests/conftest.py
index 01a9d41..1b96171 100644
--- a/tests/conftest.py
+++ b/tests/conftest.py
@@ -16,6 +16,7 @@
 from typing import cast
 
 import pytest
+import pytest_mock
 from pymodbus.datastore import ModbusSlaveContext
 
 from clu.testing import setup_test_actor
@@ -23,6 +24,7 @@
 import lvmecp
 from lvmecp import config
 from lvmecp.actor import ECPActor
+from lvmecp.modbus import Modbus
 from lvmecp.simulator import Simulator, plc_simulator
 
 
@@ -48,7 +50,7 @@ def context(simulator: Simulator) -> ModbusSlaveContext:
 
 
 @pytest.fixture()
-async def actor(simulator: Simulator, mocker):
+def test_config():
     ecp_config = deepcopy(config)
 
     del ecp_config["actor"]["log_dir"]
@@ -59,7 +61,23 @@ async def actor(simulator: Simulator, mocker):
     schema_path = ecp_config["actor"]["schema"]
     ecp_config["actor"]["schema"] = os.path.dirname(lvmecp.__file__) + "/" + schema_path
 
-    _actor = ECPActor.from_config(ecp_config)
+    yield ecp_config
+
+
+@pytest.fixture()
+async def modbus(simulator: Simulator, test_config: dict):
+    _modbus = Modbus(test_config["modbus"])
+
+    yield _modbus
+
+
+@pytest.fixture()
+async def actor(
+    simulator: Simulator,
+    mocker: pytest_mock.MockerFixture,
+    test_config: dict,
+):
+    _actor = ECPActor.from_config(test_config)
 
     mocker.patch.object(_actor.plc.hvac.modbus, "read_all", return_value={})
 
diff --git a/tests/test_modbus.py b/tests/test_modbus.py
new file mode 100644
index 0000000..e093a08
--- /dev/null
+++ b/tests/test_modbus.py
@@ -0,0 +1,153 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+#
+# @Author: José Sánchez-Gallego (gallegoj@uw.edu)
+# @Date: 2024-12-29
+# @Filename: test_modbus.py
+# @License: BSD 3-clause (http://www.opensource.org/licenses/BSD-3-Clause)
+
+from __future__ import annotations
+
+import asyncio
+
+from typing import TYPE_CHECKING, cast
+
+import pytest
+from pytest_mock import MockerFixture
+
+import lvmecp.modbus
+from lvmecp.modbus import ModbusRegister, RegisterModes
+
+
+if TYPE_CHECKING:
+    from pymodbus.datastore import ModbusSlaveContext
+
+    from lvmecp.modbus import Modbus
+
+
+async def test_modbus_read(modbus: Modbus):
+    resp = await modbus.read_register("door_locked")
+    assert resp == 1
+
+
+@pytest.mark.parametrize(
+    "register_type",
+    ["coil", "discrete_input", "input_register", "holding_register"],
+)
+async def test_modbus_register_read(
+    context: ModbusSlaveContext,
+    modbus: Modbus,
+    register_type: str,
+):
+    is_discrete = register_type in ["coil", "discrete_input"]
+
+    write_func_code = 1
+    if register_type == "discrete_input":
+        write_func_code = 2
+    elif register_type == "holding_register":
+        write_func_code = 3
+    elif register_type == "input_register":
+        write_func_code = 4
+
+    context.setValues(write_func_code, 99, [1 if is_discrete else 101])
+
+    register = ModbusRegister(
+        modbus,
+        name="test_register",
+        address=99,
+        mode=cast(RegisterModes, register_type),
+        count=1,
+        readonly=True,
+    )
+
+    resp = await register.read(use_cache=False)
+    assert resp == (1 if is_discrete else 101)
+
+
+async def test_modbus_register_read_decoder_float_32bit(
+    context: ModbusSlaveContext,
+    modbus: Modbus,
+):
+    context.setValues(3, 99, [4000, 16000])
+
+    register = ModbusRegister(
+        modbus,
+        name="test_register",
+        address=99,
+        mode="holding_register",
+        count=2,
+        readonly=True,
+        decoder="float_32bit",
+    )
+
+    resp = await register.read(use_cache=False)
+
+    assert resp == 0.250
+
+
+@pytest.mark.parametrize("register_type", ["coil", "holding_register"])
+async def test_modbus_register_write(
+    context: ModbusSlaveContext,
+    modbus: Modbus,
+    register_type: str,
+):
+    is_discrete = register_type in ["coil", "discrete_input"]
+    write_func_code = 1 if is_discrete else 3
+
+    register = ModbusRegister(
+        modbus,
+        name="test_register",
+        address=99,
+        mode=cast(RegisterModes, register_type),
+        count=1,
+        readonly=False,
+    )
+
+    await register.write(1 if is_discrete else 101)
+
+    value = context.getValues(write_func_code, 99)
+    assert value[0] == (1 if is_discrete else 101)
+
+
+@pytest.mark.parametrize("register_type", ["discrete_input", "input_register"])
+async def test_modbus_register_write_on_readonly(modbus: Modbus, register_type: str):
+    register = ModbusRegister(
+        modbus,
+        name="test_register",
+        address=99,
+        mode=cast(RegisterModes, register_type),
+        count=1,
+        readonly=False,
+    )
+
+    with pytest.raises(ValueError) as err:
+        await register.write(1)
+
+    assert "is read-only" in str(err.value)
+
+
+async def test_modbus_connection_fails(modbus: Modbus, mocker: MockerFixture):
+    mocker.patch.object(modbus.client, "connect", side_effect=ConnectionError)
+
+    with pytest.raises(ConnectionError):
+        await modbus.read_register("door_locked")
+
+
+async def test_modbus_connection_timeouts(modbus: Modbus, mocker: MockerFixture):
+    mocker.patch.object(modbus.client, "connect", side_effect=asyncio.TimeoutError)
+
+    with pytest.raises(ConnectionError):
+        await modbus.read_register("door_locked")
+
+
+async def test_modbus_lock_release(modbus: Modbus, mocker: MockerFixture):
+    mocker.patch.object(lvmecp.modbus, "CONNECTION_TIMEOUT", 0.1)
+
+    async with modbus:
+        assert modbus.client.connected
+        assert modbus.lock.locked()
+
+        await asyncio.sleep(0.2)
+
+        assert not modbus.client.connected
+        assert not modbus.lock.locked()

From a2e6fd1dca727f6a4d453418f062483c07cc12c0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jos=C3=A9=20S=C3=A1nchez-Gallego?= <gallegoj@uw.edu>
Date: Sun, 29 Dec 2024 07:39:05 -0800
Subject: [PATCH 12/12] Add more realistic test for dome opening

---
 python/lvmecp/dome.py      |  9 ++++++---
 tests/test_command_dome.py | 35 +++++++++++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/python/lvmecp/dome.py b/python/lvmecp/dome.py
index e87c4d8..894248c 100644
--- a/python/lvmecp/dome.py
+++ b/python/lvmecp/dome.py
@@ -23,6 +23,9 @@
 from lvmecp.module import PLCModule
 
 
+MOVE_CHECK_INTERVAL: float = 0.5
+
+
 class DomeController(PLCModule[DomeStatus]):
     """Controller for the rolling dome."""
 
@@ -125,17 +128,17 @@ async def _move(self, open: bool, force: bool = False):
         log.debug("Setting motor_direction.")
         await self.modbus["motor_direction"].write(open)
 
-        await asyncio.sleep(0.5)
+        await asyncio.sleep(0.1)
 
         log.debug("Setting drive_enabled.")
         await self.modbus["drive_enabled"].write(True)
 
-        await asyncio.sleep(0.5)
+        await asyncio.sleep(0.1)
 
         last_enabled: float = 0.0
         while True:
             # Still moving.
-            await asyncio.sleep(2)
+            await asyncio.sleep(MOVE_CHECK_INTERVAL)
 
             drive_enabled = await self.modbus["drive_enabled"].read(use_cache=False)
 
diff --git a/tests/test_command_dome.py b/tests/test_command_dome.py
index da22210..6639a60 100644
--- a/tests/test_command_dome.py
+++ b/tests/test_command_dome.py
@@ -29,7 +29,9 @@
 
 @pytest.mark.parametrize("open", [True, False])
 async def test_command_dome_status(
-    context: ModbusSlaveContext, actor: ECPActor, open: bool
+    context: ModbusSlaveContext,
+    actor: ECPActor,
+    open: bool,
 ):
     if open:
         address = actor.plc.modbus["dome_open"].address
@@ -93,7 +95,7 @@ async def test_command_dome_reset(context: ModbusSlaveContext, actor: ECPActor):
     assert (await actor.plc.modbus["dome_error"].read(use_cache=False)) == 0
 
 
-async def test_command_dome_open(actor: ECPActor, mocker: MockerFixture):
+async def test_command_dome_open_mock(actor: ECPActor, mocker: MockerFixture):
     mocker.patch.object(actor.plc.dome, "is_daytime", return_value=False)
     mocker.patch.object(actor.plc.dome, "_move", return_value=True)
 
@@ -105,6 +107,35 @@ async def test_command_dome_open(actor: ECPActor, mocker: MockerFixture):
     assert cmd.status.did_succeed
 
 
+async def test_command_dome_open(
+    actor: ECPActor,
+    context: ModbusSlaveContext,
+    mocker: MockerFixture,
+):
+    async def open_with_delay():
+        context.setValues(1, actor.plc.modbus["dome_closed"].address, [0])
+
+        await asyncio.sleep(0.3)
+
+        context.setValues(1, actor.plc.modbus["dome_open"].address, [1])
+        context.setValues(1, actor.plc.modbus["drive_enabled"].address, [0])
+
+    mocker.patch.object(actor.plc.dome, "is_daytime", return_value=False)
+    mocker.patch.object(lvmecp.dome, "MOVE_CHECK_INTERVAL", 0.1)
+
+    cmd = await actor.invoke_mock_command("dome open")
+
+    await asyncio.sleep(0.1)
+    asyncio.create_task(open_with_delay())
+
+    await cmd
+
+    assert cmd.status.did_succeed
+
+    assert (await actor.plc.modbus["drive_enabled"].read(use_cache=False)) == 0
+    assert (await actor.plc.modbus["dome_open"].read(use_cache=False)) == 1
+
+
 async def test_command_dome_close(actor: ECPActor, mocker: MockerFixture):
     mocker.patch.object(actor.plc.dome, "_move", return_value=True)