Skip to content

Conversation

@Remi-Gau
Copy link
Contributor

A lot of refactoring and @CerenB this should also fix #19 and problem of extra responses you were getting from getResponses.

@Remi-Gau
Copy link
Contributor Author

Added a checkAbort function that checks that the the cfg.escapeKey has been pressed and throws an error to stop everything if that's the case.

This function is also called every time getResponse('check') is triggered.

@CerenB this should fix #22

@Remi-Gau Remi-Gau requested review from CerenB and marcobarilari July 16, 2020 22:01
@Remi-Gau
Copy link
Contributor Author

TO DO
@marcobarilari once we have merged this one marcobarilari#2 and #26
I will rebase and add cfg.escapeKey as one of the default value for initPTB.

Copy link
Collaborator

@CerenB CerenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I'll run it today and let you know.

Copy link
Collaborator

@CerenB CerenB left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function checkAbort only would work if in the main script I'm using for loop, right? It'd break the for loop with stopEverything.
What if a user is not using any for loop in their experiment?

@Remi-Gau
Copy link
Contributor Author

function checkAbort only would work if in the main script I'm using for loop, right? It'd break the for loop with stopEverything.
What if a user is not using any for loop in their experiment?

Actually checkAbort is pretty "aggressive" at the moment and will just throw an error so it will stop everything no matter what (even with no loop).

I trying to think of a way to make it abort more "gracefully" (like give a chance to save stuff) and that's why I wanted to create a global variable.

Or do you think this is OK?

@CerenB
Copy link
Collaborator

CerenB commented Jul 17, 2020

well I was thinking that warning instead of error would be nicer. Unfortunately I could not make this abort work even with the error, even with a long press.

  • could it be related to stopEverything?
  • could it be related to touch screen macbook esc button? Though, same touch button works with below snippet:
    % stay in the loop until the sequence ends
    while GetSecs  < (expParam.experimentStart + audioDuration + ...
            expParam.timing.onsetDelay + expParam.timing.endDelay)
        
        % check if key is pressed
        [keyIsDown, ~, keyCode] = KbCheck(cfg.keyboard);
        
        % terminate if quit-button pressed
        if find(keyCode)==cfg.escapeKey
            error('Experiment terminated by user...');
        end
    end

@Remi-Gau
Copy link
Contributor Author

I don't think this should be related to stopEverything.

I have added a demo that should work and is very similar to what you sent.

@Remi-Gau
Copy link
Contributor Author

If this does not work try setting the cfg.escapeKey to space and check if this works when you press the space bar.

At least we will know this is likely not "key" related.

@Remi-Gau
Copy link
Contributor Author

Been thinking of how to implement this "abort" as part of getResponse.

At the moment getResponse only checks the response box, not the keyboard of the experimenter. So the only way to make this "abort" thing work woud be to ask getResponse to check for keypresses on all devices.

I could make this the default and listening to the responseBox would have to be asked for.

@CerenB
Copy link
Collaborator

CerenB commented Jul 18, 2020

that's one of points I got stuck with in getResponse function. I thought now it's listening to all key presses. OR is it? It's listening to responseBox specific button presses, but in keyboard all presses? Maybe when I was checking on Friday, I got stuck at this point because I specified the keys to check but it was checking all the key presses (no additional keyboards were connected than built in mac keyboard)

@CerenB
Copy link
Collaborator

CerenB commented Jul 18, 2020

to respond to your question, how about separate these two functions completely? Both uses KbQueue checks/buffering. Aborting function checks keyboard and aborts things - maybe there's a way to do it smoothly when things are independent two functions?

@marcobarilari
Copy link
Collaborator

marcobarilari commented Jul 18, 2020

function checkAbort only would work if in the main script I'm using for loop, right? It'd break the for loop with stopEverything.
What if a user is not using any for loop in their experiment?

Actually checkAbort is pretty "aggressive" at the moment and will just throw an error so it will stop everything no matter what (even with no loop).

I trying to think of a way to make it abort more "gracefully" (like give a chance to save stuff) and that's why I wanted to create a global variable.

Or do you think this is OK?

Maybe, since it prompts an error, we can have something in the catch part that will save everything (workspace, button presses in the buffer and close the file, eye tracker data, etc).

Does it make sense?

@Remi-Gau
Copy link
Contributor Author

to respond to your question, how about separate these two functions completely? Both uses KbQueue checks/buffering. Aborting function checks keyboard and aborts things - maybe there's a way to do it smoothly when things are independent two functions?

Need to check if we can have to KbQueues going in parallel.

@Remi-Gau
Copy link
Contributor Author

that's one of points I got stuck with in getResponse function. I thought now it's listening to all key presses. OR is it? It's listening to responseBox specific button presses, but in keyboard all presses? Maybe when I was checking on Friday, I got stuck at this point because I specified the keys to check but it was checking all the key presses (no additional keyboards were connected than built in mac keyboard)

So by default getResponse listens to the responseBox but if you have only one device then the keyboard and the responseBox should be the same.

Did you specify the keys to check like this?

expParameters.responseKey = {'a', 'b', 'c'}; 

@Remi-Gau
Copy link
Contributor Author

OK what I am thinking of starting to do is to have getResponse and the abort function to check for all devices by default (to make it most general) and then implement a way to let the user decide which device (keyboard or response box), each function should listen to.

This will require some changes to the default and the initPTB I think but nothing crazy.

This might make things less confusing for most new PTB users so that's not bad.

Remi-Gau added 14 commits July 19, 2020 11:39
- add comments
- create demo
- waitForTrigger returns automatically after last trigger
- to match the way the cfg is now structured
- will throw a specific error that can be catched to fail gracefully
- update demo
- can listen to a specific device
- all information about is passed through the cfg structure even info about restricted keys 
- information are passed through cfg.keyboard
- this will only work during the "check"
change to keyboards function and defaults
@Remi-Gau Remi-Gau requested a review from CerenB July 19, 2020 13:41
@Remi-Gau
Copy link
Contributor Author

OK so a series of changes on all the keyboard functions:

  • you can tell them which device they should listen to
  • all keyboard info is passed through cfg.keyboard
  • added some demo
  • we have some automated test for the defaults: to make sure they are set properly
  • we have a manual test to help ensure out that getResponse does what we want it to do
  • getResponse CAN check for demand to abort if the escapeKey is listed in the Keys of interest.
  • getResponse can only check for demands to abort when getResponse('check') is called: so there will be a delay between the key press and the experiment stopping
  • abort errors send specific signals that allow the catch to get them and allows us to "close" nicely (not sure how to keep yet unsaved responses though)

@Remi-Gau
Copy link
Contributor Author

Similarly will merge in 24 hours unless I get a veto. 😄

@Remi-Gau Remi-Gau merged commit d400c66 into cpp-lln-lab:master Jul 22, 2020
@Remi-Gau Remi-Gau deleted the remi-REF_getResponses branch July 22, 2020 17:47
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

Successfully merging this pull request may close these issues.

Add check fo NaN\nulls in getResponse function

3 participants