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 static limit values to LAD tables #5051

Closed
akhenry opened this issue Apr 8, 2022 · 7 comments · Fixed by #7193
Closed

Add static limit values to LAD tables #5051

akhenry opened this issue Apr 8, 2022 · 7 comments · Fixed by #7193
Labels
type:feature Feature. Required intentional design unverified
Milestone

Comments

@akhenry
Copy link
Contributor

akhenry commented Apr 8, 2022

Is your feature request related to a problem? Please describe.
In the case where static limit values are defined by a limit provider for a given telemetry point, our LAD tables should display those limit values. Currently static limit values are rendered as a limit line in plots, this would bring the same concept to LAD tables.

Describe the solution you'd like
Telemetry limit values should be displayed as additional columns in the LAD table

Additional context
There is a broader desire to bring all of the customizability of telemetry tables to LAD tables. If considered as part of that change, the new limit columns could be optionally shown or hidden by the user.

@akhenry akhenry changed the title Add limits to LAD tables Add static limit values to LAD tables Apr 8, 2022
@akhenry
Copy link
Contributor Author

akhenry commented Oct 10, 2023

Let's make the limit column show by default IF LIMITS ARE DEFINED for any of the telemetry points. We need to make sure it works with the column show/hide controls in the inspector.

@akhenry akhenry added this to the Target:3.2.0 milestone Oct 10, 2023
@scottbell
Copy link
Contributor

I've a got first draft here, but could use some design help from @charlesh88 and/or @akhenry:
Screenshot 2023-10-16 at 4 06 47 PM

We've got two values (high and low) for each limit that needs to be displayed and creating a string that combines the together. Do we instead want separate columns for high and low?

Also I could use some advice about limits in general. As telemetry items could have different classes (e.g., one could have "CRITICAL", and another "HIGH"), I'm currently taking the union of them all for column names. Is this correct?

Thanks for the help!

@unlikelyzero unlikelyzero added type:feature Feature. Required intentional design and removed type:enhancement labels Oct 27, 2023
@scottbell scottbell self-assigned this Oct 30, 2023
@scottbell
Copy link
Contributor

@unlikelyzero did you mean to unassign this?

@scottbell
Copy link
Contributor

This happened awhile back, but @charlesh88 & @akhenry were good with both the union of limits in the table, and the string combining the high/low values into a string (as LAD tables aren't sortable).

@scottbell scottbell linked a pull request Oct 31, 2023 that will close this issue
15 tasks
@scottbell
Copy link
Contributor

To test:

  1. Create a LAD table and a mixture of objects to it with limits and without limits, namely a Sine Wave Generator and an Event Message Generator.
  2. Ensure you can see various limits in the table.
  3. Edit the LAD table
  4. In the inspector, hide the various columns and ensure they work.
  5. Refresh the page, ensure the columns you selected remain hidden.
  6. Remove the Sine Wave Generator from the LAD table
  7. Ensure the LAD table no longer has Units or limit columns
  8. Edit the LAD table
  9. Ensure the Units & Limits columns aren't there.

@unlikelyzero
Copy link
Collaborator

unlikelyzero commented Oct 31, 2023

@scottbell let's sync on testing this as a feature on quickstart

  1. Conversion.
    a.Convert a vanilla LAD Table to one with static limit lines. Remove Static Limit Line definitions and verify the LAD Table still functions
    b. Add static limit lines to a quickstart telemetry point. Use limit override script to override. Which limits should be observed? Remove limit override. Do static limits remain?
    c. The same but with limits defined as XTCE
  2. Limit values
    a. Specifically should be able to copy and paste from another cell
  3. Backwards compatible with JPL deployments. Need to Sync with @davetsay
  4. Quickstart telemetry types

What we wouldn’t want to waste time on are safeguards for making sure the limits make sense. (lower limit greater than upper limit validation)

@unlikelyzero unlikelyzero added the bug:retest Retest without a specific milestone label Mar 20, 2024
@charlesh88
Copy link
Contributor

charlesh88 commented Mar 20, 2024

Testathon 2024-03-20 limits from a sine wave generator display as expected.

@charlesh88 charlesh88 removed the bug:retest Retest without a specific milestone label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature Feature. Required intentional design unverified
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants