Skip to content

Conversation

@bbixler500
Copy link
Contributor

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

  • 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:

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

@jlashner
Copy link
Contributor

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
Copy link
Contributor

ykyohei commented Apr 22, 2024

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
Copy link
Contributor Author

bbixler500 commented Apr 22, 2024

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
Copy link
Contributor

ykyohei commented Apr 22, 2024

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.

Copy link
Member

@BrianJKoopman BrianJKoopman left a comment

Choose a reason for hiding this comment

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

This doesn't seem completely superseded by #658. The fixes to the process names in the PMX and PID agents would be good to keep. Can you drop the kw_only related changes and keep the other fixes?

@BrianJKoopman
Copy link
Member

Okay, as of 60462bb this is superseded by #658. Closing.

@BrianJKoopman BrianJKoopman deleted the hwp_supervisor_satp1 branch May 1, 2024 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants