[Prototype] Address Event Callbacks Contain Unusable Parameters Issue #472
Conversation
DeborahOoi96
requested review from
zhindes,
maxxboehme and
bkeryan
as code owners
DeborahOoi96
changed the title
|
Run lint command to pass ci build |
| @@ -760,6 +760,46 @@ def read(self, number_of_samples_per_channel=NUM_SAMPLES_UNSET, | |||
|
|
|||
| return data.tolist() | |||
|
|
|||
| def add_arguments_if_necessary(self, callback_function, expected_number_of_arguments): | |||
| if (callback_function.__code__.co_argcount < expected_number_of_arguments): | |||
There was a problem hiding this comment.
__code__ is a CPython implementation detail and probably won't work with PyPy.
Using the inspect module is probably more portable.
| def add_arguments_if_necessary(self, callback_function, expected_number_of_arguments): | ||
| if (callback_function.__code__.co_argcount < expected_number_of_arguments): | ||
| print("Not enough arguments! Adding arguments") | ||
| if ("every_n_samples_event_type" in parameters): |
There was a problem hiding this comment.
[style] If-statements in Python do not have parens unless they are needed for grouping or splitting across multiple lines. Running poetry run ni-python-styleguide fix will probably fix this.
| result = lambda task_handle, signal_type, callback_data: callback_function(signal_type) | ||
| elif ("task_handle" not in parameters): | ||
| result = lambda task_handle, signal_type, callback_data: callback_function(signal_type, callback_data) | ||
| elif ("callback_data" not in parameters): |
There was a problem hiding this comment.
The last parameter would be useful if the user was allowed to pass in an optional Python object.
There was a problem hiding this comment.
Here's an example of what I mean:
def register_done_event(self, callback_method, callback_data = None):
...
def on_done(task, data):
data.signal()
done_event = threading.Event()
task.register_done_event(on_done, done_event)
It's not critical because Python makes it easy to bind a closure or to pass in a member function as the callback, but it's a potential nice-to-have and it would avoid removing parameters from the signature.
| print("Not enough arguments! Adding arguments") | ||
| if ("every_n_samples_event_type" in parameters): | ||
| result = self.n_samples_event_wrapper(parameters, callback_function) | ||
| elif ("signal_type" in parameters): |
There was a problem hiding this comment.
Every N and signal events pass the callback an enum, but they use a bare ctypes or int type rather than the enum classes from nidaqmx.constants. See my prototype: https://github.com/ni/nidaqmx-python/pull/474/files
callback_method(self, EveryNSamplesEventType(every_n_samples_event_type), number_of_samples)
callback_method(self, Signal(signal_type))
Attempt to address the issue brought up in #143
Made a wrapper in task.py that takes in the callback_function that doesn't have task_handle or callback_data inside, and pass it to wrappers that modify the function to add in the missing functions respectively. This is just a first pass, let me know if you have different thoughts on how to implement this.
Updated functional test to test for the cases when the callback function has missing parameters.
Why should this Pull Request be merged?
Addresses the issue brought up in #143
What testing has been done?
Functional test coverage added and all pass
