From e162ae23b550813b76c73e1d671fc87d5848cac2 Mon Sep 17 00:00:00 2001
From: thijs snijder <snijder@astron.nl>
Date: Wed, 21 Apr 2021 15:25:30 +0200
Subject: [PATCH] parsed review points

---
 devices/HW_device_template.py       | 26 ++++++-------
 devices/PCC.py                      | 57 ++++++++++++++--------------
 devices/SDP.py                      |  3 ++
 devices/clients/opcua_connection.py |  8 +++-
 devices/clients/test_client.py      | 59 +++++++++++------------------
 devices/src/attribute_wrapper.py    | 24 +-----------
 devices/src/comms_client.py         | 10 +----
 devices/src/hardware_device.py      | 17 +--------
 devices/test_device.py              |  5 +--
 9 files changed, 79 insertions(+), 130 deletions(-)

diff --git a/devices/HW_device_template.py b/devices/HW_device_template.py
index 2b2b0a648..0249747de 100644
--- a/devices/HW_device_template.py
+++ b/devices/HW_device_template.py
@@ -1,13 +1,11 @@
 # -*- coding: utf-8 -*-
 #
-# This file is part of the PCC project
-#
-#
+# This file wraps around a tango device class and provides a number of abstractions useful for hardware devices. It works together
 #
 # Distributed under the terms of the APACHE license.
 # See LICENSE.txt for more info.
 
-""" Hardware Device Server for LOFAR2.0
+"""
 
 """
 
@@ -23,19 +21,19 @@ __all__ = ["HW_dev"]
 
 class HW_dev(hardware_device):
     """
-	This class is the minimal (read empty) implementation of a class using 'hardware_device'
-	"""
+    This class is the minimal (read empty) implementation of a class using 'hardware_device'
+    """
 
     # ----------
     # Attributes
     # ----------
     """
-	attribute wrapper objects can be declared here. All attribute wrapper objects will get automatically put in a ist (attr_list) for easy access
+    attribute wrapper objects can be declared here. All attribute wrapper objects will get automatically put in a list (attr_list) for easy access
 
-	example = attribute_wrapper(comms_annotation="this is an example", datatype=numpy.double, dims=(8, 2), access=AttrWriteType.READ_WRITE)
-	...
+    example = attribute_wrapper(comms_annotation="this is an example", datatype=numpy.double, dims=(8, 2), access=AttrWriteType.READ_WRITE)
+    ...
 
-	"""
+    """
 
     def always_executed_hook(self):
         """Method always executed before any TANGO command is executed."""
@@ -44,10 +42,10 @@ class HW_dev(hardware_device):
     def delete_device(self):
         """Hook to delete resources allocated in init_device.
 
-		This method allows for any memory or other resources allocated in the
-		init_device method to be released.  This method is called by the device
-		destructor and by the device Init command (a Tango built-in).
-		"""
+        This method allows for any memory or other resources allocated in the
+        init_device method to be released.  This method is called by the device
+        destructor and by the device Init command (a Tango built-in).
+        """
         self.debug_stream("Shutting down...")
 
         self.Off()
diff --git a/devices/PCC.py b/devices/PCC.py
index d6b255335..b1bc93d9c 100644
--- a/devices/PCC.py
+++ b/devices/PCC.py
@@ -20,23 +20,24 @@ from tango.server import device_property
 from clients.opcua_connection import OPCUAConnection
 from src.attribute_wrapper import *
 from src.hardware_device import *
+from src.lofar_logging import device_logging_to_python
 
 __all__ = ["PCC", "main"]
 
-
+@device_logging_to_python({"device": "PCC"})
 class PCC(hardware_device):
     """
 
-	**Properties:**
+    **Properties:**
 
-	- Device Property
-		OPC_Server_Name
-			- Type:'DevString'
-		OPC_Server_Port
-			- Type:'DevULong'
-		OPC_Time_Out
-			- Type:'DevDouble'
-	"""
+    - Device Property
+        OPC_Server_Name
+            - Type:'DevString'
+        OPC_Server_Port
+            - Type:'DevULong'
+        OPC_Time_Out
+            - Type:'DevDouble'
+    """
 
     # -----------------
     # Device Properties
@@ -98,10 +99,10 @@ class PCC(hardware_device):
     def delete_device(self):
         """Hook to delete resources allocated in init_device.
 
