Mount serial fixes by wtgee · Pull Request #388 · panoptes/POCS

wtgee · 2018-01-18T22:04:11Z

This starts to support serial from config (#181) although only has that option for the mount at this point.

Sets a default timeout of 2.0s for serial devices. Sets 0.0 for the serial mounts.

Adds a far-from-complete hardware test for ioptron.

Fixes #376

…-serial-fixes-376

codecov · 2018-01-18T23:49:18Z

Codecov Report

Merging #388 into develop will increase coverage by 0.29%.
The diff coverage is 83.33%.

@@             Coverage Diff             @@
##           develop     #388      +/-   ##
===========================================
+ Coverage    69.05%   69.34%   +0.29%     
===========================================
  Files           58       59       +1     
  Lines         4640     4691      +51     
  Branches       641      650       +9     
===========================================
+ Hits          3204     3253      +49     
- Misses        1264     1265       +1     
- Partials       172      173       +1

Impacted Files	Coverage Δ
pocs/utils/rs232.py	`92.03% <100%> (ø)`	⬆️
pocs/observatory.py	`83.52% <75%> (+0.53%)`	⬆️
pocs/utils/error.py	`92.3% <0%> (-1.7%)`	⬇️
...tests/serial_handlers/protocol_arduinosimulator.py	`69.53% <0%> (-1.57%)`	⬇️
pocs/scheduler/observation.py	`93.44% <0%> (ø)`	⬆️
pocs/utils/horizon.py	`100% <0%> (ø)`
pocs/images.py	`92.52% <0%> (+0.07%)`	⬆️
pocs/scheduler/constraint.py	`96.05% <0%> (+1.6%)`	⬆️
pocs/scheduler/dispatch.py	`98.03% <0%> (+1.96%)`	⬆️

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 70fda07...3d3d706. Read the comment docs.

jamessynge · 2018-01-19T01:35:36Z

+        lat = '{:+07.0f}'.format(self.location.lat.to(u.arcsecond).value)
+        lon = '{:+07.0f}'.format(self.location.lon.to(u.arcsecond).value)

        self.query('set_long', lon)


For another PR, recall that iOptron told us that we can read the GPS location (once available) only if we don't set the lat/long.

Yes, and I believe we were waiting to hear back if that ability could ever be regained once we have set it once. I flashed the firmware on my iEQ45 Pro and tried to wait (via the handset) for the GPS to acquire and it never did.

I didn't ask them that question; the answer we did get sounded definitive: after power-up, don't set lat/long if you want to read the GPS value.

I'll try and experiment with this some more.

jamessynge · 2018-01-19T01:49:01Z

        try:
-            self._port = self.config['mount']['port']
+            serial_config = self.config['mount']['serial']
+            self.serial = rs232.SerialData(**serial_config)


At this point, I begin to wonder whether we should eliminate SerialData and just use pyserial directly... I'm not certain yet.

jamessynge · 2018-01-19T01:49:55Z


        model = mount_info.get('model')
-        port = mount_info.get('port')
+        serial_info = mount_info['serial']


This block of code seems like it all belongs somewhere in pocs/mount/

It does, it's part of the plan to support the dependency injection of the mount. All of the _create_XXX methods will be removed as part of that. I was tempted to start on that with this PR but it would have slowed it down a bunch more.

That's fine, do it later.

jamessynge · 2018-01-19T01:52:06Z

+        self.mount = mount
+
+        with pytest.raises(AssertionError):
+            assert self.mount.query('version') == 'V1.00'


Why is this here?

I'll make a comment, but it's testing that a basic query won't work until initialization happens.

Thanks, I didn't realize that.

jamessynge · 2018-01-19T01:52:34Z

+        assert self.mount.initialize() is True
+
+    def test_version(self):
+        assert self.mount.query('version') == 'V1.00'


Does this fail as soon as we update the firmware on the unit?

Sadly, no. As far as I can tell that version number doesn't actually mean anything or relate to a version. You can do a query('firmware_motor') and query('firmware_radec') to get the actual versions. It's never changed in 4+ years.

Wow. Maybe it represents a protocol identifier?

Yes, I think so. It is park of the mount handshake for serial communication. See ioptron initialize. That's required by the mount.

jamessynge · 2018-01-19T01:53:24Z

+        self.mount.set_park_coordinates()
+        assert self.mount._park_coordinates is not None
+
+        assert self.mount._park_coordinates.dec.value == -10.0


Why these values?

Empirical but should be some comment about them. Will add.

jamessynge · 2018-01-19T01:55:09Z

+    def setup(self, config):

-        self.config = load_config(ignore_local=True)
+        self.config = config


Should we not override the location in the config, maybe even run this test with different locations (Northern & Southern Hemisphere and Equator)?

Then the parking will be messed up. This test should be run when you have an actual mount attached and are wanting to watch what it does so it would in essence be testing your particular configuration.

Ah, good point. So, if using real hardware (not just on PAN001), you want there to be a pocs_local.yaml file with an accurate location of the mount (OR read it from the mount... if the mount can provide that).

But, if not using real hardware, then we can test with several different locations.

jamessynge · 2018-01-19T02:01:23Z

@@ -40,6 +40,7 @@ def __init__(self,
                 port=None,
                 baudrate=115200,


I think we should remove the baudrate, and require that it be provided in the config.

Maybe a more generic PR for #181 to do this? I could do here though.

OK, a more generic PR addressing #181 is fine. I'd be happy to work on that when you're done with this.

jamessynge · 2018-01-19T02:07:16Z

                 port=None,
                 baudrate=115200,
                 name=None,
+                 timeout=2.0,


I was expecting to see that you'd removed most or all of these params, and replaced them with kwargs to be passed to the serial_for_url call, or to the apply_settings() method of the serial object.

Just tried doing this and it will involve a number of other changes not specifically related to the mount. I think a #181 specific PR would be best.

jamessynge · 2018-01-19T02:07:34Z

        self.ser.stopbits = serial.STOPBITS_ONE
-        self.ser.timeout = 2.0  # TODO(jamessynge): Allow caller or config to set.
+        self.ser.timeout = timeout
+        self.ser.write_timeout = timeout


write_timeout of 0 means non-blocking, which might not be quite what you want.

That's what I've been using as a test and has been working great both for the mount and the arduinos. Was going to ask you specifically about this. Not sure what would otherwise be a good default.

Given that our messages are short, infrequent and that we (generally) wait for a response, 0 is probably a fine value for write_timeout.

wtgee added 2 commits January 17, 2018 21:35

Changing mount serial - WIP

a92877c

Merge branch 'develop' of https://github.com/panoptes/POCS into mount…

b1a9727

…-serial-fixes-376

wtgee requested a review from jamessynge January 18, 2018 22:04

Fixing some of the exceptions and tests when not found

a5d0247

jamessynge reviewed Jan 19, 2018

View reviewed changes

jamessynge approved these changes Jan 19, 2018

View reviewed changes

Adding some comments

3d3d706

wtgee merged commit 1e78a29 into panoptes:develop Jan 19, 2018

wtgee deleted the mount-serial-fixes-376 branch January 19, 2018 21:39

wtgee mentioned this pull request Jan 24, 2018

Release 0.6.1 #415

Closed

		@@ -40,6 +40,7 @@ def __init__(self,
		port=None,
		baudrate=115200,

Conversation

wtgee commented Jan 18, 2018

Uh oh!

codecov bot commented Jan 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wtgee Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Jan 18, 2018 •

edited

Loading

wtgee Jan 19, 2018 •

edited

Loading