Add support for Astrohaven dome #188

jamessynge · 2017-12-12T03:25:12Z

Includes a simulator for the PLC (programmable logic controller) that controls the dome's motors.

Addresses #134, though there is not yet any integration of domes into the state machine.

Includes a simulator for the PLC (programmable logic controller) that controls the dome's motors.

wtgee

Looks like this is the same PR as before but with squashed commits. Added some comments here but there were some other comments in old PR).

wtgee · 2017-12-12T20:22:50Z

pocs/dome/abstract_serial_dome.py

+    def is_connected(self):
+        """True if connected to the hardware or driver."""
+        if self.ser:
+            return self.ser.is_connected


Seems like we should be setting the self._is_connected property and then returning that. Not that someone should be messing with the private variable, but this would get it out of sync.

I've decided that it doesn't make great sense to put the _is_connected in the base class, AbstractDome, in part because of issues exactly like this. So, I'm inclined to respond to this feedback by stripping it out from AbstractDome, and implementing it in the sub-classes as appropriate to the means to establishing that connection.

wtgee · 2017-12-12T20:23:36Z

pocs/dome/abstract_serial_dome.py

+        return False
+
+    def connect(self):
+        """ Connects to the device via the serial port (`self._port`)


I still don't like advertising a private variable in the docstring. But I still kind of think the port shouldn't be private anyway.

OK, will move it to public.

Though I assume you agree that port shouldn't be changed, right?

wtgee · 2017-12-12T20:24:24Z

pocs/dome/abstract_serial_dome.py

+            except OSError as err:
+                self.logger.error("OS error: {0}".format(err))
+            except error.BadSerialConnection as err:
+                self.logger.warning('Could not create serial connection to mount.')


Still refers to mount here and in line below.

Fixed locally, will push later.

wtgee · 2017-12-12T20:24:45Z

pocs/dome/abstract_serial_dome.py

+        self.logger.debug("Closing serial port for dome")
+        self._is_connected = self.ser.disconnect()
+
+    def verifyConnected(self):


verify_connected

Done locally.

wtgee · 2017-12-12T20:28:14Z

pocs/tests/test_astrohaven_dome.py

+import pytest
+import serial
+
+from pocs.dome import CreateDomeFromConfig


This should be create_dome_from_config but I couldn't comment on file up above and must have skipped it in previous review.

wtgee · 2017-12-12T20:36:48Z

pocs/dome/astrohaven.py

+        return self.is_closed
+
+    @property
+    def state(self):


Comment in other PR was about switching this to status. You seem to be using read_latest_state for all decision making so this seems like it is for a user.

Yes, it is for the end user. Will create another issue/branch/PR for general changes to the dome API.

Filed issue #189 for this.

wtgee · 2017-12-12T20:38:06Z

pocs/dome/astrohaven.py

+            return chr(data[-1])
+        return None
+
+    def nudge_shutter(self, send, target_feedback):


Docstrings missing. Important here where there are parameters.

wtgee · 2017-12-12T20:38:59Z

pocs/dome/astrohaven.py

+        return feedback == target_feedback
+
+    def full_move(self, send, target_feedback):
+        """Send a command code until the target_feedback is recieved, or a timeout is reached."""


Document the parameters. Not sure if send should be bytes or a string nor what target_feedback represents.

wtgee · 2017-12-12T20:43:59Z

pocs/dome/astrohaven.py

+        feedback = self.read_latest_state()
+        return feedback == target_feedback
+
+    def full_move(self, send, target_feedback):


Seems like this should just be named open_shutter or open. When I think of a dome moving I think of it rotating. For consistency with (eventual) other dome subclasses I think an open method makes the most sense. All domes open in some way.

full_move goes in either direction, i.e. opens or closes, o it moves from whatever the current position is to the commanded position (i.e. until target_feedback is the output).

Also renamed internal methods to have a leading underscore. Tried to be more consistent in describing the Astrohaven protocol (command vs. action).

codecov · 2017-12-13T22:29:15Z

Codecov Report

Merging #188 into develop will decrease coverage by 0.39%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           develop     #188     +/-   ##
==========================================
- Coverage    80.52%   80.13%   -0.4%     
==========================================
  Files           39       42      +3     
  Lines         2794     3136    +342     
  Branches       354      404     +50     
==========================================
+ Hits          2250     2513    +263     
- Misses         435      490     +55     
- Partials       109      133     +24

Impacted Files	Coverage Δ
pocs/utils/rs232.py	`93.93% <100%> (+0.06%)`	⬆️
pocs/dome/simulator.py	`100% <100%> (ø)`	⬆️
pocs/utils/error.py	`92.85% <100%> (+0.35%)`	⬆️
pocs/dome/abstract_serial_dome.py	`63.04% <63.04%> (ø)`
pocs/dome/protocol_astrohaven_simulator.py	`76% <76%> (ø)`
pocs/dome/astrohaven.py	`80% <80%> (ø)`
pocs/dome/__init__.py	`91.66% <92.85%> (+14.92%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c582bef...7057db2. Read the comment docs.

only. Modified existing code to make it readily apparent that it is a presentation string and NOT a keyword or enum. Fixed imports in the dome code to be root relative (~absolute), and to be imports of packages and modules, not symbols in modules.

…n_dome.py::test_open_and_close_slit

Fix error dereferencing SerialData.ser in __del__. Issue panoptes#194.

jamessynge · 2017-12-18T02:05:54Z

PTAL

wtgee

I'm approving so you are not blocked by me as I think this is good. See comment about self.ser.

wtgee · 2017-12-18T02:32:22Z

pocs/utils/rs232.py

@@ -210,7 +211,7 @@ def __del__(self):
            # If an exception is thrown when running __init__, then self.ser may not have
            # been set, in which case reading that field will generate a NameError.


Update comment to reflect error.

wtgee · 2017-12-18T03:03:20Z

pocs/dome/abstract_serial_dome.py

+        baudrate = int(cfg.get('baudrate', 9600))
+
+        # Setup our serial connection to the given port.
+        self.ser = None


Only other comment was about turning this into self.serial so we have self.serial.ser instead of self.ser.ser, which is more consistent with the mount. Any reason you want to keep it this way?

Add support for an Astrohaven dome.

f57c0a6

Includes a simulator for the PLC (programmable logic controller) that controls the dome's motors.

wtgee requested changes Dec 12, 2017

View reviewed changes

jamessynge mentioned this pull request Dec 13, 2017

Adjust dome API #189

Closed

jamessynge added 2 commits December 13, 2017 08:31

Partial response to review feedback.

606d1db

Respond to review feedback: add doc strings.

11a6080

Also renamed internal methods to have a leading underscore. Tried to be more consistent in describing the Astrohaven protocol (command vs. action).

jamessynge added 7 commits December 13, 2017 17:51

More updating based on feedback to the prior PRs for this work.

a0f1ad7

Adding logging to debug slow performance of pocs/tests/test_astrohave…

fec200d

…n_dome.py::test_open_and_close_slit

wip

c040fe3

Remove extra logging, will work on perf of test_astrohaven_dome later.

3c3948a

Fix error dereferencing SerialData.ser in __del__. Issue panoptes#194.

Clarify some comments. Don't expect dome.disconnect() to return a value.

875ed4d

Merge branch 'develop' into astrohaven3

d0b7e02

wtgee approved these changes Dec 18, 2017

View reviewed changes

jamessynge added 2 commits December 18, 2017 08:47

Merge branch 'develop' into astrohaven3

9aeda42

Update per review feedback.

7057db2

jamessynge merged commit da4d735 into panoptes:develop Dec 18, 2017

jamessynge deleted the astrohaven3 branch December 18, 2017 15:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Astrohaven dome #188

Add support for Astrohaven dome #188

jamessynge commented Dec 12, 2017

wtgee left a comment

wtgee Dec 12, 2017

jamessynge Dec 13, 2017

wtgee Dec 12, 2017

jamessynge Dec 13, 2017

jamessynge Dec 13, 2017

wtgee Dec 12, 2017

jamessynge Dec 13, 2017

wtgee Dec 12, 2017

jamessynge Dec 13, 2017

wtgee Dec 12, 2017

jamessynge Dec 13, 2017

wtgee Dec 12, 2017

jamessynge Dec 13, 2017

jamessynge Dec 13, 2017

wtgee Dec 12, 2017

jamessynge Dec 13, 2017

wtgee Dec 12, 2017

jamessynge Dec 13, 2017

wtgee Dec 12, 2017

jamessynge Dec 13, 2017

codecov bot commented Dec 13, 2017 •

edited

Loading

jamessynge commented Dec 18, 2017

wtgee left a comment

wtgee Dec 18, 2017

wtgee Dec 18, 2017

		@@ -210,7 +211,7 @@ def __del__(self):
		# If an exception is thrown when running __init__, then self.ser may not have
		# been set, in which case reading that field will generate a NameError.

Add support for Astrohaven dome #188

Add support for Astrohaven dome #188

Conversation

jamessynge commented Dec 12, 2017

wtgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 13, 2017 • edited Loading

Codecov Report

jamessynge commented Dec 18, 2017

wtgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 13, 2017 •

edited

Loading