Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DM-45744 Implement ATBuilding CSC +790-148 #3

Merged
merged 15 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .pre-commit-config.yaml
Copy link
Member

Choose a reason for hiding this comment

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

Please, don't commit the .pre-commit-config.yaml file. This should be generated by generate_pre_commit_conf from ts-pre-commit-config package.

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.6.0
hooks:
- id: check-yaml
args:
- "--unsafe"
- id: end-of-file-fixer
- id: trailing-whitespace
- repo: https://github.com/psf/black-pre-commit-mirror
rev: 24.4.2
hooks:
- id: black
# It is recommended to specify the latest version of Python
# supported by your project here, or alternatively use
# pre-commit's default_language_version, see
# https://pre-commit.com/#top_level-default_language_version
language_version: python3.12
- repo: https://github.com/pycqa/isort
rev: 5.13.2
hooks:
- id: isort
name: isort (python)
- repo: https://github.com/astral-sh/ruff-pre-commit
# Ruff version.
rev: v0.5.5
hooks:
- id: ruff
6 changes: 4 additions & 2 deletions python/lsst/ts/vent/controller/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@
# along with this program. If not, see <https://www.gnu.org/licenses/>.

try:
from .version import * # Generated by sconsUtils
from .version import __version__ # Generated by sconsUtils
except ImportError:
__version__ = "?"

from .config import Config
from .controller import Controller, VentGateState
from .controller import Controller
from .dispatcher import Dispatcher

__all__ = ["__version__", "Config", "Controller", "Dispatcher"]
Copy link
Member

Choose a reason for hiding this comment

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

We usually don't have this __all__`` definition here in the init` file.

136 changes: 125 additions & 11 deletions python/lsst/ts/vent/controller/controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,33 +22,49 @@
import logging
from enum import IntEnum

import megaind
try:
import megaind
Copy link
Member

Choose a reason for hiding this comment

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

I missed the inclusion of this package dependency. I don't think this is part of our stack. Do you really need it?

Usually, adding new package dependencies require approval by myself and/or Wouter. We should discuss this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's necessary as a part of the hardware controlling the vents - the Sequent Microsystems Industrial Automation Hat.


MEGAIND_AVAILABLE = True
except ImportError:
MEGAIND_AVAILABLE = False
import pymodbus.client

from . import vf_drive
from .config import Config
from .simulate import DomeVentsSimulator

__all__ = ["Controller"]
__all__ = ["Controller", "VentGateState"]


class VentGateState(IntEnum):
Copy link
Member

Choose a reason for hiding this comment

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

Should this be in ts-xml?

CLOSED = 0
PARTIALLY_OPEN = 1
OPEN = 2
CLOSED = 1
PARTIALLY_OPEN = 2
OPEN = 3
FAULT = -1


class FanDriveState(IntEnum):
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be in ts-xml?

STOPPED = 1
OPERATING = 2
FAULT = 3


class Controller:
"""A controller that commands the components associated with the AT dome
vents and fans. The code in this class is meant to run on the Raspberry Pi
described in
https://confluence.lsstcorp.org/display/~fritzm/Auxtel+Vent+Gate+Automation.
"""

def __init__(self, config: Config = Config()):
def __init__(self, config: Config = Config(), simulate: bool = False):
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it is a good idea to pass config = Config() as default parameter. In cases like this I would suggest

def __init__(self, config: Config | None = None, simulate: bool = False):
    self.config = config if config is not None else Config()

self.cfg = config
Copy link
Member

Choose a reason for hiding this comment

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

Please expand the attribute name to self.config instead.

self.default_fan_frequency = self.cfg.max_freq
self.log = logging.getLogger(type(self).__name__)
self.simulate = simulate
Copy link
Member

Choose a reason for hiding this comment

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

Do you really need to store the simulate flag? I think you can determine if it is in simulation mode by checking it self.simulator is None.

self.simulator = DomeVentsSimulator(self.cfg) if simulate else None
self.vfd_client = None
self.connected = False

async def connect(self) -> None:
"""Connects to the variable frequency drive via modbus.
Expand All @@ -58,10 +74,24 @@ async def connect(self) -> None:
ModbusException
If the variable frequency drive is not available.
"""
if self.simulate:
await self.simulator.start()

self.vfd_client = pymodbus.client.AsyncModbusTcpClient(
self.cfg.hostname, port=self.cfg.port
)
await self.vfd_client.connect()
self.connected = True

async def stop(self) -> None:
"""Disconnects from the variable frequency drive, and stops
the simulator if simulating.
"""
if self.simulate:
await self.simulator.stop()

