Removes subclassing for ControlStates

[jlashner]

Bugfixes that are related to subclassing dataclasses

Description

This fixes the same problem that @bbixler500 addresses in #657, however does it by replacing the inheritance of the Base class with the addition of a ControlStateInfo class that manages general state metadata such as start and last update time, and the state-name.

Motivation and Context

This fixes issues in the current code where having a dataclass subclass a non-dataclass object will overwrite the init function, leaving fields undefined.

I think Bryce's method of using kw_only=True is a valid fix, but only works python >=3.10.

Since the mechanics of combining dataclasses and inheritance is not fully clear to me, I think its generally better to use this method even after python3.10, since the inheritance is more prone to error.

How Has This Been Tested?

I added a hwp-supervisor unit test that exposes the existing bug, and it is passes with the new code. If @bbixler500 or @ykyohei could test on one of the sat platforms that would be ideal though.

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.

[bbixler500]

I just tested this bug fix with the satp1 hwp. I needed to rename every instance of cur_state_info to cur_state to get the code to run (there are various instances of cur_state in the code which become undefined without this renaming). Other than the aforementioned issue, the bug fix resolved the dataclass inheritance issue.

[jlashner]

I'm a bit confused, where did you need to rename cur_state_info back into cur_state? I thought I caught all the references and searching through the code can't find any.

[bbixler500]

Specifically lines 620, 621, 634, 715, and 716 (basically I just reverted the changes that were made to rename cur_state to cur_state_info in the PR). Looking through the code for an example of where cur_state is referenced and not changed, I can see the line return action.success, f"Completed with state: {action.cur_state}" at the end of most supervisor functions.

[jlashner]

Ah ok, yes I did miss those return statements.

I changed cur_state -> cur_state_info because the object that's being stored in it has changed from just the State object to the StateInfo object (which contains State + metadata). Instead of reverting those name changes we want to replace cur_state with cur_state_info.state... I'll commit that change

[jlashner]

@bbixler500 if you could re-test this next time you have the chance that would be great. Thanks for catching that!

[bbixler500]

Just tested the fix on satp1 and everything ran without issue (so long as I used docker images that had older definitions of main/acq)

[BrianJKoopman]

Just tested the fix on satp1 and everything ran without issue (so long as I used docker images that had older definitions of main/acq)

Does this mean we still need #657 wrapped up for newer images to work?

[bbixler500]

Does this mean we still need #657 wrapped up for newer images to work?

Yes, but only the portions which change the naming of 'acq' to 'main'

[jlashner]

I can add those changes to this PR.

[jlashner]

Are we currently waiting on anything for this? Do we need to test again with the latest commit, or are we just waiting for an approval from @BrianJKoopman?

[bbixler500]

I think we just waiting for approval. I tested the most recent changes without issue

[BrianJKoopman]

Looks good to me, just one docstring change needed.

[BrianJKoopman]

Add arg to docstring.

[BrianJKoopman]

Looks good, thanks!