-
Notifications
You must be signed in to change notification settings - Fork 13
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, Karen! Sorry it took us a while to get back to you; since your initial request, we've restructured socs
so PR's are now made to merge into main
instead of develop
.
I changed a lot of the key structure of your star camera agent branch to make it compatible with the updated socs
. This way, the comments are more related to the agent itself. As a result, when you go to make updates to this agent, be sure to pull from the starcamera branch first to make sure you have the up-to-date version of this branch.
A couple notes were made on the extra files pushed to the branch.
One major note that isn't present in the comments below: you'll also need to write documentation for your agent so that it's available to see on the socs readthedocs page.
socs/agents/starcam/agent.py
Outdated
params = {} | ||
with self.lock.acquire_timeout(timeout=100, job='init') as acquired: | ||
if not acquired: | ||
self.log.warn("Could not start init because {} is already running".format(self.lock.job)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also need to change init
in this message to acq
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks @karenperezsarmiento! And thanks @sanahabhimani for the thorough review!
I've commented on some points that Sanah brought up, and also have a structural question about the way the task and process interact to command the starcamera.
socs/agents/starcam/agent.py
Outdated
|
||
def send_commands(self, session, params=None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parts needed for an initialization task are in lines 101-104, where the starcam_Helper
class (the class that sets up the socket connection to the hardware) is initiated.
socs/agents/starcam/agent.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to confirm the usage of this agent, as the task/process structure seems a bit unusual to me. The send_commands()
task runs when you call it in a client to actually send the commands to the star camera, and the acq()
process runs continuously to receive the response from the starcamera.
Do you keep acq()
running and only call send_commands()
when you need to take a set of photos with the camera? Is there always a response to be receiving in get_astrom_data
? What happens if you start acq()
but never call send_commands()
?
What I would have expected was either:
- The
acq()
process sent commands each time before trying to receive a response withget_astrom_data()
, and then you likely don't need a separate task, unless running a manual trigger is useful. - The
send_commands()
task also received the response after sending commands. This would be just a manual version of theacq()
process essentially.
I'm not sure which of these two makes more sense, as I'm not familiar with the actual use of the camera. But the current implementation seems like sort of a hybrid since commands are sent by the task and the response received in the process.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks so much @karenperezsarmiento, @sanahabhimani, @BrianJKoopman for all your thorough work on this! going to try addressing all these comments in the coming days since the starcam will start becoming super useful in just another few months:)
to address your last comment, brian, your first paragraph above is spot on. i believe send_commands()
is called upon startup to set various camera+astrometry parameters. these params are fed to the starcam computer and set things like exposure time (and other camera params like autofocusing), and lat/long (which affect its ability to find an Az El pointing solution). the camera software should be running a program that takes these parameters and is continuously taking pictures, finding pointing solutions, and delivering packets of info to the agent via get_astrom_data()
(at a cadence determined by the exposure time + the time it takes for a pointing solution to be found in software). if send_commands()
were to not be called, there should be defaults set locally for each of these parameters in this starcam software that would be used in their stead.
thanks again!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to address your last comment, brian, your first paragraph above is spot on. i believe
send_commands()
is called upon startup to set various camera+astrometry parameters. these params are fed to the starcam computer and set things like exposure time (and other camera params like autofocusing), and lat/long (which affect its ability to find an Az El pointing solution). the camera software should be running a program that takes these parameters and is continuously taking pictures, finding pointing solutions, and delivering packets of info to the agent viaget_astrom_data()
(at a cadence determined by the exposure time + the time it takes for a pointing solution to be found in software). ifsend_commands()
were to not be called, there should be defaults set locally for each of these parameters in this starcam software that would be used in their stead.
I see, so send_commands()
is only called at startup? Is it required every time the agent connects to the star camera? Or only if the star camera was rebooted?
…ocs/socs/agents/starcam_lat in agent.py: - changed class names from starcam_Helper and starcam_Agent to StarcamHelper and StarcamAgent - combined pack_cmds() and send_cmds() into single function: pack_and_send_cmds() - in pack_and_send_cmds(), added all values to a list (named values) packed and sent said commands added a return values - in get_astrom_data(), added a list of keys for a dictionary made dictionary from unpacked data added a return for this dictionary - in acq(), replace dictionary definition given the changes to get_astrom_data() changed job = 'init' to job = 'acq' - in add_agent_args() removed defult ip address changed --user-port to --port change all instances of parser_in to parser moved import statement to top of file - in main() removed startup=True added txaio commands for logging and import txaio at top of file - changed doc strings across file - insterted ocs param decorator above send_commands() - changed latitude, longitude, and height params in send_commands() to reflect chilean coords
001a752
to
3fc8d8b
Compare
for more information, see https://pre-commit.ci
…tarcamera changes: - reduced line lengths to satisfy pep8 rules -- some variable names changed to accomodate this change
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @AlexManduca, sorry I haven't had a chance to review your recent changes in detail yet, but there are a couple of things to address first anyway. In addition to the comment below the pre-commit checks are failing due to invalid characters in the code, see details here: https://results.pre-commit.ci/run/github/186511668/1732120479.uB-orrk5RL2nfbGl2oGNXg
(This link is linked at the bottom of the PR if you click "details" next to "pre-commit.ci - pr" in the checks section.)
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good, thanks @AlexManduca. A few more comments to address below.
Another general comment is we need a page for this agent in the documentation. That will live in socs/docs/agents/
. A template for this page can be found in the ocs docs. Feel free to reference any of the existing pages too. Once you create a page, please add it to the index here as well.
socs/agents/starcam_lat/agent.py
Outdated
""" | ||
pack_and_send_cmds() | ||
|
||
**Process** | ||
packs commands and parameters to be sent to starcamera and sends | ||
|
||
**Return** | ||
returns list of values sent | ||
""" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see commit 336557b
**Return** | ||
returns dictionary of unpacked data | ||
""" | ||
(scdata_raw, _) = self.comm.recvfrom(224) |
There was a problem hiding this comment.
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?
self.active = True | ||
self.log = agent.log | ||
self.job = None |
There was a problem hiding this comment.
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).
socs/agents/starcam/agent.py
Outdated
""" | ||
if params is None: | ||
params = {} | ||
with self.lock.acquire_timeout(timeout=100, job='init') as acquired: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The timeout is for acquiring the lock. So if another operation was running when you call acq()
, it would wait 100 seconds before giving up. 100 seconds is unusually long to wait, I would recommend lowering this. When calling the other method the client should just .wait()
for it to finish. If acq
still can't acquire the lock it'll error out and the caller should handle that accordingly.
params = {} | ||
with self.lock.acquire_timeout(timeout=100, job='acq') as acquired: | ||
if not acquired: | ||
self.log.warn("Could not start init because {} is already " |
There was a problem hiding this comment.
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).
if params is None: | ||
params = {} |
There was a problem hiding this comment.
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.
socs/agents/starcam/agent.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to address your last comment, brian, your first paragraph above is spot on. i believe
send_commands()
is called upon startup to set various camera+astrometry parameters. these params are fed to the starcam computer and set things like exposure time (and other camera params like autofocusing), and lat/long (which affect its ability to find an Az El pointing solution). the camera software should be running a program that takes these parameters and is continuously taking pictures, finding pointing solutions, and delivering packets of info to the agent viaget_astrom_data()
(at a cadence determined by the exposure time + the time it takes for a pointing solution to be found in software). ifsend_commands()
were to not be called, there should be defaults set locally for each of these parameters in this starcam software that would be used in their stead.
I see, so send_commands()
is only called at startup? Is it required every time the agent connects to the star camera? Or only if the star camera was rebooted?
socs/agents/starcam_lat/agent.py
Outdated
try: | ||
self.StarcamHelper = StarcamHelper(ip_address, port) | ||
except socket.timeout: | ||
self.log.error("Starcamera connection has times out") | ||
return False, "Timeout" |
There was a problem hiding this comment.
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:
socs/socs/agents/cryomech_cpa/agent.py
Lines 50 to 91 in 9242ab2
@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.
self.agent.register_feed("starcamera", record=True, | ||
agg_params=agg_params, buffer_time=1) | ||
try: | ||
self.StarcamHelper = StarcamHelper(ip_address, port) |
There was a problem hiding this comment.
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
.
session.set_status('stopping') | ||
self.take_data = False | ||
ok = True | ||
# self.StarcamHelper.close() |
There was a problem hiding this comment.
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.
Docstrings are now consistently formatted according to the 'Example Google Style Python Docstrings' document found at the following URL: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/ex ample_google.html#example-google Several different ways of writing "starcam" (e.g. "starcamera", "star camera", etc.) made references to the instrument name inconsistent. All instances have been changed to a succinct "starcam". One or two typos were also fixed, and periods were added where appropriate. Resolves:
Description
The latest version of the starcamera agent is able to connect to the device, receive telemetry/astrometry data and save it to the correct registries.
Motivation and Context
Agent is capable to connecting to device and receiving telemetry data.
How Has This Been Tested?
The agent has been tested with the starcamera system connected but dark (test run not done on real sky).
Types of changes
Checklist:
develop
branch.