Improvements to Modbus handling

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

python/lvmecp/actor/actor.py

@@ -60,8 +60,6 @@
         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 @@
         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 @@
         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 @@
         """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()

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()

python/lvmecp/actor/commands/engineering.py

@@ -18,7 +18,28 @@
 
 
 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")
@@ -36,12 +57,32 @@
     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 +91,14 @@
 
     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))

python/lvmecp/actor/commands/modbus.py

@@ -0,0 +1,178 @@
+#!/usr/bin/env python
+# -*- coding: utf-8 -*-
+#
+# @Author: José Sánchez-Gallego ([email protected])
+# @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, Literal
+
+import click
+
+from lvmecp.modbus import ModbusRegister
+
+from . import parser
+
+
+if TYPE_CHECKING:
+    from lvmecp.actor import ECPCommand
+    from lvmecp.modbus import RegisterModes
+
+
+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
+
+        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
+                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
+
+    return register
+
+
+@parser.group()
+def modbus():
+    """Low-level access to the PLC Modbus variables."""
+
+    pass
+
+
+@modbus.command()
+@click.argument("address", metavar="ADDRESS|NAME")
+@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.",
+)
+@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=register_type,
+            allow_unknown=allow_unknown,
+        )
+    ):
+        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)
+@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.",
+)
+@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: 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=register_type,
+            allow_unknown=allow_unknown,
+        )
+    ):
+        return False
+
+    name = register.name
+
+    if register.readonly:
+        return command.fail(f"Register {name!r} is read-only.")
+
+    if register.mode == "coil":
+        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)
+
+    return command.finish(
+        register={
+            "name": name,
+            "address": register.address,
+            "value": new_value,
+        }
+    )