Should all hardware objects' `init()` method call their super's `init()`?

[fabcor-maxiv]

Should all hardware objects' init() method call their super's init()?

For example some abstract hardware objects do not, and I wonder if it is on purpose or if it was simply forgotten:

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractActuator.py

  Lines 84 to 97 in 4947e54

  	def init(self):
  	"""Init properties: actuator_name, username, read_only and default_value."""
  	self.actuator_name = self.get_property("actuator_name")
  	self.read_only = self.get_property("read_only") or False
  	self.default_value = self.get_property("default_value")
  	if self.default_value is not None:
  	self.update_value(self.default_value)
  	limits = self.get_property("default_limits")
  	if limits:
  	try:
  	self._nominal_limits = tuple(literal_eval(limits))
  	except TypeError:
  	print("Invalid limits")
  	self.username = self.get_property("username")

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractAperture.py

  Lines 56 to 65 in 4947e54

  	def init(self):
  	try:
  	self._diameter_size_list = eval(self.get_property("diameter_size_list"))
  	except Exception:
  	logging.getLogger("HWR").error("Aperture: no diameter size list defined")
  	try:
  	self._position_list = eval(self.get_property("position_list"))
  	except Exception:
  	logging.getLogger("HWR").error("Aperture: no position list defined")

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractCollect.py

  Lines 101 to 136 in 4947e54

  	def init(self):
  	self.ready_event = gevent.event.Event()
  	undulators = []
  	try:
  	for undulator in self.config.undulators:
  	undulators.append(undulator)
  	except Exception:
  	pass
  	beam_div_hor, beam_div_ver = HWR.beamline.beam.get_beam_divergence()
  	self.set_beamline_configuration(
  	synchrotron_name=HWR.beamline.session.synchrotron_name,
  	directory_prefix=self.get_property("directory_prefix"),
  	default_exposure_time=HWR.beamline.detector.get_property(
  	"default_exposure_time"
  	),
  	minimum_exposure_time=HWR.beamline.detector.get_property(
  	"minimum_exposure_time"
  	),
  	detector_fileext=HWR.beamline.detector.get_property("fileSuffix"),
  	detector_type=HWR.beamline.detector.get_property("type"),
  	detector_manufacturer=HWR.beamline.detector.get_property("manufacturer"),
  	detector_model=HWR.beamline.detector.get_property("model"),
  	detector_px=HWR.beamline.detector.get_property("px"),
  	detector_py=HWR.beamline.detector.get_property("py"),
  	detector_binning_mode=HWR.beamline.detector.get_binning_mode(),
  	undulators=undulators,
  	focusing_optic=self.get_property("focusing_optic"),
  	monochromator_type=self.get_property("monochromator"),
  	beam_divergence_vertical=beam_div_hor,
  	beam_divergence_horizontal=beam_div_ver,
  	polarisation=self.get_property("polarisation"),
  	input_files_server=self.get_property("input_files_server"),
  	)

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractEnergyScan.py

  Lines 26 to 30 in 4947e54

  	def init(self):
  	"""Initialisation"""
  	self.lims = HWR.beamline.lims
  	if not self.lims:
  	logging.getLogger().warning("EnergyScan: no lims set")

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractMachineInfo.py

  Lines 54 to 61 in 4947e54

  	def init(self):
  	"""Get the attributes to be defined as keys in the _machine_info_dict"""
  	attr = self.get_property("parameters")
  	if attr:
  	self._check_attributes(literal_eval(attr))
  	else:
  	# at least current should be in the list
  	self._check_attributes(["current"])

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractOnlineProcessing.py

  Lines 90 to 97 in 4947e54

  	def init(self):
  	self.done_event = gevent.event.Event()
  	self.ssx_setup = self.get_object_by_role("ssx_setup")
  	self.result_types = self.get_property("result_types", DEFAULT_RESULT_TYPES)
  	self.start_command = str(self.get_property("processing_command"))
  	self.kill_command = str(self.get_property("kill_command"))
  	self.interpolate_results = self.get_property("interpolate_results")

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractProcedure.py

  Lines 105 to 106 in 4947e54

  	def init(self):
  	pass

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractSampleChanger.py

  Lines 233 to 250 in 4947e54

  	def init(self):
  	"""
  	HardwareObject init method
  	"""
  	use_update_timer = self.get_property("useUpdateTimer", True)
  	msg = f"SampleChanger: Using update timer is {use_update_timer}"
  	logging.getLogger("HWR").info(msg)
  	if use_update_timer:
  	task1s = self.__timer_1s_task(wait=False)
  	task1s.link(self._on_timer_1s_exit)
  	updateTask = self.__update_timer_task(wait=False)
  	updateTask.link(self._on_timer_update_exit)
  	self.use_update_timer = use_update_timer
  	self.update_info()

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractVideoDevice.py

  Lines 91 to 160 in 4947e54

  	def init(self):
  	"""Initialise the values from config and set default values,
  	when appropriate
  	"""
  	self.cam_name = self.get_property("name", "camera")
  	try:
  	self.cam_mirror = eval(self.get_property("mirror"))
  	except TypeError:
  	self.cam_mirror = [False, False]
  	try:
  	self.cam_encoding = self.get_property("encoding").lower()
  	except AttributeError:
  	# apply default value
  	self.cam_encoding = AbstractVideoDevice.default_cam_encoding
  	scale = self.get_property("scale")
  	try:
  	self.cam_scale_factor = eval(scale)
  	except TypeError:
  	logging.getLogger().warning(
  	"%s: failed to interpret scale factor for camera.\nUsing default.",
  	self.id,
  	)
  	self.cam_scale_factor = self.default_scale_factor
  	self.poll_interval = self.get_property("interval", self.default_poll_interval)
  	try:
  	self.cam_gain = float(self.get_property("gain"))
  	except TypeError:
  	pass
  	try:
  	self.cam_exposure = float(self.get_property("exposure"))
  	except TypeError:
  	# apply default value
  	self.cam_exposure = self.poll_interval / 1000.0
  	self.scale = self.get_property("scale", 1.0)
  	try:
  	self.cam_type = self.get_property("type").lower()
  	except AttributeError:
  	# apply default value
  	self.cam_type = self.default_cam_type
  	# Apply values
  	self.set_video_live(False)
  	time.sleep(0.1)
  	self.set_cam_encoding(self.cam_encoding)
  	self.set_exposure_time(self.cam_exposure)
  	if self.cam_gain is not None:
  	self.set_gain(self.cam_gain)
  	self.image_dimensions = self.get_image_dimensions()
  	# Start polling greenlet
  	if self.image_polling is None:
  	self.set_video_live(True)
  	self.change_owner()
  	logging.getLogger("HWR").info("Starting polling for camera")
  	self.image_polling = gevent.spawn(
  	self.do_image_polling, self.poll_interval / 1000.0
  	)
  	self.image_polling.link_exception(self.polling_ended_exc)
  	self.image_polling.link(self.polling_ended)

