nasa / openmct

A web based mission control framework.
https://nasa.github.io/openmct/
Other
11.99k stars 1.24k forks source link

is the plot feature compatible with new API? #1684

Closed klepsydra-technologies closed 7 years ago

klepsydra-technologies commented 7 years ago

Dear all,

after playing around with the plots as per in the tutorial with the new telemetry format, I could not figure out two things:

  1. How to use the units and names from the telemetry values in the plot. It seems that the plot always takes the default value for the labels and units. This seems to be related to the fact that the plot is looking for ranges field in the telemetry metadata, while now the field passed is called values.

  2. I also cannot make the plot to display more than one serie. Similar problem I think.

Could you please tell me if this is related to the new API work in progress or if not, is there somewhere an example of how to achieve these things? The tutorial examples are not working either.

Thanks,

Pablo.

VWoeltjen commented 7 years ago

Thanks for the report - that's a significant regression. These should still be working with the new API.

I haven't fully diagnosed or understood the problem yet, but I am seeing some errors on console when I try to plot sine wave generators:

TypeError: Cannot read property 'forEach' of undefined
    at valueMetadatasFromOldFormat (http://localhost:8080/src/api/telemetry/TelemetryMetadataManager.js:38:25)
    at new TelemetryMetadataManager (http://localhost:8080/src/api/telemetry/TelemetryMetadataManager.js:124:35)
    at TelemetryAPI.getMetadata (http://localhost:8080/src/api/telemetry/TelemetryAPI.js:308:17)
    at ImageryViewPolicy.hasImageTelemetry (http://localhost:8080/platform/features/imagery/src/policies/ImageryViewPolicy.js:44:47)
    at ImageryViewPolicy.allow (http://localhost:8080/platform/features/imagery/src/policies/ImageryViewPolicy.js:51:25)
    at PolicyProvider.allow (http://localhost:8080/platform/policy/src/PolicyProvider.js:125:36)
    at allow (http://localhost:8080/platform/policy/src/PolicyViewDecorator.js:46:38)
    at Array.filter (native)
    at PolicyViewDecorator.getViews (http://localhost:8080/platform/policy/src/PolicyViewDecorator.js:50:60)
    at ViewCapability.invoke (http://localhost:8080/platform/core/src/views/ViewCapability.js:53:37) <mct-representation key="'switcher'" mct-object="domainObject" ng-model="representation" class="ng-pristine ng-untouched ng-valid ng-isolate-scope">

That this is occurring in the TelemetryMetadataManager makes me suspicious that it is the root cause of at least Item 1 from your report. git blame shows no recent changes to TelemetryMetadataManager that would concern me; I'll do a git bisect to find out more. Of recent pull requests, #1655 looks most likely to have introduced this

VWoeltjen commented 7 years ago

Digging in a little more:

image

I'm going to work through the metadata issue first, since that should be easy to find with a bisect at this point.

VWoeltjen commented 7 years ago
$ git bisect skip
There are only 'skip'ped commits left to test.
The first bad commit could be any of:

347fb6d8822393f85fb5cbe45685e00ca539ac75 3bd556a406306ebe5f97c3f783d751110f0af8e0 b3cf7a5d932ffc3467ef1b2949c1e4491944ca72 c1d6e21c3ca9d559c91d29a18e63eeadaf777918

We cannot bisect more!

Well hm!

VWoeltjen commented 7 years ago

Confirming your first point, the problem does appear to be that legacy views do not have full access to metadata from non-legacy telemetry objects. This takes me back to suspecting an issue with metadata conversion. For sake of simplicity, I'd like to fix this in the adapter layer (which will also help any other legacy views), but this is a good prompt for us to consider refactoring the plot to use new APIs directly.

VWoeltjen commented 7 years ago

Downgrading from blocker to critical; the displayed data is still correct, but lacking useful context.

VWoeltjen commented 7 years ago

1698 addresses the problem with the name not showing

Filed #1697 for units in plots. This has been a feature request for a while and has been implemented in at least a prototype; I'm a little surprised we aren't tracking this already.

@klepsydra-robotics I'm not able to reproduce the problem with displaying multiple series in a single plot. I'd be curious to see if the changes from #1698 resolve this issue for you; if not, would you mind filing a followup?

Thanks again for the bug report!

klepsydra-technologies commented 7 years ago

Dear @VWoeltjen , thanks for the responses. Do I need to wait for the merge to test the fix #1698 ? I'll check out the sine wave generators (as explained here? https://github.com/nasa/openmct/blob/ec4fe8efb39e4decb869edde9c6c61393c32ec6d/docs/src/tutorials/index.md)

Thanks,

Pablo.

VWoeltjen commented 7 years ago

You can wait for the merge, or you could checkout that branch (git checkout plot-metadata-1684) directly and test with that.

Sine Wave Generators are available as an example plugin. There is an example of this being enabled in index.html: openmct.install(openmct.plugins.Generator());. Once installed, you should see Sine Wave Generator under the Create menu.

(Also, apologies if the threaded responses created a lot of notifications for you. I have a habit of using issue comments to keep notes as I work, in case I get interrupted for some reason.)

klepsydra-technologies commented 7 years ago

Dear @VWoeltjen , I have just finishes testing the branch as you suggested. It actually does show the units and labels correctly, so thanks a lot for that.

I noticed another difference though. When looking at the individual telemetry in the gui, in the tutorial appear as plots while in the branch they appear as telemetry tables.

Once composed as telemetry panel, yes it all works fine. But I saw this difference just in case it is some kind of bug.... not sure.

Let me know if you want me to send you screenshots or something.

Thanks,

Pablo.