-
Notifications
You must be signed in to change notification settings - Fork 51
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
Auto-generation support for daqmx streaming apis #1123
base: main
Are you sure you want to change the base?
Conversation
source/codegen/service_helpers.py
Outdated
"""Get the list of output streaming parameter that needs to be defined.""" | ||
params = [] | ||
output_params = [p for p in parameters if common_helpers.is_output_parameter(p)] | ||
for param in output_params: |
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.
Nit: the list comprehension for selecting output_params
is much cleaner than the loop for selecting streaming params. Using list comprehensions and helper methods makes a big difference:
def is_streaming_param_except_name(param, name_to_avoid_for_some_reason):
# TODO why avoid name_to_avoid_for_some_reason?
param.get("is_streaming_type", False) and param["name"] != name_to_avoid_for_some_reason
def get_output_streaming_params_to_define(parameters, streaming_param) -> List[dict]:
"""Get the list of output streaming parameter that needs to be defined."""
output_params = [p for p in parameters if common_helpers.is_output_parameter(p)]
return [p for p in output_params if is_streaming_param(p, streaming_param["name"])]
Actually, rewriting this, I realize I don't understand this code. We're avoiding one specific streaming parameter? Why is it not sufficient to look for is_output_parameter
and is_streaming_type
? Is streaming_param
named correctly?
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 reason behind avoiding that specific parameter is that it is the 'first' streaming parameter (eg: readarray), here we want to define other params which will be part of the response message for streaming apis. (eg: samp_per_chan_read).
And yes, it should be named as "first_streaming_param
" to avoid confusion.
Modified to use list comprehensions and a helper method and renamed the parameter.
What does this Pull Request accomplish?
This PR adds support for auto-generation of DAQmx streaming APIs.
Why should this Pull Request be merged?
custom_proto.mako
which defines protobuf message definitions to encapsulate different data types for DAQmx apis.service_helpers.mako
to auto-generate the streaming function bodies.service_helpers.py
andcommon_helpers.py
to generate the correct function structure for streaming apis.nifpga_service.cpp
.What testing has been done?
I am able to build the server code without any errors and able to use the client call and call the server with all the new streaming APIs.