-
Notifications
You must be signed in to change notification settings - Fork 10
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
Implementation of generic command handling in odin data adapter #373
base: master
Are you sure you want to change the base?
Conversation
Needs re-testing, execute hook added and finally sending of command using IpcTornadoClient. |
Supported commands are read out from the underlying applications and ParameterTree structure built as appropriate. Commands are queued and then executed after configuration updates. |
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.
Looks good. Just a couple of comments.
0068953
to
1b39f31
Compare
14352d4
to
eab3e9b
Compare
1b39f31
to
52228df
Compare
52228df
to
114c2a7
Compare
"allowed": ( | ||
lambda x=client.parameters["commands"][plugin]["supported"]: x, | ||
None, | ||
{} | ||
), |
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.
Getting the error [E 241213 11:50:07 odin_data_controller:167] string indices must be integers, not 'str'
with these lines uncommented in the fastcs-odin dev deployment odin server
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.
And on the FP C++ process: Error handling control channel request from client ...: Illegal command request value: 5
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.
I am not sure what that error means. Maybe try removing the try except and letting it crash to see if the stack trace is helpful? And if you stop the control server in the deployment and run the launch config you can debug and pause exception.
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.
Can reproduce:
[E 241216 10:07:28 odin_data_controller:168] 'supported'
Exception in thread Thread-1 (update_loop):
Traceback (most recent call last):
File "/usr/lib/python3.11/threading.py", line 1045, in _bootstrap_inner
self.run()
File "/usr/lib/python3.11/threading.py", line 982, in run
self.run()
File "/usr/lib/python3.11/threading.py", line 982, in run
self._target(*self._args, **self._kwargs)
File "/workspaces/odin-data/python/src/odin_data/control/odin_data_controller.py", line 158, in update_loop
self.parse_available_commands(index, client)
File "/workspaces/odin-data/python/src/odin_data/control/odin_data_controller.py", line 186, in parse_available_commands
lambda x=client.parameters["commands"][plugin]["supported"]: x,
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
TypeError: string indices must be integers, not 'str'
self._target(*self._args, **self._kwargs)
File "/workspaces/odin-data/python/src/odin_data/control/odin_data_controller.py", line 158, in update_loop
self.parse_available_commands(index, client)
File "/workspaces/odin-data/python/src/odin_data/control/odin_data_controller.py", line 186, in parse_available_commands
lambda x=client.parameters["commands"][plugin]["supported"]: x,
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^
KeyError: 'supported'
because
client.parameters["commands"][plugin] == 'Illegal command request value: 5'
because frameReceiver does not support commands yet
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.
aha, that makes sense
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.
Fixed by adding fr support in #379, so this PR depends on that now.
Fixes #363