Skip to content
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

Do away with the need to use indexing on user response functions #177

Open
jcollins1983 opened this issue Sep 20, 2023 · 6 comments
Open
Assignees

Comments

@jcollins1983
Copy link
Collaborator

This may be more involved than it sounds.

Refer to ui_gui_qt.py

def _topic_UI_req_choices(self, msg, q, choices, target, attempts=5):
    """
    This can be replaced anywhere in the project that needs to implement the user driver
    Temporarily a simple input function.
    The result needs to be put in the queue with the first part of the tuple as 'Exception' or 'Result' and the
    second part is the exception object or response object
    This needs to be compatible with forced exit. Look to user action for how it handles a forced exit
    ... 

The requirement for the item to be put in the queue as a Tuple with Exception or Result as the first element is the thing that drives the necessity to do things in test scripts like this relay_open = user_yes_no("Confirm the relay is open. (DMM not buzzing)")[1]

The same is done in the cmd_line.py, where _user_choices has the same docsctring. Understanding why this was thought (or is) necessary and/or whether it actually requires this format would be good. I'd like to dig in because I'm interested but can't right now.

@jcollins1983
Copy link
Collaborator Author

_user_req_choices in ui.py is the mechanism used by user_yes_no and user_retry_abort_fail.
_user_req_choices looks after the messaging between the actual UI with the topic of UI_req_choices.
In cmd_line.py the listener is _user_choices and in ui_gui_qt.py the listener is _topic_UI_req_choices.

Both have identical logic, where a loop iterates until the user selects a valid choice. In the gui the user has no option other than to select a valid choice, so the message put on the queue will always be ("Result", ret_val).

In the command line, it is possible to enter an invalid choice, however when the number of retries is exceeded (default for user_yes_no is 1) the returned value is only the string "Exception".

image

For some reason the UserInputError exception object is not ending up in the queue.
image

And, when called by user_retry_abort_fail when the number of attempts are exceeded a different error is encountered.
image

I'll come back to this soon.

@clint-lawrence
Copy link
Collaborator

It's kind of horrendous that this has remained unfixed for so long. And that it was ever like this in the first place

@jcollins1983
Copy link
Collaborator Author

The cause of the weirdness around the "Exception" logic is a couple of missing brackets. It seems that Queue.put() will accept more than one argument but will only put the first one on the queue.

q.put("a", "b") # doesn't complain
q.get() # returns "a"
q.get() # hangs waiting for something to be put on the queue

The offending line

q.put(
    "Exception",
    UserInputError("Maximum number of attempts {} reached".format(attempts)),
)

This is why I was only seeing "Exception" as the response, and why a different exception occurs in the user_retry_abort_fail call.
In exceptional circumstances the response being returned was a single value, which can't be unpacked.
image

In any case, the "Exception" logic isn't used by the sequencer (regardless of whether it puts a tuple on the queue or not).

image

status is ignored and the exception is not checked for. If in the command line an invalid entry is made attempts times this is implicitly interpreted as a "FAIL", if a test failed or "ERROR", if an error occurred like Keyboard interrupt etc., since UserInputError(...) == "RETRY|ERROR" -> False,

We would get the same behaviour with just putting something like "BAD_VALUE" or anything that isn't a valid choice on the queue, assuming we want to continue to support the command line, because this really is a non-event in the GUI.

image

This change would require every script to be updated to move away from the common _, resp = user_yes_no(), resp = user_yes_no()[1] patterns.

I have a local branch ready to push we're keen.

@daniel-montanari
Copy link
Collaborator

daniel-montanari commented Aug 14, 2024

So entering 0 into user_input_float actually fails due to the gui converting the result to boolean.
Also happens in the cli due to the same conversion.
image

@clint-lawrence
Copy link
Collaborator

So entering 0 into user_input_float actually fails due to the gui converting the result to boolean.

Could you create an issue for that?

@daniel-montanari
Copy link
Collaborator

So entering 0 into user_input_float actually fails due to the gui converting the result to boolean.

Could you create an issue for that?

Raised in #213

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants