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

Starcamera - first PR request for testing starcamera at the LAT #335

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
262 changes: 262 additions & 0 deletions socs/agents/starcam_lat/agent.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,262 @@
import argparse
import socket
import struct
import time
from os import environ

import txaio
from ocs import ocs_agent, site_config
from ocs.ocs_twisted import TimeoutLock


class StarcamHelper:

"""
CLASS to control and retrieve data from the starcamera

Args:
ip_address: IP address of the starcamera computer
port: port of the starcamera
"""

def __init__(self, ip_address, port, timeout=10):
self.ip = ip_address
self.port = port
self.server_addr = (self.ip, self.port)
self.comm = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
self.comm.connect(self.server_addr)
self.comm.settimeout(timeout)

def pack_and_send_cmds(self):
"""
pack_and_send_cmds()

**Process**
packs commands and parameters to be sent to starcamera and sends

**Return**
returns list of values sent
"""
Copy link
Member

Choose a reason for hiding this comment

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

This syntax looks like it's trying to be for the agent process docstring. It should instead be formatted more like:

"""Packs commands and parameters to be sent to star camera and sends.

Returns:
    list: Values sent to star camera.
"""

Here's a good example of docstring formatting that I find myself constantly referencing: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html#example-google

This also applies to the docstrings for the rest of the methods in this class.

Choose a reason for hiding this comment

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

see commit 336557b

logodds = 1e8
latitude = -22.9586
longitude = -67.7875
height = 5200.0
exposure = 700
timelimit = 1
set_focus_to_amount = 0
auto_focus_bool = 1
start_focus = 0
end_focus = 0
step_size = 5
photos_per_focus = 3
infinity_focus_bool = 0
set_aperture_steps = 0
max_aperture_bool = 0
make_HP_bool = 0
use_HP_bool = 0
spike_limit_value = 3
dynamic_hot_pixels_bool = 1
r_smooth_value = 2
high_pass_filter_bool = 0
r_high_pass_filter_value = 10
centroid_search_border_value = 1
filter_return_image_bool = 0
n_sigma_value = 2
star_spacing_value = 15
values = [logodds,
latitude,
longitude,
height,
exposure,
timelimit,
set_focus_to_amount,
auto_focus_bool,
start_focus,
end_focus,
step_size,
photos_per_focus,
infinity_focus_bool,
set_aperture_steps,
max_aperture_bool,
make_HP_bool,
use_HP_bool,
spike_limit_value,
dynamic_hot_pixels_bool,
r_smooth_value,
high_pass_filter_bool,
r_high_pass_filter_value,
centroid_search_border_value,
filter_return_image_bool,
n_sigma_value,
star_spacing_value]
# Pack values into the command for the camera
self.cmds_for_camera = struct.pack('ddddddfiiiiiiiiiifffffffff',
*values)
# send commands to the camera
self.comm.sendto(self.cmds_for_camera, (self.ip, self.port))
print("Commands sent to camera")
# Return the list of values
return values

def get_astrom_data(self):
"""
get_astrom_data()

**Process**
receives and unpacks data from camera

**Return**
returns dictionary of unpacked data
"""
(scdata_raw, _) = self.comm.recvfrom(224)
Copy link
Member

Choose a reason for hiding this comment

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

224 is an unusual bufsize. This is the maximum amount of data to be received at once, and it's generally recommended to be a relatively small power of 2. Can we make it 256 instead?

data = struct.unpack_from("dddddddddddddiiiiiiiiddiiiiiiiiiiiiiifiii",
scdata_raw)
keys = ['c_time',
'gmt',
'blob_num',
'obs_ra',
'astrom_ra',
'obs_dec',
'fr',
'ps',
'alt',
'az',
'ir',
'astrom_solve_time',
'camera_time']
# Create a dictionary of the unpacked data
astrom_data = [data[i] for i in range(len(keys))]
astrom_data_dict = {keys[i]: astrom_data[i] for i in range(len(keys))}
return astrom_data_dict

def close(self):
"""
close()

**Process**
closes the socket of the connection
"""
self.comm.close()


class StarcamAgent:

def __init__(self, agent, ip_address, port):
self.agent = agent
self.active = True
self.log = agent.log
self.job = None
Comment on lines +133 to +135
Copy link
Member

Choose a reason for hiding this comment

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

self.active and self.job seem leftover from the agent this one was based on. Remove them. See #335 (comment).

self.take_data = False
self.lock = TimeoutLock()
agg_params = {'frame_length': 60}
self.agent.register_feed("starcamera", record=True,
agg_params=agg_params, buffer_time=1)
try:
self.StarcamHelper = StarcamHelper(ip_address, port)
Copy link
Member

Choose a reason for hiding this comment

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

Attribute names should be lower_case_with_underscores, so self.starcam_helper = StarcamHelper(ip_address, port). I'd probably even just go with self.starcam or self.star_camera.

except socket.timeout:
self.log.error("Starcamera connection has times out")
return False, "Timeout"
Copy link
Member

Choose a reason for hiding this comment

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