• mxcubecore/mxcubecore/HardwareObjects/abstract/AbstractXRFSpectrum.py

  Lines 66 to 72 in 4947e54

  	def init(self):
  	"""Initialisation"""
  	self.default_integration_time = self.get_property("default_integration_time", 3)
  	self.file_suffix = self.get_property("file_suffix", "dat")
  	self.lims = HWR.beamline.lims
  	if not self.lims:
  	logging.getLogger().warning("XRFSpectrum: no lims set")

I am not sure that it makes a big difference either way, but that seems strange to me. But again, things seem to work fine like this, so I am confused.

[rhfogh]

Well spotted.

The only init() method in BaseHardwareObjects is in HardwareObjectMixin, inherited by HardwareObject. The only thing it does is to set up machinery for exported commands. Now I am not sure whether that is actually ever used, but there seems to be a single call to execute_exported_command in mxcube_web adapter_base. Is that live?

One would say that HardwareObjects lower down in the inheritance tree should call their super - or have a comment saying why not. For objects just below HardwareObject (like the abstract classes listed) it would be a question for the people who wrote and (presumably) use the exported commands. For consistency one would say that either the super() functions should be called, or the init() should be removed from HardwareObject, if it is not needed.

[marcus-oscarsson]

The only init() method in BaseHardwareObjects is in HardwareObjectMixin, inherited by HardwareObject. The only thing it does is to set up machinery for exported commands. Now I am not sure whether that is actually ever used, but there seems to be a single call to execute_exported_command in mxcube_web adapter_base. Is that live?

It is still alive, but we might eventually remove it. Its quite convenient because it allows a HardwareObject to export methods directly via xml or yaml configuration. However there is now a almost equally practical and safer mechanism in mxcubeweb which might replace the "exported commands" of HardwareObjects.

One would say that HardwareObjects lower down in the inheritance tree should call their super - or have a comment saying why not. For objects just below HardwareObject (like the abstract classes listed) it would be a question for the people who wrote and (presumably) use the exported commands. For consistency one would say that either the super() functions should be called, or the init() should be removed from HardwareObject, if it is not needed.

I completely agree that there should in general be a comment in the cases when init() or any overriding method is not calling its super class. However it may not necessarily be incorrect to omit init(). In this case the functionality we don't get is the "exported methods". Its not a big issue but I would categorize it as a bug.

[beteva]

Some of the missing calls to superclass init() method are simply a non deliberate omission, mainly because the HardwareObjectMixin was not in place when the classes have been created. I do agree if it is deliberate, it should explain why. I believe the same question is true for the init(). In any case, if there is no addition to the super class methods, they should be rather removed from the inheriting class.