Adds flags for ACU checking to hwp supervisor, along with tests #802

jlashner · 2024-12-16T13:34:37Z

Description

Motivation and Context

This adds gripper-state monitoring to the HWP monitor process, and adds the following flags to the HWP state which can be checked by the ACU:

gripper_state which can be "cold", "warm", "ungripped", or "unknown"
is_spinning boolean

How Has This Been Tested?

Along with these changes, I am building out the HWP emulator into a set of integration tests that test the behavior of these flags and other operations.

This has been tested with HWP and ACU hardware on satp1.

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.

for more information, see https://pre-commit.ci

…p-flags-for-acu

for more information, see https://pre-commit.ci

jlashner · 2025-01-03T00:48:12Z

@BrianJKoopman Ended up getting the tests to work properly! Turns out I missed a shutdown call for one of the device_emulators in the hwp emulator. Adding that makes tests clean up correctly, so I reverted the misc changes I made to test the github workflow.

This contains the same update to the HWP supervisor as #805, so if you'd rather just review and merge this one we can close the other one. Thanks!

BrianJKoopman

This is great, awesome to see all the tests come together for the supervisor agent. Mostly small documentation related comments. The SIGKILL workaround for getting the tests to run does mean code coverage doesn't get reported, which is unfortunate. It's makes it a bit challenging to understand what really gets tested.

BrianJKoopman · 2025-01-06T22:51:13Z

socs/testing/device_emulator.py

-        time.sleep(1)
+        # time.sleep(1)


Why drop this? (I kind of forget why it was there, probably to give things a second to wrap up. But if it's not needed anymore, perhaps just remove altogether?)

BrianJKoopman · 2025-01-07T17:24:08Z

socs/agents/hwp_supervisor/agent.py

+class GripperState:
+    instance_id: str
+    limit_warm_grip_state: List[bool] = field(default_factory=lambda: [False, False, False])
+    limit_cold_grip_state: List[bool] = field(default_factory=lambda: [False, False, False])
+    emg: List[bool] = field(default_factory=lambda: [False, False, False])
+    brake: List[bool] = field(default_factory=lambda: [False, False, False])
+    grip_state: Literal['cold', 'warm', 'ungripped', 'unknown'] = 'unknown'
+    last_updated: Optional[float] = None
+    gripper_max_time_since_update: float = 60.0


This could use a docstring to describe the class and its attributes. On initial look I don't know what emg is, for instance.

BrianJKoopman · 2025-01-07T18:11:13Z

socs/agents/hwp_supervisor/agent.py

+                self.is_spinning = None
+            elif now - self.pid_last_updated > self.pid_max_time_since_update:
+                self.is_spinning = None
+            elif self.pid_current_freq > self.pid_current_tolerance:


pid_current_tolerance was a bit confusing to me. Perhaps pid_freq_tolerance would be better?

BrianJKoopman · 2025-01-07T19:14:00Z

socs/agents/hwp_supervisor/agent.py

@@ -1283,8 +1400,13 @@ def get_client(id):
            gripper=get_client(self.args.hwp_gripper_id),
        )

+    def query_hwp_state(self, session, params) -> Tuple[bool, str]:


Needs a docstring.

BrianJKoopman · 2025-01-07T19:19:12Z

socs/agents/hwp_supervisor/agent.py


+            ops = self.hwp_state.update(test_mode=test_mode)
+            session.data['monitored_sessions'] = ops


Related to this change, I think the documented session.data structure might be slightly out of date. The keys now seem to be 'temperature', 'encoder', 'pmx', 'pid', and 'ups'. Can you confirm and update the docstring?

BrianJKoopman · 2025-01-07T19:25:34Z

socs/testing/hwp_emulator.py

+cold_limit_pos = 13.0  # mm
+warm_limit_pos = 10.0  # mm
+limit_tolerance = 1.0  # mm


Make these constants all caps, per PEP8.

BrianJKoopman · 2025-01-07T19:50:58Z

tests/integration/test_scpi_psu_agent_integration.py

@@ -45,6 +47,7 @@ def test_scpi_psu_init_psu(wait_for_crossbar, gpib_emu, run_agent, client):

 @pytest.mark.integtest
 def test_scpi_psu_init_psu_acq_mode(wait_for_crossbar, gpib_emu, run_agent_acq, client):
+    time.sleep(2)


Add a comment for why this is needed.

BrianJKoopman · 2025-01-07T20:23:14Z

tests/integration/test_hwp_pid_agent_integration.py

+    # don't send SIGINT if we've already sent SIGKILL
+    if not runner._timedout:
+        runner.proc.send_signal(signal.SIGKILL)


This SIGKILL interrupts the ability for testing coverage to get reported. With this I get no coverage on the HWP PID agent, but if I remove it I see 95% coverage. Is this just for quicker clean up, or are the tests in CI relying on it to function properly? It runs fine on my system without it (with the kill here turned to an interrupt.) This isn't true for the supervisor test, though it has the same coverage reporting issue.

