Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SASF "note" equivalent field and SASF "FROM_REQUEST_START" equivalent time tag #30

Open
2 tasks
shaheerk94 opened this issue Dec 5, 2024 · 23 comments · May be fixed by #32
Open
2 tasks

Add SASF "note" equivalent field and SASF "FROM_REQUEST_START" equivalent time tag #30

shaheerk94 opened this issue Dec 5, 2024 · 23 comments · May be fixed by #32
Assignees

Comments

@shaheerk94
Copy link

shaheerk94 commented Dec 5, 2024

Related problems
Psyche SASFs make extensive use of "notes" and "FROM_REQUEST_START" time tags which are not supported in SEQ JSON today. Other missions that use SASF likely will need to use those fields as well (JUNO uses both fields as well).

Describe the feature request

  • Add a new element to the JSON schema for note support. Notes are a type of step that can be included in either sasfs or satfs. Notes should be a step type at the same level as command, ground_event, and ground blocks that always takes a single STRING argument and can have metadata, description, models, etc.
  • Add a new element to the JSON schema for FROM_REQUEST_START time tag support.

Open question: What are the right strings to use for note and FROM_REQUEST_START? There's precedence both for renaming legacy keywords from sasf/satf in SEQJSON and renaming legacy keywords. Some time tags have been renamed (FROM_PREVIOUS_START -> COMMAND_RELATIVE, ABSOLUTE -> ABSOLUTE, WAIT_PREVIOUS_END -> COMMAND_COMPLETE).

I propose using "note" to match SASF (as opposed to renaming) and "BLOCK_RELATIVE" instead of "FROM_REQUEST_START" to match the style of COMMAND_RELATIVE. Calling it BLOCK_RELATIVE allows it to be used outside of request contexts.

SASF Example of note+FROM_REQUEST_START:

request(BKG_comment_007, REQUESTOR, "SEQ",
            START_TIME, 2024-219T01:12:39.187445,
            PROCESSOR, "VC2A",
            KEY, "NO_KEY")
        note(1, SCHEDULED_TIME, 00:00:00.000000, FROM_REQUEST_START,
            TEXT,"Earliest possible comm_end"
        ),
    end;
@shaheerk94 shaheerk94 changed the title Add new "note" field and FROM_REQUEST_START time tag Add new "note" field and "FROM_REQUEST_START" time tag Dec 5, 2024
@shaheerk94 shaheerk94 changed the title Add new "note" field and "FROM_REQUEST_START" time tag Add SASF "note" equivalent field and SASF "FROM_REQUEST_START" equivalent time tag Dec 5, 2024
@cartermak
Copy link
Member

Could you include an example SASF (just a snippet is fine) that shows the request format in context?

@shaheerk94
Copy link
Author

Could you include an example SASF (just a snippet is fine) that shows the request format in context?

Sure, added to top comment.

@parkerabercrombie
Copy link

@shaheerk94 how do you picture FROM_REQUEST_START working in seq.json? If I understand correctly, SASF can include multiple requests, each of which can contain multiple steps. In the context of seq.json which only supports a single sequence per file, does this mean time measured from the start of the sequence?

@shaheerk94
Copy link
Author

That's a good question. In the legacy formats, FROM_REQUEST_START is only valid within sasf request structures. So we might also restrict REQUEST_RELATIVE in seq json such that it's only an allowed time tag within requests. That might be hard to implement via the schema, though.

The other easier option would be to allow it everywhere, and let it mean what @parkerabercrombie which is relative to the start of the sequence. This wouldn't be the first instance of SEQ JSON allowing something that isn't allowed in the legacy formats - SEQJSON allows multiple models to be attached to a single step which isn't supported by SASF or SATF.

@parkerabercrombie
Copy link

So we might also restrict REQUEST_RELATIVE in seq json such that it's only an allowed time tag within requests.

Not sure I understand what you have in mind. How will the request be represented in seq.json?

@shaheerk94
Copy link
Author

Requests currently are represented in seq json like so:

  "requests": [
    {
      "description": "Absolute-timed request object with all possible fields",
      "name": "test_request1",
      "steps": [
        {
          "stem": "FAKE_COMMAND1",
          "time": { "tag": "00:00:01.000", "type": "COMMAND_RELATIVE" },
          "type": "command",
          "args": []
        }
      ],
      "time": { "tag": "2020-173T20:00:00.000", "type": "ABSOLUTE" },
      "type": "request"
    },
    {
      "description": "Ground-epoch timed request object with all possible fields",
      "ground_epoch": { "delta": "+00:30:00", "name": "test_ground_epoch" },
      "name": "test_request1",
      "steps": [
        {
          "stem": "FAKE_COMMAND1",
          "time": { "tag": "00:00:01.000", "type": "COMMAND_RELATIVE" },
          "type": "command",
          "args": []
        }
      ],
      "type": "request"
    }
  ]

@shaheerk94
Copy link
Author

So in other words, if we restrict REQUEST_RELATIVE, it would only be allowed as a time tag that is attached to a step that is in a step array within a "request" type object. I don't off the top of my head know if that's even possible with seq json schemas - it would require the time tag object to have knowledge of the nature of its grandfather

@cartermak
Copy link
Member

To make sure I follow - the goal is to achieve something like this?

  "requests": [
    {
      "name": "test_request1",
      "steps": [
        {
          "stem": "FAKE_COMMAND1",
          "time": { "tag": "00:00:01.000", "type": "REQUEST_RELATIVE" },
          "type": "command",
          "args": []
        }
      ],
      "time": { "tag": "2020-173T20:00:00.000", "type": "ABSOLUTE" },
      "type": "request"
    }
  ]

@shaheerk94
Copy link
Author

That's right!

@cartermak
Copy link
Member

What about BLOCK_RELATIVE instead of REQUEST_RELATIVE, and then that time type is valid for both requests and regular sequences? Missions may choose to make it invalid in their context (looking at FCPL sequencing), but it could still be valid in the case of a sequence wanting to express a step relative to sequence start time. Thoughts? Is that overloading the term "block" too much?

IMO, we should avoid making the constructs (and their names) unnecessarily restrictive. If we can add a new time tag which could mean something outside requests, and if it's a lot of work to scope it to only requests, then we can just make it apply more broadly?

@shaheerk94
Copy link
Author

I think that's a great idea. Even if FCPL doesn't support it, it seems very reasonable that a future mission may have a time tag that's "relative to sequence start". I agree that block is overloaded already, but I'm struggling to think of a better idea... maybe BATCH_RELATIVE?

@cartermak
Copy link
Member

Actually...do any existing definitions of "block" match this context? I think the set of steps in a request, or a full onboard sequence, both qualify as things we already call "block"?

I'm happy to pick up a new term (and it might contribute to our "multiple sequences per file" discussion), but curious whether "block" actually fits.

@cartermak
Copy link
Member

I think of "batch" as a processing mode, less so a static collection, but that could be me.

@shaheerk94
Copy link
Author

We've actually been trying to move away from "block" in seq2.0 terminology. We've been using "onboard subroutine" instead. So right now, "block" is only defined in legacy VML contexts and not defined in seq 2.0 land. So I think BLOCK_RELATIVE might actually be okay. I could be onboard with BLOCK_RELATIVE if we can't think of a better name (it's also nice that conceptually it's not that far away from how block has been used before)

@parkerabercrombie
Copy link

Ah, I missed that there was already a requests element in the schema. I'm in favor adopting a general purpose name rather than making it specific to requests. I agree with Carter that "batch" makes me think of batch processing, so I prefer "block" (and can't think of a better word right now).

@shaheerk94
Copy link
Author

I'm onboard with block as well. Let's do that. I updated my top comment.

@shaheerk94
Copy link
Author

Are you all onboard with "print" for "note"? @parkerabercrombie @cartermak

@cartermak
Copy link
Member

Is there a rationale for renaming it and not sticking with "note"? I think either is fine, but again just hunting for what's the most 1) accurate and 2) broadly applicable.

@shaheerk94
Copy link
Author

I think sticking with note is a fine option as well. The main driver for renaming it that I see is that if we want to this new field to support any type of print-message-to-simulation function that other command simulators might also use, if we plan to use SEQ JSON with simulator besides SEQGEN. @dandelany had some thoughts on why we might rename it as well.

@dandelany
Copy link
Collaborator

Thanks @shaheerk94 - I think after fresh reconsideration, I have a slight preference for note instead of print. If the proposal is to have the field be a step[n].type, a step type of note feels more consistent with the other types, such as command, ground_block and ground_event, and with how I think about types in general. On the other hand, we also have verb types load and activate in steps...

I was talking to @cartermak on Slack about this and he summed it up well:

I think the higher-level design question is whether this is a directive to do something, or simply a declaration in the sequence that some tool can do whatever with ... IMO the latter is the more generalized answer, but I can see value in having a dedicated “print” directive type

I agree with this, and prefer the more generalized "note declaration" rather than "print directive", but can see it either way & ultimately fine with either.

@shaheerk94
Copy link
Author

I'm onboard with that as well. Let's go with the more generalized "note declaration", if @cartermak and @parkerabercrombie are onboard.

@cartermak
Copy link
Member

I'm onboard 👍

@cartermak
Copy link
Member

@shaheerk94 go ahead and open a PR with the changes as discussed here and we can collect a final sign-off from everyone.

@shaheerk94 shaheerk94 linked a pull request Dec 12, 2024 that will close this issue
@dandelany dandelany added this to Aerie Dec 17, 2024
@github-project-automation github-project-automation bot moved this to Todo in Aerie Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

6 participants