It would still be useful to separate this part into an init operation that can be called from an ocs client (as hinted at in #335 (comment).) That would allow users to re-initialize the connection to the star camera without needing to completely restart the agent. That structure is in many of the other agents, one good example is the CryomechAgent:

@ocs_agent.param('auto_acquire', default=False, type=bool)
def init(self, session, params=None):
"""init(auto_acquire=False)
**Task** - Initializes the connection to the PTC.
Parameters:
auto_acquire (bool): Automatically start acq process after
initialization if True. Defaults to False.
"""
if self.initialized:
return True, "Already Initialized"
with self.lock.acquire_timeout(0, job='init') as acquired:
if not acquired:
self.log.warn("Could not start init because "
"{} is already running".format(self.lock.job))
return False, "Could not acquire lock."
# Establish connection to ptc
self.ptc = PTC(self.ip_address, port=self.port,
fake_errors=self.fake_errors)
# Test connection and display identifying info
try:
self.ptc.get_data()
except ConnectionError:
self.log.error("Could not establish connection to compressor.")
return False, "PTC agent initialization failed"
print("PTC Model:", self.ptc.model)
print("PTC Serial Number:", self.ptc.serial)
print("Software Revision is:", self.ptc.software_revision)
self.initialized = True
# Start data acquisition if requested
if params['auto_acquire']:
resp = self.agent.start('acq', params={})
self.log.info(f'Response from acq.start(): {resp[1]}')
return True, "PTC agent initialized"

I'd be happy to help with this, and to simultaneously set this up to use the new TCPInterface base class for the TCP connection to the star camera, as that will help make the agent robust against network issues.


@ocs_agent.param('_')
def send_commands(self, session, params=None):
"""
send_commands()

**Process**
packs and sends camera+astrometry-related commands to starcam

**Return**
returns a touple with True/False and a string describing whether
or not a lock could be acquired and commands were sent to the sc
"""
Copy link
Member

Choose a reason for hiding this comment

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

The syntax of this is still a bit off. Full example can be found here. It should look like:

"""send_commands()

**Process** - Pack and send camera and astrometry related commands to the star camera.

"""

We don't include the Returns: section in our agent operation docstrings.

with self.lock.acquire_timeout(job='send_commands') as acquired:
if not acquired:
self.log.warn(f"Could not start Task because "
f"{self._lock.job} is already running")
return False, "Could not acquire lock"
self.log.info("Sending commands")
self.StarcamHelper.pack_and_send_cmds()
return True, "Sent commands to starcamera"

@ocs_agent.param('_')
def acq(self, session, params=None):
"""
acq()

**Process**
acquires data from starcam and publishes to feed

**Return**
once the acq() loop exits (wherein data is retrieved from
the camera and pulished), a touple with True/False and a string
describing whether or not the loop was exited after the end of
an acquisition.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Reformat like demonstrated above in the send_commands docstring.

if params is None:
params = {}
Comment on lines +172 to +173
Copy link
Member

Choose a reason for hiding this comment

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

These two lines aren't needed, remove.

with self.lock.acquire_timeout(timeout=100, job='acq') as acquired:
if not acquired:
self.log.warn("Could not start init because {} is already "
Copy link
Member

Choose a reason for hiding this comment

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

This should read "Could not start acq because {} is already ". See #335 (comment).

"running".format(self.lock.job))
return False, "Could not acquire lock"
session.set_status('running')
self.log.info("Starting acquisition")
self.take_data = True
while self.take_data:
data = {
'timestamp': time.time(),
'block_name': 'astrometry',
'data': {}
}
# get astrometry data
astrom_data_dict = self.StarcamHelper.get_astrom_data()
# update the data dictionary+session and publish
data['data'].update(astrom_data_dict)
session.data.update(data['data'])
Copy link
Member

Choose a reason for hiding this comment

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

The session.data object's structure needs to be documented in the docstring for this operation. See documentation on that here: https://ocs.readthedocs.io/en/main/developer/agent_references/documentation.html#session-data

self.agent.publish_to_feed('starcamera', data)

return True, 'Acquisition exited cleanly'

def _stop_acq(self, session, params):
ok = False
if self.take_data:
session.set_status('stopping')
self.take_data = False
ok = True
# self.StarcamHelper.close()
Copy link
Member

Choose a reason for hiding this comment

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

Can remove this comment. We don't want to close the TCP connection when stopping acq, since as written there's no way to re-establish that connection without restarting the agent.

return (ok, {True: 'Requested process to stop',
False: 'Failed to request process stop.'}[ok])


def add_agent_args(parser=None):
if parser is None:
parser = argparse.ArgumentParser()
pgroup = parser.add_argument_group('Agent Options')
pgroup.add_argument("--ip-address", type=str,
help="IP address of starcam computer")
pgroup.add_argument("--port", default="8000", type=int,
help="Port of starcam computer")
return parser


def main(args=None):
# for logging
txaio.use_twisted()
txaio.make_logger()

# start logging
txaio.start_logging(level=environ.get("LOGLEVEL", "info"))

parser = add_agent_args()
args = site_config.parse_args(agent_class="StarcamAgent", parser=parser)
agent, runner = ocs_agent.init_site_agent(args)
starcam_agent = StarcamAgent(agent, ip_address=args.ip_address,
port=args.port)
agent.register_task('send_commands', starcam_agent.send_commands,
startup=True)
agent.register_process('acq', starcam_agent.acq, starcam_agent._stop_acq)
runner.run(agent, auto_reconnect=False)


if __name__ == '__main__':
main()