Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a 'dome' attribute to Observatory. #231

Merged

Conversation

jamessynge
Copy link
Contributor

Add logger kwarg to create_dome_from_config, enabling caller
to provide logger to be used.

Add logger kwarg to create_dome_from_config, enabling caller
to provide logger to be used.
@@ -488,6 +498,8 @@ def _create_mount(self, mount_info=None):
Note:
This does not actually make a serial connection to the mount. To do so,
call the 'mount.connect()' explicitly.
TODO(jamessynge): Discuss this claim with Wilfred. SerialData automatically
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The claim is wrong as you point out and should be updated.

@@ -30,10 +26,10 @@ def create_dome_from_config(config):
else:
brand = dome_config.get('brand')
driver = dome_config['driver']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This already made it's way in, but this will fail if dome is in config but driver is not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps that was intended but if so we should catch and warn.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. This is certainly a fatal error in terms of creating the dome instance, and will most likely keep POCS from working as intended. Probably we should just raise an exception.

Add TODO about non-bisque mounts, which assumes all are serial.
…ng called,

causing an exception because a str can't be called.
@codecov
Copy link

codecov bot commented Dec 23, 2017

Codecov Report

Merging #231 into develop will decrease coverage by 0.19%.
The diff coverage is 75%.

Impacted file tree graph

@@            Coverage Diff            @@
##           develop    #231     +/-   ##
=========================================
- Coverage       80%   79.8%   -0.2%     
=========================================
  Files           42      42             
  Lines         3170    3204     +34     
  Branches       412     415      +3     
=========================================
+ Hits          2536    2557     +21     
- Misses         495     503      +8     
- Partials       139     144      +5
Impacted Files Coverage Δ
pocs/observatory.py 86.28% <66.66%> (-0.41%) ⬇️
pocs/dome/__init__.py 91.89% <83.33%> (+0.22%) ⬆️
pocs/dome/astrohaven.py 78.66% <0%> (-1.34%) ⬇️
pocs/utils/images.py 49.07% <0%> (-0.77%) ⬇️
pocs/utils/database.py 93.47% <0%> (-0.47%) ⬇️
pocs/utils/rs232.py 93.61% <0%> (-0.33%) ⬇️
pocs/dome/protocol_astrohaven_simulator.py 75.94% <0%> (-0.06%) ⬇️
pocs/utils/config.py 90.47% <0%> (ø) ⬆️

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 86dafaa...4a9d321. Read the comment docs.

@jamessynge jamessynge merged commit 160f3f2 into panoptes:develop Dec 23, 2017
@jamessynge jamessynge deleted the issue-199-add-Observatory.dome branch December 23, 2017 23:13
@@ -128,6 +135,9 @@ def status(self):
status['mount']['mount_target_ha'] = self.observer.target_hour_angle(
t, self.mount.get_target_coordinates())

if self.dome:
status['dome'] = self.dome.status
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this has already been merged, but it's a little odd that status is a property here but a method for everything else. Why not make the dome status a method?

@wtgee wtgee mentioned this pull request Dec 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants