[Prototype] Address Event Callbacks Contain Unusable Parameters Issue#472
[Prototype] Address Event Callbacks Contain Unusable Parameters Issue#472DeborahOoi96 wants to merge 1 commit intoni:masterfrom
Conversation
DeborahOoi96
requested review from
bkeryan,
maxxboehme and
zhindes
as code owners
DeborahOoi96
changed the title
|
Run lint command to pass ci build |
zhindes
left a comment
zhindes
left a comment
There was a problem hiding this comment.
I am really not in the loop on this one. My gut says this looks kind of complicated, but maybe that's necessary. I'm gonna let Brad handle this one for now.
| 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))
|
Closing this old PR. If anybody is interesting in contributing a feature like this, we can open a new one and have a discussion. |
4 participants
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
