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

Allow a LabVIEW Service to return DoubleAnalogWaveform data and create example. #515

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ccaltagi
Copy link
Collaborator

What does this Pull Request accomplish?

  • This change allows a LabVIEW service to return a DoubleAnalogWaveform in its outputs. DoubleAnalogWaveform is defined as matching the ni_protobuf_types_DoubleAnalogWaveform typedef that we are now shipping. This was generated from the waveform.proto file. If it is a cluster of that type, the parameter type will be TypeMessage and the message type will be ni.protobuf.types.DoubleAnalogWaveform.
    The main change that allows the above support is the modification to Source/Runtime/MeasurementLink Measurement Server/Helpers/Is Supported Cluster.vi to allow the support of the new data types.

  • As part of this support, we need to provide conversion helper VIs to convert back and forth between the LabVIEW Waveform data type and the corresponding autogenerated gRPC type.
    After talking with @jasonmreding, we agreed on creating an Extensions.lvlib that will contain those conversion VIs.
    I added that code in the Source/gRPC/Extensions folder and modified the Source/Build Specs/ni_protobuf_types.vipb package builder to install them with the protobuf types.

  • Modified the generator template files for Measurement Configuration and Results to add the new waveform datatype as both an input and output.

  • Creation of a Waveform Measurement service to showcase the new measurement type.
    I tested the new service both with a LabVIEW client, and InstrumentStudio with a measui file, since I haven't made the changes yet to the G Plugin to support LabVIEW UIs for the Waveform data type.

image

Why should this Pull Request be merged?

  • Allows LabVIEW services to return DoubleAnalogWaveform data similar to how C# already can.
  • Demonstrates a LabVIEW measurement service returning XYData.

What testing has been done?

Tested the new example with both a LabVIEW client and InstrumentStudio using a measui file.

Copy link
Collaborator

@dixonjoel dixonjoel left a comment

Choose a reason for hiding this comment

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

I'd like to see 3 PRs here

  1. Fix the ni_protobuf_types.vipb to include the waveform types and conversion VIs.
  2. Update the generator template to demonstrate the waveform data type with some hard-coded (output only) data like a sine wave and a graph on the UI
  3. The Waveform Measurement example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I built the MeasurementLink Service .vip and installed it into LV 2021. Then I tried to build the ni_protobuf_types.vipb and I got these errors. Did you have a way to build / install these locally to test? It seems to me like you need to update MeasurementLink Service.vipb, build it and make sure it's installing these files so we can then build ni_protobuf_types.vipb. But then, this seems like a bad dependency chain. I think ni_protobuf_types.vipb should be able to build on its own without any other MeasurementLink packages being installed.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because of this, I'm unable to get a good setup to try out your example. I think we should make a separate PR to get all of the .vipb changes in place and then add the example after that's all ready.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the PR doesn't include the graphical diffs, can you show what's changed in this VI with a screenshot?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess just adding this case?
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see this new .lvlib file, but I don't see it being added to any .lvproj file. @jasonmreding should we add this to the ni_protobuf_types .lvproj?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it makes sense to add these to the configuration and results typedefs unless you're also going to update the Measurement Logic.vi and Measurement UI.vi to show their usage. This way when users generate their initial measurement from the generator, they'll see how to use a waveform in the measurement all the way through the UI.

Also, for Double XY Data, we didn't put it in the Measurement Configuration.ctl since the main usage was for outputs. We just hard-coded some interesting data (fibonacci sequence) in the Measurement Logic.vi. I think we should do something similar for waveform.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Since you haven't done the changes to the G Plugin to support waveforms in LV UIs, I don't think this measurement will work as is. We should check this in at a point when it's fully working. So you should either do the G Plugin changes first and ensure that the LabVIEW UI works with that version or use a .measui (I think it can do waveforms already?) when you check this in.

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

Successfully merging this pull request may close these issues.

2 participants