Skip to content

Commit

Permalink
Improvements to Modbus handling (#32)
Browse files Browse the repository at this point in the history
* Update modbus variables

* Update config

* Improve performance of PLC reading

* Report/sec software/hardware overrides in engineering mode

* Add low-level modbus command

* Improve the simulator to handle events and register overrides

* Improve test coverage and fix some issues

* Allow reading/writing to unknown registers

* Fix linting

* Update changelog

* Add test coverage for modbus.py

* Add more realistic test for dome opening
  • Loading branch information
albireox authored Dec 29, 2024
1 parent d32b54c commit b0c7349
Show file tree
Hide file tree
Showing 23 changed files with 1,329 additions and 333 deletions.
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
10 changes: 3 additions & 7 deletions python/lvmecp/actor/actor.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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):
Expand All @@ -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()
Expand Down Expand Up @@ -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()
Expand Down
15 changes: 9 additions & 6 deletions python/lvmecp/actor/commands/dome.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.")

Expand All @@ -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()
Expand All @@ -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()
69 changes: 49 additions & 20 deletions python/lvmecp/actor/commands/engineering.py
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -36,12 +57,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()
Expand All @@ -50,26 +91,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))
178 changes: 178 additions & 0 deletions python/lvmecp/actor/commands/modbus.py
Original file line number Diff line number Diff line change
@@ -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,
}
)
Loading

0 comments on commit b0c7349

Please sign in to comment.