Bug fixes to get hwp_supervisor working for satp1

bbixler500 commented 2 months ago

Modifications to the hwp_supervisor agent to get it working with the most recent docker images

Description

Renamed the pid monitoring process to main from acq Changed the structure of the ControlState.Base() class to allow for proper inheritance

Motivation and Context

When testing with the most recent docker images, the process spin_control continually crashes with the error spin_control:1 CRASH: [Failure instance: Traceback: <class 'AttributeError'>: 'Idle' object has no attribute 'start_time'. Upon investigation the cause was found to be because of improper inheritance of the base class ControlState.Base()

How Has This Been Tested?

This was tested by running the agent on daq-satp1 and calling the functions: pid_to_freq, brake, pmx_off, enable_driver_board, and disable_driver_board

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.
[x] I have updated the documentation accordingly.

jlashner commented 2 months ago

Ah thanks for this bryce... I see this bug has always been there, but this commit I added after kyohei's test seemed to introduce the call that raises the error. @ykyohei I wonder why you're not currently seeing this issue... is it possible you're still running a dev version of the supervisor?

This looks good to me, but it looks like one of the tests is failing because the dataclass kw_only kwarg was added to the decorator in python 3.10, but the tests are running on python3.8. Can we make it so that it sets kw_only for each field individually? Then I'd be happy to merge. Thanks!!

ykyohei commented 2 months ago

Actually, satp3 also saw this problem. We updated the image today, and immediately encountered this issue, the test I did for latest PR was not good. Sorry about that. This fix is also helpful for satp3.

bbixler500 commented 2 months ago

One additional comment that I have is that the brake method might need to be changed for satp1. The connection to the satp1 pcu currently has high latency, with the first command sent sometimes taking up to a minute to register (subsequent commands only take a fraction of a second, so the issue seems to be in the initial setup of the connection). I haven't investigated the high latency too much at this point but the latency is likely due to the usb-to-fiber converters that satp1 uses (as well as other fiber interfaces between the platform and c2). What this means is that there is some time between when the pmx is powered off and when the pcu changes to "stop" mode. In that time the hwp accelerates a little due to the stray voltage on the driver board. timedelta

ykyohei commented 2 months ago

satp3 also has high latency for pcu commands (tens of seconds). I don't think this is due to the different usb configuration. Probably we can improve the latency by improving the implementation of send_command of pcu agent. It's great to improve brake/pcu, but I would like it to be addressed in a separate PR.

BrianJKoopman commented 2 months ago

Okay, as of https://github.com/simonsobs/socs/pull/658/commits/60462bbf14e6dab3a27ba92a362cce6e3b6e0609 this is superseded by #658. Closing.

simonsobs / socs