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

Mdb overrides subscription #333

Merged
merged 14 commits into from
Jul 14, 2023
Merged

Mdb overrides subscription #333

merged 14 commits into from
Jul 14, 2023

Conversation

shefalijoshi
Copy link
Collaborator

@shefalijoshi shefalijoshi commented Jun 14, 2023

Closes VIPEROMCT-295

Describe your changes:

Adds the subscribeToLimits API to the realtime provider
This subscribes to the type mdb-changes and returns any limits that have changed.

All Submissions:

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Is this change backwards compatible? For example, developers won't need to change how they are calling the API or how they've extended core plugins such as Tables or Plots.

Author Checklist

  • Changes address original issue?
  • Tests included and/or updated with changes?
  • Command line build passes?
  • Has this been smoke tested?
  • Testing instructions included in associated issue OR is this a dependency/testcase change?

Reviewer Checklist

  • Changes appear to address issue?
  • Reviewer has tested changes by following the provided instructions?
  • Changes appear not to be breaking changes?
  • Appropriate automated tests included?
  • Code style and in-line documentation are appropriate?
  • Has associated issue been labelled unverified? (only applicable if this PR closes the issue)
  • Has associated issue been labelled bug? (only applicable if this PR is for a bug fix)

Copy link
Collaborator

@jvigliotta jvigliotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Nice! I've got a couple small questions/suggestions.

@unlikelyzero unlikelyzero added this to the Target:2.2.6 milestone Jun 21, 2023
@shefalijoshi shefalijoshi requested a review from jvigliotta June 22, 2023 03:12
@shefalijoshi
Copy link
Collaborator Author

shefalijoshi commented Jun 22, 2023

Testing Instructions:

  • Create an overlay plot in Open MCT
  • Add an endpoint that has no limits setup to this overlay plot
  • Edit the overlay plot and enable limit lines
  • Using the yamcs API, create an alarm at runtime for the endpoint above (For Quickstart use yamcs version >5.8.2)
Example: 
http://0.0.0.0:8090/api/mdb-overrides/<yamcs-instance>/realtime/parameters/<parameter-name>
{
    action: SET_DEFAULT_ALARMS,
    defaultAlarm: {
        minViolations: 1,
        staticAlarmRange: [
            {
            level: WATCH,
            minInclusive: 18,
            maxInclusive: 26
            }
        ]
    }
}
  • The overlay plot in Open MCT should now show the new limits drawn.
  • Remove the limit or change it using the yamcs API
  • The overlay plot in Open MCT should now show the change.

Copy link
Collaborator

@jvigliotta jvigliotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I have one more question that I think may be an existing issue in other subscription providers as well (definitely main realtime provider).

Copy link
Owner

@akhenry akhenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just needs some tests. The same tests can verify both nasa/openmct#6735 and this.

@shefalijoshi shefalijoshi requested a review from akhenry July 7, 2023 17:25
Copy link
Owner

@akhenry akhenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have done some testing in our test environment. The changes work for catching limit changes immediately as they happen via subscription, but they do not resolve mdb overrides on load.

The limit-provider needs to also make a call to https://github.com/akhenry/openmct-yamcs/pull/333/files#diff-4a950de5fe10da4e7dff6cde433a8e4e90c4a1b606c6bb3919b79661c3302997L101 to resolve any MDB overrides on plot load.

@shefalijoshi shefalijoshi requested a review from akhenry July 13, 2023 18:45
Copy link
Collaborator

@jvigliotta jvigliotta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@@ -100,12 +101,13 @@ test.describe("Quickstart network requests @yamcs", () => {
// wait for debounced requests in YAMCS Latest Telemetry Provider to finish
await new Promise(resolve => setTimeout(resolve, 500));
filteredRequests = filterNonFetchRequests(networkRequests);

console.log(filteredRequests);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if you can remove this, we should. Not a big deal to print diagnostic to screen in tests

@akhenry akhenry merged commit c18b88c into master Jul 14, 2023
@unlikelyzero unlikelyzero deleted the mdb-overrides-subscription branch July 17, 2023 00:41
@jvigliotta
Copy link
Collaborator

jvigliotta commented Jul 20, 2023

Verified (Overlay Plots) - Testathon - 7/20/23
Looks great! I was able to set the limits using our script. The limits show up in the Overlay Plot where I set them. I can update the limits and they update in realtime without a refresh. I'm also able to enable and disable with the expected results.

Not sure if it's working for gauges or if that was a requirement. Not seeing anything there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants