Improve SPM eclipse enable state

[taldcroft]

Description

This refactors the handling for the sun_pos_mon state into three separate states. This was needed in order to ensure that continuity is correct for any state query start time.

Fixes #289

Interface impacts

Adds new states battery_connect and eclipse_enable_spm.

Testing

Unit tests

• Mac

Independent check of unit tests by Jean

• Linux

Functional tests

Created (in a notebook) a new validator class:

class ValidateSunPosMon(ValidateStateCode):
    state_name = "sun_pos_mon"
    msids = ["aopssupm"]
    plot_attrs = PlotAttrs(title="Sun position monitor", ylabel="Sun position monitor")
    min_violation_duration = 300

    def add_exclude_intervals(self):
        super().add_exclude_intervals()
        self.exclude_ofp_intervals_except(["NRML"])

    @cached_property
    def state_vals(self):
        """Convert ENAB (commanded states) to ACT (telemetry).

        The "ENAB" is an artifact of the backstop history sun position monitor states.
        This method is otherwise equivalent to the ValidateStateCode method.
        """
        states_interp = interpolate_states(self.states, self.tlm["time"])
        state_vals = states_interp[self.state_name]
        state_vals[state_vals == "ENAB"] = "ACT"
        state_vals_raw = convert_state_code_to_raw_val(state_vals, self.state_codes)
        return state_vals_raw

With this the SPM transitions for a 200 day span are mostly OK:

>>> vp = ValidateSunPosMon("2023:200", days=200)
>>> viols = vp.get_violations()
>>> print(viols)
        start                  stop        
--------------------- ---------------------
2023:045:04:46:49.525 2023:045:04:52:17.525  # Safe mode
2023:049:03:38:31.067 2023:049:04:12:08.267  # Safe mode
2023:056:21:59:54.110 2023:056:22:06:11.310  # Real diff between expected penumbra exit and actual?

[jeanconn]

This speaks to a lack of knowledge on my part, but for a non-eclipse transition, are the battery_connect and eclipse_enable_spm keys relevant?

[taldcroft]

This tells the code that if you are asking for (e.g.) battery_connect that it should include this class in the processing that looks for potential state-changing commands since battery_connect and sun_pos_mon are coupled in some way. I'm honestly not 100% sure if this is needed but it's generally a good idea for states that work in tandem.

[jeanconn]

OK, that's fine. It seemed not necessary in passing but also seems like it would be benign if not really needed (and is tested this way as fine).

[jeanconn]

Testing looks good and code makes sense to me from a "use states and continuity in a sane way to manage these things that are tricky" perspective.

[jeanconn]

Related to "With this the SPM transitions for a 200 day span are mostly OK" -- what knobs would we have to twiddle to in the command google sheet if we ended up with a mismatch or wanted to fix a "mostly ok" that wasn't based on the code being incorrect?

[jeanconn]

Oh, and I guess I shouldn't have Approved with an outstanding Flake 8 issue and it still probably should have that comment fixed.

[jeanconn]

I'll reset my review to "request changes" - I had that one question about relevant states and this needs flake8 update and maybe a fix to that comment.

[taldcroft]

Related to "With this the SPM transitions for a 200 day span are mostly OK" -- what knobs would we have to twiddle to in the command google sheet if we ended up with a mismatch or wanted to fix a "mostly ok" that wasn't based on the code being incorrect?

In the Exclude Intervals sheet of the Chandra Command events spreadsheet, we would add interval(s) to exclude with the key sun_pos_mon.

[taldcroft]

@jeanconn - I think I addressed your comments.