jlashner and others added 2 commits December 16, 2024 08:30

Adds flags for ACU checking to hwp supervisor, along with tests

f753ebe

[pre-commit.ci] auto fixes from pre-commit.com hooks

285d353

for more information, see https://pre-commit.ci

jlashner mentioned this pull request Dec 16, 2024

Adds kill_on_exit to _AgentRunner in testing simonsobs/ocs#418

Merged

6 tasks

jlashner added 6 commits December 16, 2024 14:11

Try using random port for encoder

03b06da

Merge branch 'hwp-flags-for-acu' of github.com:simonsobs/socs into hw…

a37fc7c

…p-flags-for-acu

Try using random port for encoder

c24f634

Bump ocs version

27fba1c

Patch runner shutdown function with SIGKILL

10f7170

Adds query_hwp_state task to supervisor to force state update

0498c71

jlashner force-pushed the hwp-flags-for-acu branch from 461566b to 0498c71 Compare December 16, 2024 21:02

pre-commit-ci bot and others added 3 commits December 16, 2024 21:02

[pre-commit.ci] auto fixes from pre-commit.com hooks

984a05c

for more information, see https://pre-commit.ci

flake8 and bugfixes

79e0362

bugfixes

cd0d427

jlashner force-pushed the hwp-flags-for-acu branch from 08854be to cd0d427 Compare December 16, 2024 22:25

Place emulator in try/catch

143e679

jlashner force-pushed the hwp-flags-for-acu branch from d2ebe65 to 143e679 Compare December 17, 2024 21:56

mark hwp supervisor tests as integtest

bce28e8

jlashner force-pushed the hwp-flags-for-acu branch from 55a273f to bce28e8 Compare December 17, 2024 22:23

jlashner marked this pull request as ready for review December 17, 2024 22:27

jlashner requested a review from BrianJKoopman December 17, 2024 22:27

test

44196c0

jlashner force-pushed the hwp-flags-for-acu branch from 7feffe5 to 44196c0 Compare December 18, 2024 01:10

test

e355b34

jlashner force-pushed the hwp-flags-for-acu branch from aa77700 to e355b34 Compare December 18, 2024 04:32

check open processes in test

7aa2fcc

jlashner force-pushed the hwp-flags-for-acu branch from 363ea46 to 7aa2fcc Compare December 18, 2024 19:41

Adds psutil to testing reqs

100d412

jlashner force-pushed the hwp-flags-for-acu branch from 6837f47 to 100d412 Compare December 18, 2024 19:45

test finally to close ibootbar outlet

85db2c5

jlashner force-pushed the hwp-flags-for-acu branch from 1742336 to 85db2c5 Compare December 18, 2024 20:26

change sigint to sigkil for snmp tests

d8adce5

jlashner force-pushed the hwp-flags-for-acu branch from 4b6f71a to d8adce5 Compare December 18, 2024 20:38

Join on snmp processes

0e5f9f2

jlashner force-pushed the hwp-flags-for-acu branch from bb0343d to 0e5f9f2 Compare December 18, 2024 20:59

[pre-commit.ci] auto fixes from pre-commit.com hooks

c9a6115

for more information, see https://pre-commit.ci

jlashner mentioned this pull request Dec 19, 2024

Adds flags for ACU safety checks to HWP state #805

Closed

6 tasks

jlashner and others added 8 commits December 20, 2024 12:18

test leftover process detection

7fb69df

Lingering subprocess detector

ec6a331

pytest workflow debugging

41b0efa

Add gripper device shutdown

1105520

[pre-commit.ci] auto fixes from pre-commit.com hooks

ff1df1e

for more information, see https://pre-commit.ci

revert unnecessary changes made during testing

6fcfe28

Add sleep back to scpi test

f48c270

[pre-commit.ci] auto fixes from pre-commit.com hooks

ca3b190

for more information, see https://pre-commit.ci

BrianJKoopman requested changes Jan 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds flags for ACU checking to hwp supervisor, along with tests #802

Adds flags for ACU checking to hwp supervisor, along with tests #802

jlashner commented Dec 16, 2024 •

edited

Loading

jlashner commented Jan 3, 2025

BrianJKoopman left a comment

BrianJKoopman Jan 6, 2025

BrianJKoopman Jan 7, 2025

BrianJKoopman Jan 7, 2025

BrianJKoopman Jan 7, 2025

BrianJKoopman Jan 7, 2025

BrianJKoopman Jan 7, 2025

BrianJKoopman Jan 7, 2025

BrianJKoopman Jan 7, 2025


		ops = self.hwp_state.update(test_mode=test_mode)
		session.data['monitored_sessions'] = ops

Adds flags for ACU checking to hwp supervisor, along with tests #802

Are you sure you want to change the base?

Adds flags for ACU checking to hwp supervisor, along with tests #802

Conversation

jlashner commented Dec 16, 2024 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

jlashner commented Jan 3, 2025

BrianJKoopman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlashner commented Dec 16, 2024 •

edited

Loading