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.

Sign up for GitHub

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

Jump to bottom

Addition of reflector CSC #2

Merged
merged 5 commits into from
Jan 24, 2025
Merged

Addition of reflector CSC #2

merged 5 commits into from
Jan 24, 2025

Conversation

JosephParsons
Copy link
Contributor

No description provided.

@JosephParsons JosephParsons self-assigned this Jun 10, 2024
@JosephParsons JosephParsons requested a review from couger01 July 15, 2024 18:58
couger01
couger01 previously requested changes Jul 17, 2024
View reviewed changes
Copy link
Contributor

@couger01 couger01 left a comment

Choose a reason for hiding this comment

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

Please add a setup.py file using one of the other repos as a template, this is still required for conda recipe version metadata.

@JosephParsons JosephParsons requested a review from couger01 July 18, 2024 22:22
@couger01 couger01 assigned couger01 and unassigned JosephParsons Aug 13, 2024
@couger01 couger01 marked this pull request as draft August 20, 2024 13:11
@couger01 couger01 removed their request for review August 20, 2024 13:17
@couger01 couger01 dismissed their stale review August 20, 2024 13:18

I am now the one to finish the work.

@couger01 couger01 marked this pull request as ready for review January 20, 2025 17:36
@couger01 couger01 requested a review from wvreeven January 20, 2025 17:37
wvreeven
wvreeven requested changes Jan 21, 2025
View reviewed changes
Copy link

@wvreeven wvreeven left a comment

Choose a reason for hiding this comment

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

Looks pretty good but there is an unnecessary duplicate schema definition and I disagree with the LabjackChannel class. For the rest please review all doc strings and error messages and make sure they follow our coding guidelines.

.gitignore Outdated Show resolved Hide resolved
COPYRIGHT Outdated Show resolved Hide resolved
bin/run_mtreflector Show resolved Hide resolved
python/lsst/ts/mtreflector/csc.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/csc.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/controller.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/controller.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/controller.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/controller.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/controller.py Outdated Show resolved Hide resolved
@couger01 couger01 requested a review from wvreeven January 22, 2025 21:56
wvreeven
wvreeven requested changes Jan 23, 2025
View reviewed changes
Copy link

@wvreeven wvreeven left a comment

Choose a reason for hiding this comment

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

Looks a lot better. you missed a few of my earlier comments even though you indicated that you didn't. Also, please improve the code as indicated by my new comments. We're getting there!

python/lsst/ts/mtreflector/controller.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/csc.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/csc.py Outdated Show resolved Hide resolved
wvreeven
wvreeven reviewed Jan 23, 2025
View reviewed changes
Copy link

@wvreeven wvreeven left a comment

Choose a reason for hiding this comment

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

More comments added.

python/lsst/ts/mtreflector/csc.py Outdated Show resolved Hide resolved
wvreeven
wvreeven requested changes Jan 23, 2025
View reviewed changes
Copy link

@wvreeven wvreeven left a comment

Choose a reason for hiding this comment

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

Looks much better. Please make sure to go to FAULT whenever a RuntimeError happens.

python/lsst/ts/mtreflector/controller.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/controller.py Show resolved Hide resolved
python/lsst/ts/mtreflector/controller.py Outdated
async def connect(self) -> None:
"""Connect to the labjack."""
self.configure()
try:
Copy link

Choose a reason for hiding this comment

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

If you move the self.log.exception line to the write_channel method, you can remove the try/exception block.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I still need it for the raising the RuntimeError.

Copy link

Choose a reason for hiding this comment

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

If you

  • remove the try/except block on lines 112 to 115
  • raise RuntimeError in (the current) line 120
    then you can get rid of the try/except in the connect method.

python/lsst/ts/mtreflector/csc.py Outdated Show resolved Hide resolved
python/lsst/ts/mtreflector/csc.py Outdated Show resolved Hide resolved
@couger01 couger01 requested a review from wvreeven January 23, 2025 16:24
wvreeven
wvreeven requested changes Jan 23, 2025
View reviewed changes
Copy link

@wvreeven wvreeven left a comment

Choose a reason for hiding this comment

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

Almost there. If we fix the run and connect methods as I indicated, we're done.

@wvreeven
Copy link

Oh and the CSC still needs to go to FAULT state whenever a RuntimeError happens.

@couger01 couger01 requested a review from wvreeven January 23, 2025 18:07
wvreeven
wvreeven approved these changes Jan 23, 2025
View reviewed changes
Copy link

@wvreeven wvreeven left a comment

Choose a reason for hiding this comment

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

LGTM. Please improve that one error message as per my suggestion.

python/lsst/ts/mtreflector/csc.py Outdated
"""
if self.controller is None:
raise RuntimeError(
"CSC Tried to use reflector controller without a valid object."
Copy link

Choose a reason for hiding this comment

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

Please improve this error message. Something like Reflector controller is None. would be much clearer.

@couger01 couger01 merged commit 06a4ec5 into develop Jan 24, 2025
3 checks passed
@couger01 couger01 deleted the tickets/DM-43456 branch January 24, 2025 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@couger01 couger01 couger01 left review comments

@wvreeven wvreeven wvreeven approved these changes

Assignees

@couger01 couger01

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JosephParsons @wvreeven @couger01