-
Notifications
You must be signed in to change notification settings - Fork 2
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 note and block relative #32
base: develop
Are you sure you want to change the base?
Conversation
How about REQUEST_RELATIVE? request and ground activity allow steps with time relation relative to the START_TIME of the request. Similar to BLOCK_RELATIVE. |
@tloreilly56 does |
I assume BLOCK_RELATIVE is equivalent to FROM_ACTIVITY_START, which is the start of the ground block. FROM_REQUEST_START is relative to the start of the request. Suppose in a request you have two ground activities and the steps within each activity have references to FROM_ACTIVITY_START. The relative time of FROM_ACTIVITY_START is different within each activity, each relative to the start of the two ground blocks in the expansion. |
Aha, thank you. As I understand, SeqJSON doesn't support activities, so a SeqJSON request may only have a flat list of steps and that wouldn't apply unless we added activities. So maybe the question is, do we ever need SeqJSON to support activity expansion? I'm inclined to say no, but maybe it should? |
@cartermak got it. If activity is not going to be supported in seq.json, we don't need to support a time type relative to the activity. For request, we need a time type relative to request start. Naming it REQUEST_START may be less confusing and have BLOCK_START available for later when activity is supported. |
@tloreilly56 Why not use BLOCK_RELATIVE for request now and also be able to use it for activity later if we need to? I don't think we would ever be in a context where are activities and requests, so BLOCK_RELATIVE can mean the start of the request if it's in a request, the start of an activity if it's in an activity, and the start of a sequence if it's in a sequence but not inside a request (like satf). Agree with Carter that I don't expect us to ever support activity block types in seq json, though. |
I understand we never expect to support activity block in seq.json. However, a request in seq.json can have a ground_block step which refers to a ground activity outside seq.json. That ground activity can have FROM_ACTIVITY_START or FROM_REQUEST_START time tag in the steps. I think using BLOCK_RELATIVE for steps in requests to refer to the start of the request is confusing. |
@tloreilly56 The idea is that we can use BLOCK_RELATIVE as a general time tag to capture any time of time tag where the step should be issued relative to its container. The container could be an activity, a request, a sequence, etc. but BLOCK_RELATIVE can be used the same way in each case. In other words, how BLOCK_RELATIVE is used will depend on the context in which it is used. The issue is that if we use REQUEST_RELATIVE, then REQUEST_RELATIVE can only be a valid time tag within request types, and trying to enforce that REQUEST_RELATIVE can only be used for steps inside requests is going to complicate the schema. So we're trying to use a generalized time tag that for now is only going to be used for FROM_REQUEST_START but later could be used for other, similar types of time tags that may come up. More generally, I would like to move away from having everything in SEQ JSON be 1:1 with what was in SATF/SASF Does that make sense? In your example, the request would only have a call to a ground block step, and the ground block would be entirely separate, so BLOCK_RELATIVE should have two distinct meanings in both cases. Also, as you pointed out, we do not plan to support ground blocks in seq json. So the meaning of BLOCK_RELATIVE in seq json should always be clear. |
@shaheerk94 what I was trying to point out is in a ground block you could have a time tag relative to start of the block (BLOCK_RELATIVE) or relative to the start of the request (REQUEST_RELATIVE) that calls the block. Yes, in the request, you could use BLOCK_RELATIVE to represent the start of the request. I just think user might find it confusing using the term BLOCK_RELATIVE to represent relative to the start of request. |
schema.json
Outdated
@@ -361,6 +361,36 @@ | |||
"required": ["offset", "value", "variable"], | |||
"type": "object" | |||
}, | |||
"note": { | |||
"additionalProperties": false, | |||
"description": "Note object used to represent a SEQGEN note construct. Used by SEQGEN to output strings in the simulation results at specific times.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend not tying this documentation to SEQGEN, even though some seq.json features were inspired by SEQGEN. Just describe what note
is intended to do. The seq.json format will support the new seq2.0 tools, which will presumably replace SEQGEN at some point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me know if the new documentation is satisfactory @parkerabercrombie !
}, | ||
{ | ||
"description": "Note step with all possible fields.", | ||
"models": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do these models do in the context of a note
? Can the note reference these values?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The note itself can't reference the models, but the note can be used as an anchor where models are updated. For example, someone may know that some telemetry value is going to be updated to some at some absolute time (not relative to anything else) and they need that value to be updated at that time. They can place a note at that absolute time and attach models to the note to update the telemetry states.
Attendees@shaheerk94 Minutes
|
Some examples to describe the time type behavior. This first example shows how multiple block-relative steps in an absolute-timed request would be evaluated: {
"id": "example",
"metadata": {},
"requests": [
{
"description": "This is a request",
"name": "test_request_1",
"steps": [
{
"description": "This command executes 10s after the request start at 2020-001T00:00:10",
"args": [],
"time": {
"type": "BLOCK_RELATIVE",
"tag": "00:00:10"
},
"stem": "CMD_NO_OP",
"type": "command"
},
{
"description": "This note is evaluated 20s after the request start at 2020-001T00:00:20",
"string_arg": "Note Test.",
"time": {
"type": "BLOCK_RELATIVE",
"tag": "00:00:20"
},
"type": "note"
},
{
"description": "This command executes 30s after the request start at 2020-001T00:00:30",
"args": [],
"time": {
"type": "BLOCK_RELATIVE",
"tag": "00:00:30"
},
"stem": "CMD_NO_OP",
"type": "command"
}
],
"time": {
"tag": "2020-001T00:00:00",
"type": "ABSOLUTE"
},
"type": "request"
}
]
} This second example shows how the timing of these sequence steps would depend on modeling context of the sequence (I'm not sure there's any heritage for this pattern, but one could imagine the modeling software resolving these times to, e.g., absolute time tags for commands): {
"id": "mixed time types",
"metadata": {},
"steps": [
{
"description": "This command executes at 2020-001T00:00:00",
"args": [],
"time": {
"type": "ABSOLUTE",
"tag": "2020-001T00:00:00"
},
"stem": "CMD_NO_OP",
"type": "command"
},
{
"description": "This command executes 10s after the sequence start",
"args": [],
"time": {
"type": "BLOCK_RELATIVE",
"tag": "00:00:10"
},
"stem": "CMD_NO_OP",
"type": "command"
},
{
"description": "This command executes after the previous command completes",
"args": [],
"time": {
"type": "COMMAND_COMPLETE"
},
"stem": "CMD_NO_OP",
"type": "command"
},
{
"description": "This command executes 30s after the previous command start",
"args": [],
"time": {
"type": "COMMAND_RELATIVE",
"tag": "00:00:30"
},
"stem": "CMD_NO_OP",
"type": "command"
},
{
"description": "This command executes 1h after the sequence start",
"args": [],
"time": {
"type": "BLOCK_RELATIVE",
"tag": "01:00:00"
},
"stem": "CMD_NO_OP",
"type": "command"
}
]
} |
Closes #30 by adding new
note
construct andBLOCK_RELATIVE
tag type.Added
note
andBLOCK_RELATIVE
to tests, which are passing.Also updated owner to @cartermak instead of Chris, since Chris is gone, and version.
Closes #24 by conditionally enforcing
tag
field on all time types exceptCOMMAND_COMPLETE