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

Default argument values for bridge create channel functions are unusable #482

Closed
zhindes opened this issue Feb 1, 2024 · 5 comments
Closed
Assignees

Comments

@zhindes
Copy link
Collaborator

zhindes commented Feb 1, 2024

Impacted functions:

  • ai_channel_collection
    • add_ai_force_bridge_polynomial_chan
    • add_ai_force_bridge_table_chan
    • add_ai_pressure_bridge_polynomial_chan
    • add_ai_pressure_bridge_table_chan
    • add_ai_torque_bridge_polynomial_chan
    • add_ai_torque_bridge_table_chan

For the Polynomial functions, these parameters are bad:

  • forward_coeffs=None
  • reverse_coeffs=None

For the Table functions, these parameters are bad:

  • electrical_vals=None
  • physical_vals=None

Those 4 parameters must not be NULL when passed to the underlying C API and give an error indicating as such. The new default values should match those used in LabVIEW, which effectively give them the same behavior of the Two-Point Linear functions (e.g. add_ai_force_bridge_two_point_lin_chan) that already exist and are in good shape. I think that looks like this, but we should double check each impacted function to be sure.

forward_coeffs=[0.0, 1.0], reverse_coeffs=[0.0, 1.0]

electrical_vals=[-1.0, 0.0, 1.0], physical_vals=[-100.0, 0.0, 100.0]
@bkeryan
Copy link
Collaborator

bkeryan commented Feb 1, 2024

The LV VIs map 2 mV/V to 100 lbs, psi, or lb-in, so we should use these defaults:

forward_coeffs=[0.0, 50.0], reverse_coeffs=[0.0, 0.02]

electrical_vals=[-2.0, 0.0, 2.0], physical_vals=[-100.0, 0.0, 100.0]

image
image
image

@bkeryan
Copy link
Collaborator

bkeryan commented Feb 1, 2024

BTW, the parameter list specifies a default value of None, but the body of the function changes the default value:

        if forward_coeffs is None:
            forward_coeffs = []

        if reverse_coeffs is None:
            reverse_coeffs = []

This was probably done because specifying a default value of [] in the parameter list is often a bug, since the empty array literal is shared between all calls to the function.

@charitylxy
Copy link
Collaborator

Hi @zhindes @bkeryan, While looking through grpc-device-scrapigen, I noticed this lines of code is always setting default value of mutable object (list, in our case) as None when generating the metadata.

Looking further into why it's being done this way, I found out that mutable objects are often set to None in function parameters to avoid unexpected behavior. In Python, default parameter values are evaluated only once, when the function is defined. If the default value is a mutable object (like a list or a dictionary) and we modify it, the modified object will be used as the default value in subsequent function calls which will lead to unexpected behavior (https://betterprogramming.pub/the-problem-with-mutable-types-as-default-values-in-functions-in-python-81ca88ff4d91)

Currently the default value in metadata for the impacted functions are set as None. If we want to modify the default value to the functions' parameter, my approach would be either:

  1. Update daqmxAPISharp.json and remove the code in extract_functions.py to generate new functions.py metadata.
  2. Modify the default value at transformfunctions.py to generate new functions.py metadata.

However given the situation above, do we still want to proceed to setting default value to the parameter for the impacted functions above? One of the alternative I could think of is to set the default in the body of the function.

        if forward_coeffs is None:
            forward_coeffs = [0.0, 1.0]

        if reverse_coeffs is None:
            reverse_coeffs = [0.0, 0.02]

However, I'm not too sure how I can specifically change to the respective default values since each of the function are generated from function_template.py, unless we make those functions handwritten.

Appreciate your input on this, thankyou!

@bkeryan
Copy link
Collaborator

bkeryan commented Feb 20, 2024

The quick and dirty fix is to have https://github.com/ni/nidaqmx-python/blob/master/src/codegen/templates/function_template.py.mako use a helper function to look up the correct value based on func and input_param.

<%
def get_list_default_value(func, param):
     if func.function_name.startswith("add_ai"):
        if param.parameter_name == "forward_coeffs":
            return "[0.0, 1.0]"
        elif param.parameter_name == "reverse_coeffs":
            return "[0.0, 0.02]"
        elif param.parameter_name == "electrical_vals":
            return "[whatever]"
        ...
        else:
            raise RuntimeError("oh no")
    else:
        return "[]"
%>\
        %if input_param.is_list:
        if ${input_param.parameter_name} is None:
            ${input_param.parameter_name} = ${get_list_default_value(func, input_param)}

        %endif

@charitylxy
Copy link
Collaborator

Issue is fixed in #509, hence closing this.

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

No branches or pull requests

3 participants