-
Notifications
You must be signed in to change notification settings - Fork 0
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
WSPOW00000, WSPOW0002A, and WSVIDALLDN Power Commands Recording added… #3
base: master
Are you sure you want to change the base?
Conversation
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.
Scanning it by eye, all looks good, Gregg
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.
Since it includes the WSVIDALLDN command, it's not FEPs ON/OFF. Maybe "Power commands"?
Would there be any value in also adding a text field for an arbitrary power command? |
Right now the power commands covered are the 3 mentioned. This is to avoid misspellings during those exciting 3am shut downs. A simnple, can't miss click. If we want to open it up to any power command that could possibly be used I'd have to change the GUI software entirely and force the user to type the power command in. Or, alternatively, to provide a list from which to select. This can be done, of course, but I'd rather keep it to these three unless we start issuing single power commands in a CAP, for a non-load event, that are not one of the three we are working with now. This way the operation of the GUI is simple and very clear. Also, we would have to create new power commands just like we had to create WSPOW0002A prior to using it. Remember: the WSPOW0002A command is non standard in that it doesn't power on FEPs 1,2 and 3 as normal 3 chip SI modes would. We had to create this power command specially. A 2 chip version would also be non-standard. And then Ken would most likely want to create the RTS file for the new power command. Another alternative would be to modify the GUI so that the user can either select from the radio buttons, one of which is "None of the above" and then the user would be forced to enter the text in. But since we really have only those 3 available I prefer to not make massive changes to the GUI for a feature that we have never needed in the past. Should we need it in the future, then it can be added. Any power command in a load is automatically covered and not part of this system. |
Yep, that's what I was thinking; with decom of the command. But if not useful at this time and a hassle, this was definitely not a feature request on my part. I suppose I was just curious to know if the proposed changes completely met the features you have needed when trying to update with non-load commands. |
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.
This looks fine by me aside from the minor comments I left about checking equality with True
(I saw a number of those) and the raising of the error when we can't write to the file.
# elif self.ECbuttonANOMALOUSEVENT.get_active() == True: | ||
# self.ProcessNonCTI(self.Event_Tracking_output_filespec, "Anomaly", date, '---', description, "Final") | ||
# ---- Show me: FEP_ON_FEP_OFF EVENT | ||
elif self.ECbuttonFEP_ON_FEP_OFF_EVENT.get_active() == True: |
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.
Minor nit, but couldn't this just be elif self.ECbuttonFEP_ON_FEP_OFF_EVENT.get_active():
?
x = subprocess.Popen(self.RNLE_command_list) | ||
# Now reset the command list to the base command | ||
self.RNLE_command_list = [ '/proj/sot/ska3/flight/bin/python', '/home/gregg/GIT_NLET_GUI/nlet_gui/RecordNonLoadEvent.py', 'LTCTI', '--source', 'NLET'] | ||
except: |
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.
Is a particular error being thrown here? Like say, IOError
, or something else? We should except that error (or maybe a tuple of the errors you'd expect) so that you don't silently swallow up some unexpected thing.
Would it ever be useful to capture the error and log to the screen exactly what it was?
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'm not sure if this is an active PR at this point either?
… to the GUI.
New option added to the GUI - FEPs ON/FEPs OFF