DM-45744 Implement ATBuilding CSC +790-148 #3
Conversation
.pre-commit-config.yaml
Outdated
There was a problem hiding this comment.
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.
| from .dispatcher import Dispatcher | ||
|
|
||
| __all__ = ["__version__", "Config", "Controller", "Dispatcher"] |
There was a problem hiding this comment.
We usually don't have this __all__`` definition here in the init` file.
| @@ -22,33 +22,49 @@ | |||
| import logging | |||
| from enum import IntEnum | |||
|
|
|||
| import megaind | |||
| try: | |||
| import megaind | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think it's necessary as a part of the hardware controlling the vents - the Sequent Microsystems Industrial Automation Hat.
|
|
||
| __all__ = ["Controller"] | ||
| __all__ = ["Controller", "VentGateState"] | ||
|
|
||
|
|
||
| class VentGateState(IntEnum): |
| FAULT = -1 | ||
|
|
||
|
|
||
| class FanDriveState(IntEnum): |
| raise ModuleNotFoundError("The megaind module is not available.") | ||
| return megaind.getOptoCh(*args, **kwargs) | ||
|
|
||
| def setOd(self, *args, **kwargs) -> None: |
There was a problem hiding this comment.
Methods should be snake_case but, same as above, we need to talk about the use of megaind.
| return value.lower() in ("true", "t", "1") | ||
| if value.lower() in ("true", "t", "1"): | ||
| return True | ||
| if value.lower() in ("false", "f", "0"): |
| return True | ||
| if value.lower() in ("false", "f", "0"): | ||
| return False | ||
| raise ValueError(f"Expected bool value but got {value}") |
There was a problem hiding this comment.
Maybe document what are the valid values for conversion to bool?
There was a problem hiding this comment.
Consider renaming this dome_vents_simulator.py
| async def get_drive_state(self) -> FanDriveState: | ||
| """Returns the current fan drive state based on the contents | ||
| of the IPAE register. | ||
|
|
There was a problem hiding this comment.
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.
| def get_opto_ch(self, *args, **kwargs) -> int: | ||
| """Calls hardware I/O or a simulated substitute depending | ||
| whether the class was instantiated with simulate = True. | ||
|
|
There was a problem hiding this comment.
Please, document input arguments and return values (use numpy docs style).
| def set_od(self, *args, **kwargs) -> None: | ||
| """Calls harware I/O or a simulated substitute depending | ||
| whether the class was instantiated with simulate = True. | ||
|
|
| @@ -27,7 +49,15 @@ def _cast_string_to_type(new_type: type, value: str): | |||
| """ | |||
No description provided.