-
Notifications
You must be signed in to change notification settings - Fork 99
Add mechanism to allow subscribing to Driver Warning events #2099
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -120,6 +120,28 @@ class SelfTestError(Error): | |
|
||
|
||
% endif | ||
class DriverWarningEvent: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The DriverWarning is already being used as the name for the class which houses the details of warning thrown by the driver(inherits native python Warning class). I guess "Event" here is only conveying that the action is going to be taken only in the event of a driver warning. Maybe we can have the name of the class as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you guys invent this whole thing from scratch or did you derive from another NI driver or something else? I'm wondering about terminology, etc. but not if it introduces inconsistency across our drivers or even Python conventions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea of warning event is derived from RFSG and RFSA drivers' .NET API experience (explained here). Implementation was just based on expected behavior. I do not have a single reference as such from other python modules, but it was sort of the approach taken generally wherever folks have implemented a event handler in python. |
||
'''Event handler for driver warnings.''' | ||
|
||
def __init__(self): | ||
self.subscribers = [] | ||
|
||
def subscribe(self, callback): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. Will change this to say |
||
"""Subscribe to warning events.""" | ||
if callback not in self.subscribers: | ||
self.subscribers.append(callback) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do you support subscribing the same thing N times? what is the use case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Current implementation does not restrict the users to have just one callback function. Went with this approach mostly to match the RFSA/G .NET API behavior. I do not see a reason for restricting as such, however I do not have a known use case for this either. This could enable handling different warnings in their own way by defining different callbacks and registering all of them. Gives a flexibility that potentially can lead to a cleaner user application. |
||
|
||
def unsubscribe(self, callback): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is it ok to unsubscribe a callback function that was not subscribed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the current implementation, yes it is ok for the users to do this. Thought process was to not be strict on this regard since the result of unsubscription or not being notified again occurs anyways. Will add a unit test for this behavior. |
||
"""Unsubscribe from warning events.""" | ||
if callback in self.subscribers: | ||
self.subscribers.remove(callback) | ||
|
||
def notify(self, driver_warning: DriverWarning): | ||
"""Notify all subscribers about the warning.""" | ||
for callback in self.subscribers: | ||
callback(driver_warning) | ||
|
||
|
||
def handle_error(library_interpreter, code, ignore_warnings, is_error_handling): | ||
'''handle_error | ||
|
||
|
@@ -142,4 +164,6 @@ def handle_error(library_interpreter, code, ignore_warnings, is_error_handling): | |
raise DriverError(code, description) | ||
|
||
assert _is_warning(code) | ||
warnings.warn(DriverWarning(code, description)) | ||
driver_warning = DriverWarning(code, description) | ||
library_interpreter.generate_driver_warning_event(driver_warning) | ||
warnings.warn(driver_warning) |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -227,9 +227,9 @@ class Session(_SessionBase): | |||
|
||||
<% | ||||
ctor_for_docs = init_function | ||||
import copy | ||||
ctor_for_docs = copy.deepcopy(ctor_for_docs) | ||||
if grpc_supported: | ||||
import copy | ||||
ctor_for_docs = copy.deepcopy(ctor_for_docs) | ||||
ctor_for_docs['parameters'].append( | ||||
{ | ||||
'default_value': None, | ||||
|
@@ -245,17 +245,34 @@ if grpc_supported: | |||
'use_in_python_api': False, | ||||
}, | ||||
) | ||||
|
||||
ctor_for_docs['parameters'].append( | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. injecting metadata in a mako template seems very hacky There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we do. nimi-python/build/templates/session.py.mako Line 233 in e66d8ee
I agree. It's hacky. It should only be done if it's not possible to include directly in our metadata. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is here since my initial approach was to try to restrict this only for rfsg. This was conditional inject at that point. Given the approach now is to apply for all drivers, I could move this to config metadata and consume from there. |
||||
{ | ||||
'default_value': None, | ||||
'direction': 'in', | ||||
'documentation': { 'description': 'Driver warning event which can be subscribed to, with a callback method.\nSample callback method:\n\ndef sample_callback_method(driver_warning: ' + module_name + '.DriverWarning):\n print(str(driver_warning))\n' }, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't find this description to be very clear or helpful. Once we nail the class terminology we need to workshop this. |
||||
'enum': None, | ||||
'is_repeated_capability': False, | ||||
'is_session_handle': False, | ||||
'python_name': 'driver_warning_event', | ||||
'size': {'mechanism': 'fixed', 'value': 1}, | ||||
'type_in_documentation': module_name + '.DriverWarningEvent', | ||||
'type_in_documentation_was_calculated': False, | ||||
'use_in_python_api': False, | ||||
}, | ||||
) | ||||
%>\ | ||||
${helper.get_function_docstring(ctor_for_docs, False, config, indent=8)} | ||||
''' | ||||
driver_warning_event = errors.DriverWarningEvent() | ||||
% if grpc_supported: | ||||
if grpc_options: | ||||
import ${module_name}._grpc_stub_interpreter as _grpc_stub_interpreter | ||||
interpreter = _grpc_stub_interpreter.GrpcStubInterpreter(grpc_options) | ||||
interpreter = _grpc_stub_interpreter.GrpcStubInterpreter(grpc_options, warning_event_handler=driver_warning_event) | ||||
else: | ||||
interpreter = _library_interpreter.LibraryInterpreter(encoding='windows-1251') | ||||
interpreter = _library_interpreter.LibraryInterpreter(encoding='windows-1251', warning_event_handler=driver_warning_event) | ||||
% else: | ||||
interpreter = _library_interpreter.LibraryInterpreter(encoding='windows-1251') | ||||
interpreter = _library_interpreter.LibraryInterpreter(encoding='windows-1251', warning_event_handler=driver_warning_event) | ||||
% endif | ||||
|
||||
# Initialize the superclass with default values first, populate them later | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,9 +62,10 @@ class LibraryInterpreter(object): | |
* Converting errors returned by Library into Python exceptions. | ||
''' | ||
|
||
def __init__(self, encoding): | ||
def __init__(self, encoding, warning_event_handler: errors.DriverWarningEvent): | ||
self._encoding = encoding | ||
self._library = _library_singleton.get() | ||
self._warning_event_handler = warning_event_handler | ||
global _was_runtime_environment_set | ||
if _was_runtime_environment_set is None: | ||
try: | ||
|
@@ -91,6 +92,14 @@ def set_session_handle(self, value=0): | |
def get_session_handle(self): | ||
return self._vi | ||
|
||
def generate_driver_warning_event(self, driverwarning: errors.DriverWarning): | ||
'''generate_driver_warning_event | ||
|
||
Generates a driver warning event. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does it mean to "generate a driver warning event"? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We call this function from handle_error function in our errors.py(similar to get_error_description placed next to this method). Hence the lack of underscore. This should be public method of library interpreter which owns the warning_event_handler and has interaction with a utility function in errors.py. Completely internal though. Idea was to not expose warning event handler's internal operations in session object. Library interpreter object owning the current session's warning_event_handler instance was making sense since there should be just one handler per session. |
||
''' | ||
if self._warning_event_handler is not None: | ||
self._warning_event_handler.notify(driverwarning) | ||
|
||
def get_error_description(self, error_code): | ||
'''get_error_description | ||
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comparison with other APIs: NIDCPower .NET warnings:
DAQmx .NET warnings:
DAQmx Python events:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One missing feature is the ability to pass the session to the warning handler so it can call back into the session. You can work around this by using a different callable for each session. Note that this creates a reference cycle: session -> interpreter -> warning_event_handler -> subscribers -> callable -> session It's good if closing the session breaks the cycle, but apparently Python's GC can handle reference cycles. Customers may want to use a weak ref to break the cycle. If the Session class has slots, it needs to include the |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,6 +101,28 @@ def __init__(self, code, msg): | |
super(SelfTestError, self).__init__('Self-test failed with code {}: {}'.format(code, msg)) | ||
|
||
|
||
class DriverWarningEvent: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bkeryan Hey Brad, question for you: As of now, nimi-python drivers just log warnings. This PR adds a way for clients to pass a callback function for whenever a warning occurs so that clients can customize the behavior. Do you know what other driver Python APIs do with warnings? I don't want to invent something only to have nisyscfg or nidaqmx invent something different for the sake of being different. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bkeryan , any thoughts on this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think using the standard Python
nidaqmx reports them via the standard Python The DAQmx .NET API uses an event like this, though.
I don't know. I think #2088 is the only request I've seen for this capability in Python. |
||
'''Event handler for driver warnings.''' | ||
|
||
def __init__(self): | ||
self.subscribers = [] | ||
|
||
def subscribe(self, callback): | ||
"""Subscribe to warning events.""" | ||
if callback not in self.subscribers: | ||
self.subscribers.append(callback) | ||
|
||
def unsubscribe(self, callback): | ||
"""Unsubscribe from warning events.""" | ||
if callback in self.subscribers: | ||
self.subscribers.remove(callback) | ||
|
||
def notify(self, driver_warning: DriverWarning): | ||
"""Notify all subscribers about the warning.""" | ||
for callback in self.subscribers: | ||
callback(driver_warning) | ||
|
||
|
||
def handle_error(library_interpreter, code, ignore_warnings, is_error_handling): | ||
'''handle_error | ||
|
||
|
@@ -123,4 +145,6 @@ def handle_error(library_interpreter, code, ignore_warnings, is_error_handling): | |
raise DriverError(code, description) | ||
|
||
assert _is_warning(code) | ||
warnings.warn(DriverWarning(code, description)) | ||
driver_warning = DriverWarning(code, description) | ||
library_interpreter.generate_driver_warning_event(driver_warning) | ||
warnings.warn(driver_warning) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7498,16 +7498,23 @@ def __init__(self, resource_name, channels=None, reset=False, options={}, indepe | |
|
||
grpc_options (nidcpower.grpc_session_options.GrpcSessionOptions): MeasurementLink gRPC session options | ||
|
||
driver_warning_event (nidcpower.DriverWarningEvent): Driver warning event which can be subscribed to, with a callback method. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not the documentation for instance attributes. It's the documentation for the |
||
Sample callback method: | ||
|
||
def sample_callback_method(driver_warning: nidcpower.DriverWarning): | ||
print(str(driver_warning)) | ||
|
||
|
||
Returns: | ||
session (nidcpower.Session): A session object representing the device. | ||
|
||
''' | ||
driver_warning_event = errors.DriverWarningEvent() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From what I can tell, this doesn't actually allow users to subscribe to these events. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Naming might be the culprit here. errors.DriverWarningEvent() is not the event itself, but a class which allows handling of warnings as events. I've handled it somewhat similar to session_handle which gets created during session initialize at session object and then gets passed into library_interpreter for future use. The same instance of DriverWarningEvent() object is at play at the session object(public member) and library interpreter(private member), somewhat like a shared_ptr. Hence the addition of generate_warning_event method in library interpreter to access its private member. Encapsulation is not broken as such here as per my interpretation. Also, the functionality of this is tested already and I've added a unit test in nifake to check the subscription and notification operation. I see interpreter member name in session object not starting with _. Maybe there was a need to not treat interpreter as private? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not a member variable. It's a local variable in the |
||
if grpc_options: | ||
import nidcpower._grpc_stub_interpreter as _grpc_stub_interpreter | ||
interpreter = _grpc_stub_interpreter.GrpcStubInterpreter(grpc_options) | ||
interpreter = _grpc_stub_interpreter.GrpcStubInterpreter(grpc_options, warning_event_handler=driver_warning_event) | ||
else: | ||
interpreter = _library_interpreter.LibraryInterpreter(encoding='windows-1251') | ||
interpreter = _library_interpreter.LibraryInterpreter(encoding='windows-1251', warning_event_handler=driver_warning_event) | ||
|
||
# Initialize the superclass with default values first, populate them later | ||
super(Session, self).__init__( | ||
|
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.
Missing an underscore
driverwarning -> driver_warning