if self.vfd_client is not None:
self.vfd_client.close()

async def get_fan_manual_control(self) -> bool:
"""Returns the variable frequency drive setting for manual
Expand All @@ -83,6 +113,7 @@ async def get_fan_manual_control(self) -> bool:
"""

self.log.debug("get fan_manual_control")
assert self.connected
settings = tuple(
[
(
Expand Down Expand Up @@ -117,6 +148,7 @@ async def fan_manual_control(self, manual: bool) -> None:
"""

self.log.debug("set vfd_manual_control")
assert self.connected
settings = vf_drive.MANUAL if manual else vf_drive.AUTO
for address, value in zip(vf_drive.CFG_REGISTERS, settings):
await self.vfd_client.write_register(
Expand All @@ -132,6 +164,7 @@ async def start_fan(self):
If a communications error occurs.
"""
self.log.debug("start_fan()")
assert self.connected
await self.set_fan_frequency(self.default_fan_frequency)

async def stop_fan(self):
Expand All @@ -143,6 +176,7 @@ async def stop_fan(self):
If a communications error occurs.
"""
self.log.debug("stop_fan()")
assert self.connected
await self.set_fan_frequency(0.0)

async def get_fan_frequency(self) -> float:
Expand All @@ -155,6 +189,7 @@ async def get_fan_frequency(self) -> float:
"""

self.log.debug("get fan_frequency")
assert self.connected
cmd = (
await self.vfd_client.read_holding_registers(
slave=self.cfg.slave, address=vf_drive.Registers.CMD_REGISTER
Expand Down Expand Up @@ -188,6 +223,7 @@ async def set_fan_frequency(self, frequency: float) -> None:
If a communications error occurs.
"""
self.log.debug("set fan_frequency")
assert self.connected
if not 0 <= frequency <= self.cfg.max_freq:
raise ValueError(f"Frequency must be between 0 and {self.cfg.max_freq}")

Expand All @@ -209,11 +245,35 @@ async def vfd_fault_reset(self) -> None:
If a communications error occurs.
"""

assert self.connected
for address, value in vf_drive.FAULT_RESET_SEQUENCE:
await self.vfd_client.write_register(
slave=self.cfg.slave, address=address, value=value
)

async def get_drive_state(self) -> FanDriveState:
"""Returns the current fan drive state based on the contents
of the IPAE register.

Copy link
Member

Choose a reason for hiding this comment

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

Please, document return value.

Copy link
Member

Choose a reason for hiding this comment

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

Please use the numpy docs style.

This should be something like:

Returns
--------
`FanDriveState`
    Current fan drive state.

You don't need to document all the values as that should be in the enumeration.

Raises
------
ModbusException
tribeiro marked this conversation as resolved.
Show resolved Hide resolved
If a communications error occurs.
"""

assert self.connected
ipae = (
tribeiro marked this conversation as resolved.
Show resolved Hide resolved
await self.vfd_client.read_holding_registers(
slave=self.cfg.slave, address=vf_drive.Registers.IPAE_REGISTER
)
).registers[0]

if ipae in (0, 1, 2, 3, 5):
return FanDriveState.STOPPED
if ipae == 4:
return FanDriveState.OPERATING
return FanDriveState.FAULT

async def last8faults(self) -> list[tuple[int, str]]:
"""Returns the last eight fault conditions recorded by the drive.

Expand All @@ -231,8 +291,9 @@ async def last8faults(self) -> list[tuple[int, str]]:
"""

self.log.debug("last8faults")
assert self.connected
rvals = await self.vfd_client.read_holding_registers(
slave=1, address=vf_drive.Registers.FAULT_REGISTER, count=8
slave=self.cfg.slave, address=vf_drive.Registers.FAULT_REGISTER, count=8
Copy link
Member

Choose a reason for hiding this comment

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

I would avoid the use of terms like slave. Please, consider renaming this; e.g. client or any other less offensive term.

)
return [(r, vf_drive.FAULTS[r]) for r in rvals.registers]

Expand All @@ -246,6 +307,10 @@ def vent_open(self, vent_number: int) -> None:

Raises
------
ModuleNotFoundError
Copy link
Member

Choose a reason for hiding this comment

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

So, you are not really raising this exception here. But we should talk abut this.

If the megaind module has not been installed, in which case the
daughterboard cannot be controlled.

ValueError
If vent_number is invalid.

Expand All @@ -254,11 +319,12 @@ def vent_open(self, vent_number: int) -> None:
"""

self.log.debug(f"vent_open({vent_number})")
assert self.connected
if not 0 <= vent_number <= 3:
raise ValueError(f"Invalid {vent_number=} should be between 0 and 3")
if self.cfg.vent_signal_ch[vent_number] == -1:
raise ValueError(f"Vent {vent_number=} is not configured.")
megaind.setOd(self.cfg.megaind_stack, self.cfg.vent_signal_ch[vent_number], 1)
self.setOd(self.cfg.megaind_stack, self.cfg.vent_signal_ch[vent_number], 1)

def vent_close(self, vent_number: int) -> None:
"""Closes the specified vent.
Expand All @@ -270,6 +336,10 @@ def vent_close(self, vent_number: int) -> None:

Raises
------
ModuleNotFoundError
If the megaind module has not been installed, in which case the
daughterboard cannot be controlled.

ValueError
If vent_number is invalid.

Expand All @@ -278,11 +348,12 @@ def vent_close(self, vent_number: int) -> None:
"""

self.log.debug(f"vent_close({vent_number})")
assert self.connected
if not 0 <= vent_number <= 3:
raise ValueError(f"Invalid {vent_number=} should be between 0 and 3")
if self.cfg.vent_signal_ch[vent_number] == -1:
raise ValueError(f"Vent {vent_number=} is not configured.")
megaind.setOd(self.cfg.megaind_stack, self.cfg.vent_signal_ch[vent_number], 0)
self.setOd(self.cfg.megaind_stack, self.cfg.vent_signal_ch[vent_number], 0)

def vent_state(self, vent_number: int) -> VentGateState:
"""Returns the state of the specified vent.
Expand All @@ -294,6 +365,10 @@ def vent_state(self, vent_number: int) -> VentGateState:

Raises
------
ModuleNotFoundError
If the megaind module has not been installed, in which case the
daughterboard cannot be controlled.

ValueError
If vent_number is invalid.

Expand All @@ -302,6 +377,7 @@ def vent_state(self, vent_number: int) -> VentGateState:
"""

self.log.debug(f"vent_state({vent_number})")
assert self.connected
if not 0 <= vent_number <= 3:
raise ValueError(f"Invalid {vent_number=} should be between 0 and 3")

Expand All @@ -311,10 +387,10 @@ def vent_state(self, vent_number: int) -> VentGateState:
):
raise ValueError(f"Vent {vent_number=} is not configured.")

op_state = megaind.getOptoCh(
op_state = self.getOptoCh(
self.cfg.megaind_stack, self.cfg.vent_open_limit_ch[vent_number]
)
cl_state = megaind.getOptoCh(
cl_state = self.getOptoCh(
self.cfg.megaind_stack, self.cfg.vent_close_limit_ch[vent_number]
)

Expand All @@ -327,3 +403,41 @@ def vent_state(self, vent_number: int) -> VentGateState:
return VentGateState.CLOSED
case _:
return VentGateState.FAULT

def getOptoCh(self, *args, **kwargs) -> int:
Copy link
Member

Choose a reason for hiding this comment

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

Method should be snake_case. But we need to talk about the use of this megaind dependency.

"""Calls megaind.getOptoCh or a simulated getOptoCh depending
whether the class was instantiated with simulate = True.

Copy link
Member

Choose a reason for hiding this comment

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

Please, document input arguments and return values (use numpy docs style).

Raises
------
ModuleNotFoundError
If the megaind module has not been installed, in which case the
daughterboard cannot be controlled.
"""

assert self.connected
if self.simulate:
return self.simulator.getOptoCh(*args, **kwargs)
else:
if not MEGAIND_AVAILABLE:
raise ModuleNotFoundError("The megaind module is not available.")
return megaind.getOptoCh(*args, **kwargs)

def setOd(self, *args, **kwargs) -> None:
Copy link
Member

Choose a reason for hiding this comment

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

Methods should be snake_case but, same as above, we need to talk about the use of megaind.

"""Calls megaind.setOd or a simulated setOd depending
whether the class was instantiated with simulate = True.

Copy link
Member

Choose a reason for hiding this comment

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

Please, document input args.

Raises
------
ModuleNotFoundError
If the megaind module has not been installed, in which case the
daughterboard cannot be controlled.
"""

assert self.connected
if self.simulate:
self.simulator.setOd(*args, **kwargs)
else:
if not MEGAIND_AVAILABLE:
raise ModuleNotFoundError("The megaind module is not available.")
megaind.setOd(*args, **kwargs)
Loading
Loading