-		This method allows for any memory or other resources allocated in the
-		init_device method to be released.  This method is called by the device
-		destructor and by the device Init command (a Tango built-in).
-		"""
+        This method allows for any memory or other resources allocated in the
+        init_device method to be released.  This method is called by the device
+        destructor and by the device Init command (a Tango built-in).
+        """
         self.debug_stream("Shutting down...")
 
         self.Off()
@@ -151,8 +152,8 @@ class PCC(hardware_device):
     def RCU_off(self):
         """
 
-		:return:None
-		"""
+        :return:None
+        """
         self.function_mapping["RCU_off"]()
 
     @command()
@@ -162,8 +163,8 @@ class PCC(hardware_device):
     def RCU_on(self):
         """
 
-		:return:None
-		"""
+        :return:None
+        """
         self.function_mapping["RCU_on"]()
 
     @command()
@@ -173,8 +174,8 @@ class PCC(hardware_device):
     def ADC_on(self):
         """
 
-		:return:None
-		"""
+        :return:None
+        """
         self.function_mapping["ADC_on"]()
 
     @command()
@@ -184,8 +185,8 @@ class PCC(hardware_device):
     def RCU_update(self):
         """
 
-		:return:None
-		"""
+        :return:None
+        """
         self.function_mapping["RCU_update"]()
 
     @command()
@@ -195,8 +196,8 @@ class PCC(hardware_device):
     def CLK_off(self):
         """
 
-		:return:None
-		"""
+        :return:None
+        """
         self.function_mapping["CLK_off"]()
 
     @command()
@@ -206,8 +207,8 @@ class PCC(hardware_device):
     def CLK_on(self):
         """
 
-		:return:None
-		"""
+        :return:None
+        """
         self.function_mapping["CLK_on"]()
 
     @command()
@@ -217,8 +218,8 @@ class PCC(hardware_device):
     def CLK_PLL_setup(self):
         """
 
-		:return:None
-		"""
+        :return:None
+        """
         self.function_mapping["CLK_PLL_setup"]()
 
 
diff --git a/devices/SDP.py b/devices/SDP.py
index 879ed5943..05a5841bf 100644
--- a/devices/SDP.py
+++ b/devices/SDP.py
@@ -20,9 +20,12 @@ from clients.opcua_connection import OPCUAConnection
 from src.attribute_wrapper import *
 from src.hardware_device import *
 
+from src.lofar_logging import device_logging_to_python
+
 
 __all__ = ["SDP", "main"]
 
+@device_logging_to_python({"device": "SDP"})
 class SDP(hardware_device):
     """
 
diff --git a/devices/clients/opcua_connection.py b/devices/clients/opcua_connection.py
index d7094ef02..85962f421 100644
--- a/devices/clients/opcua_connection.py
+++ b/devices/clients/opcua_connection.py
@@ -1,5 +1,9 @@
-from src.comms_client import *
-
+from threading import Thread
+import socket
+from src.comms_client import CommClient
+import numpy
+import opcua
+from opcua import Client
 
 __all__ = ["OPCUAConnection"]
 
diff --git a/devices/clients/test_client.py b/devices/clients/test_client.py
index 4227ac292..640d2edb5 100644
--- a/devices/clients/test_client.py
+++ b/devices/clients/test_client.py
@@ -1,28 +1,23 @@
 from src.comms_client import *
+import numpy
 
-<<<<<<< HEAD
-<<<<<<< HEAD
 import os
