added handling of pid error response

bbixler500 commented 1 month ago

Attempts to fix hwp pid error handling issues

Description

Occasionally the hwp pid controller will read an error value which it does not know how to handle, which will crash the monitoring process. This PR adds handling to this specific error code.

Motivation and Context

Issue described here (link)

How Has This Been Tested?

Briefly ran changes on satp1 hardware. Error rarely occurs, so was unable to verify correct handling explicitly.

Types of changes

[x] Bug fix (non-breaking change which fixes an issue)
[ ] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

[x] My code follows the code style of this project.
[ ] My change requires a change to the documentation.
[ ] I have updated the documentation accordingly.

BrianJKoopman commented 4 weeks ago

Added some reviewers. I expect we're all busy preparing for SPIE, but hopefully one of the three of us can review this soon.

jlashner commented 2 weeks ago

Hi @bbixler500, thanks for looking into this. I like your description of the problem here: https://github.com/simonsobs/chwp-discussions/discussions/20#discussioncomment-9610840. Can we add a description like this to the docs of this agent, and maybe a short inline comment in addition?

A few requests:

Can we make this error handling a bit more general? I'd rather the decode function check for the ? char indicating an error, and log a warning with the corresponding error if we recognize it, or "unrecognized error" if we don't.
I'm really not a fan of returning 9.999 on failure and checking this in calling functions. Is it too late to change this to return a "Result" class that contains stuff like success, value, error_msg? Or alternatively, can we just throw an error?

simonsobs / socs