-=======
->>>>>>> 99e3d08... fixed 'illegal state transition' bug and off->initialise state bug
-=======
->>>>>>> 99e3d08... fixed 'illegal state transition' bug and off->initialise state bug
 
 # <class 'numpy.bool_'>
 
 class example_client(CommClient):
     """
-	this class provides an example implementation of a comms_client.
-	Durirng initialisation it creates a correctly shaped zero filled value. on read that value is returned and on write its modified.
-	"""
+    this class provides an example implementation of a comms_client.
+    Durirng initialisation it creates a correctly shaped zero filled value. on read that value is returned and on write its modified.
+    """
 
     def start(self):
         super().start()
 
     def __init__(self, fault_func, streams, try_interval=2):
         """
-		initialises the class and tries to connect to the client.
-		"""
+        initialises the class and tries to connect to the client.
+        """
         super().__init__(fault_func, streams, try_interval)
 
         # Explicitly connect
@@ -33,16 +28,8 @@ class example_client(CommClient):
 
     def connect(self):
         """
-		this function provides a location for the code neccecary to connect to the client
-		"""
-<<<<<<< HEAD
-<<<<<<< HEAD
-        self.streams.debug_stream(os.path.dirname(os.path.abspath(__file__)))
-=======
-=======
->>>>>>> 99e3d08... fixed 'illegal state transition' bug and off->initialise state bug
-
->>>>>>> 99e3d08... fixed 'illegal state transition' bug and off->initialise state bug
+        this function provides a location for the code neccecary to connect to the client
+        """
         self.streams.debug_stream("the example client doesn't actually connect to anything silly")
 
         self.connected = True  # set connected to true
@@ -54,23 +41,23 @@ class example_client(CommClient):
 
     def _setup_annotation(self, annotation):
         """
-		this function gives the client access to the comm client annotation data given to the attribute wrapper.
-		The annotation data can be used to provide whatever extra data is necessary in order to find/access the monitor/control point.
+        this function gives the client access to the comm client annotation data given to the attribute wrapper.
+        The annotation data can be used to provide whatever extra data is necessary in order to find/access the monitor/control point.
 
-		the annotation can be in whatever format may be required. it is up to the user to handle its content
-		example annotation may include:
-		- a file path and file line/location
-		- COM object path
-		"""
+        the annotation can be in whatever format may be required. it is up to the user to handle its content
+        example annotation may include:
+        - a file path and file line/location
+        - COM object path
+        """
 
         # as this is an example, just print the annotation
         self.streams.debug_stream("annotation: {}".format(annotation))
 
     def _setup_value_conversion(self, attribute):
         """
-		gives the client access to the attribute_wrapper object in order to access all
-		necessary data such as dimensionality and data type
-		"""
+        gives the client access to the attribute_wrapper object in order to access all
+        necessary data such as dimensionality and data type
+        """
 
         if attribute.dim_y > 1:
             dims = (attribute.dim_y, attribute.dim_x)
@@ -83,8 +70,8 @@ class example_client(CommClient):
 
     def _setup_mapping(self, dims, dtype):
         """
-		takes all gathered data to configure and return the correct read and write functions
-		"""
+        takes all gathered data to configure and return the correct read and write functions
+        """
 
         value = numpy.zeros(dims, dtype)
 
@@ -101,9 +88,9 @@ class example_client(CommClient):
 
     def setup_attribute(self, annotation=None, attribute=None):
         """
-		MANDATORY function: is used by the attribute wrapper to get read/write functions.
-		must return the read and write functions
-		"""
+        MANDATORY function: is used by the attribute wrapper to get read/write functions.
+        must return the read and write functions
+        """
 
         # process the comms_annotation
         self._setup_annotation(annotation)
diff --git a/devices/src/attribute_wrapper.py b/devices/src/attribute_wrapper.py
index 039849c32..107632521 100644
--- a/devices/src/attribute_wrapper.py
+++ b/devices/src/attribute_wrapper.py
@@ -11,8 +11,6 @@ logger = logging.getLogger()
 
 class attribute_wrapper(attribute):
     """
-<<<<<<< HEAD
-<<<<<<< HEAD
     Wraps all the attributes in a wrapper class to manage most of the redundant code behind the scenes
     """
 
@@ -26,26 +24,6 @@ class attribute_wrapper(attribute):
         dims: dimensions of the
         init_value: value
         """
-=======
-		Wraps all the attributes in a wrapper class to manage most of the redundant code behind the scenes
-	"""
-
-    def __init__(self, comms_annotation=None, datatype=None, dims=(1,), access=AttrWriteType.READ, init_value=None, **kwargs):
-        """
-		wraps around the tango Attribute class. Provides an easier interface for 1d or 2d arrays. Also provides a way to abstract
-		managing the communications interface.
-		"""
->>>>>>> 99e3d08... fixed 'illegal state transition' bug and off->initialise state bug
-=======
-		Wraps all the attributes in a wrapper class to manage most of the redundant code behind the scenes
-	"""
-
-    def __init__(self, comms_annotation=None, datatype=None, dims=(1,), access=AttrWriteType.READ, init_value=None, **kwargs):
-        """
-		wraps around the tango Attribute class. Provides an easier interface for 1d or 2d arrays. Also provides a way to abstract
-		managing the communications interface.
-		"""
->>>>>>> 99e3d08... fixed 'illegal state transition' bug and off->initialise state bug
 
         # ensure the type is a numpy array
         if "numpy" not in str(datatype) and datatype != str:
@@ -162,7 +140,7 @@ class attribute_wrapper(attribute):
             def pass_func(value=None):
                 pass
 
-            logger.error("setting comm_client failed. using pass function instead")
+            logger.error("Exception while setting %s read/write functions. using pass function instead.", self.comms_annotation)
 
             self.read_function = pass_func
             self.write_function = pass_func
diff --git a/devices/src/comms_client.py b/devices/src/comms_client.py
index fc33bfad8..0405c737d 100644
--- a/devices/src/comms_client.py
+++ b/devices/src/comms_client.py
@@ -1,13 +1,5 @@
 from threading import Thread
-import socket
 import time
-import numpy
-
-import opcua
-from opcua import Client
-
-from tango import DevState
-
 
 class CommClient(Thread):
     """
@@ -75,7 +67,7 @@ class CommClient(Thread):
                 self.fault_func()
 
     def ping(self):
-        pass
+        return
 
     def stop(self):
         """
diff --git a/devices/src/hardware_device.py b/devices/src/hardware_device.py
index 0ee9d826b..bd570d3ff 100644
--- a/devices/src/hardware_device.py
+++ b/devices/src/hardware_device.py
@@ -17,13 +17,13 @@ from tango import DevState, DebugIt
 # Additional import
 
 from src.attribute_wrapper import *
-
+from src.lofar_logging import log_exceptions
 
 __all__ = ["hardware_device"]
 
 from src.wrappers import only_in_states
 
-
+@log_exceptions()
 class hardware_device(Device):
     """
 
@@ -87,17 +87,6 @@ class hardware_device(Device):
 
         self.set_state(DevState.STANDBY)
 
-    # @only_in_states([DevState.INIT])
-    # def Standby(self):
-    # 	"""
-    # 	Command to ask for initialisation of this device. Can only be called in FAULT or OFF state.
-    #
-    # 	:return:None
-    # 	"""
-    #
-    # 	self.standby()
-    # 	self.set_state(DevState.STANDBY)
-
     @command()
     @only_in_states([DevState.STANDBY])
     @DebugIt()
@@ -154,8 +143,6 @@ class hardware_device(Device):
         pass
     def on(self):
         pass
-    def standby(self):
-        pass
     def initialise(self):
         pass
 
diff --git a/devices/test_device.py b/devices/test_device.py
index f84d810e7..1d7a2f486 100644
--- a/devices/test_device.py
+++ b/devices/test_device.py
@@ -7,8 +7,7 @@
 # Distributed under the terms of the APACHE license.
 # See LICENSE.txt for more info.
 
-""" PCC Device Server for LOFAR2.0
-
+""" test Device Server
 """
 
 # PyTango imports
@@ -71,7 +70,7 @@ class test_device(hardware_device):
 
         self.set_state(DevState.INIT)
 
-        # set up the OPC ua client
+        # set up the test client
         self.example_client = example_client(self.Fault, self)
 
         # map an access helper class
-- 
GitLab