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

Pressure Control: Disable manual controls when user access is enabled #2137

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

t-b
Copy link
Collaborator

@t-b t-b commented Jun 6, 2024

Otherwise we can get an assertion as the manual pressure pulse interferes.

Close #2124

@t-b t-b added the PR:NeedsBackport (Pull requests only) Mark it as requiring a backport to the latest release branch label Jun 6, 2024
@t-b t-b requested a review from timjarsky as a code owner June 6, 2024 21:44
@t-b t-b force-pushed the bugfix/2137-fix-assertion-during-manual-pressure-pulse branch from fefd073 to f33927c Compare June 6, 2024 21:45
@t-b t-b removed the PR:NeedsBackport (Pull requests only) Mark it as requiring a backport to the latest release branch label Jun 10, 2024
@timjarsky
Copy link
Collaborator

timjarsky commented Jun 17, 2024

@t-b I think it also makes sense to stop the application of a manual pressure, when user access is checked.

@timjarsky timjarsky assigned t-b and unassigned timjarsky Jun 17, 2024
@t-b t-b force-pushed the bugfix/2137-fix-assertion-during-manual-pressure-pulse branch 2 times, most recently from 94fac2a to 84481a1 Compare June 18, 2024 22:12
@t-b
Copy link
Collaborator Author

t-b commented Jun 18, 2024

@timjarsky Thanks for testing. I've revised the solution completely. Please test.

@t-b t-b assigned timjarsky and unassigned t-b Jun 18, 2024
@t-b t-b force-pushed the bugfix/2137-fix-assertion-during-manual-pressure-pulse branch from 84481a1 to 9f4bd85 Compare June 18, 2024 22:20
@timjarsky
Copy link
Collaborator

@t-b this assertion when moving the headstage slider with user access enabled is also in main

  !!! Assertion FAILED !!!
  Message: "The control setvar_DataAcq_SSPressure in the panel ITC1600_Dev_0 is disabled. The control state cannot be changed when disabled."
  Please provide the following information if you contact the MIES developers:
  ################################
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  Stacktrace:
  DAP_SliderProc_MIESHeadStage(...)#L3662 [MIES_DAEphys.ipf]
P_UpdatePressureModeTabs(...)#L1827 [MIES_PressureControl.ipf]
PGC_SetAndActivateControl(...)#L203 [MIES_ProgrammaticGUIControl.ipf]
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  Time: 2024-07-10T16:21:17-07:00
  Locked device: [ITC1600_Dev_0]
  Current sweep: [NaN]
  DAQ: [DAQ_NOT_RUNNING]
  Testpulse: [TEST_PULSE_BG_MULTI_DEVICE]
  Experiment: Untitled ()
  Igor Pro version: 9.0.6.1 (56565)
  MIES version:
  Release_2.7_20230809-785-g9f4bd850c
Date and time of last commit: 2024-06-19T00:20:29+02:00
Submodule status: 
-bcbf5729ee85da80ea0f14dd5a42b55ed8fda47e Packages/IPNWB
-ecdc0c6271c96ce2101b626b1361ab242a30a2e7 Packages/doc/doxygen-filter-ipf
-d557db885f2077bb1998cf9ef938477881c44775 Packages/igortest
  ################################

@timjarsky
Copy link
Collaborator

@t-b the changes specific to this PR look good.

@t-b t-b assigned t-b and unassigned timjarsky Jul 11, 2024
@t-b t-b force-pushed the bugfix/2137-fix-assertion-during-manual-pressure-pulse branch from 9f4bd85 to d2dd674 Compare July 11, 2024 16:53
@t-b
Copy link
Collaborator Author

t-b commented Jul 11, 2024

@timjarsky Fixed.

@t-b t-b assigned timjarsky and unassigned t-b Jul 11, 2024
@t-b t-b enabled auto-merge July 11, 2024 19:10
@timjarsky
Copy link
Collaborator

@t-b, when enabling user access, the Apply button and the psi setvar for manual pressure control are disabled. However, when selecting another headstage while user access is enabled (and functional), the Apply button is enabled on the newly selected headstage. It should remain disabled.

When turning off user access, the Apply button and psi setvar become enabled, however, the pressure is not applied to the headstage.

I wonder if the desired behavior should be that User access works with manual pressure control and the manual value is only applied when user access is turned off.

@timjarsky timjarsky assigned t-b and unassigned timjarsky Jul 12, 2024
@t-b
Copy link
Collaborator Author

t-b commented Jul 15, 2024

However, when selecting another headstage while user access is enabled (and functional), the Apply button is enabled on the newly selected headstage. It should remain disabled.

I'm not sure. The code suggests that user access is a per headstage setting, see e.g. P_GetUserAccess.

When turning off user access, the Apply button and psi setvar become enabled, however, the pressure is not applied to the headstage.

Fixed.

I wonder if the desired behavior should be that User access works with manual pressure control and the manual value is only applied when user access is turned off.

We can chat about this today.

t-b added 2 commits July 15, 2024 14:07
We don't need to take care of a non-existing pressure data wave.
We need to save returned free waves in WAVE statements first before using
them.
t-b added 3 commits July 15, 2024 14:07
This is broken since e29c778 (PGC_SetAndActivateControl: Make setting a
disabled control an error, 2021-09-30).
@t-b t-b force-pushed the bugfix/2137-fix-assertion-during-manual-pressure-pulse branch from d2dd674 to 633d0cc Compare July 15, 2024 12:08
@t-b
Copy link
Collaborator Author

t-b commented Jul 16, 2024

Tim will playaround to find out the desired behaviour regarding user pressure.

@t-b t-b removed their assignment Jul 16, 2024
@timjarsky
Copy link
Collaborator

@t-b User access and manual interactions should mimic user access and auto pressure option interactions; the manual mode should be maintained (not disabled) while giving the user access. So, all that is being switched are the TTLs, and the regulator output pressure is maintained (as is the button state).

@timjarsky timjarsky assigned t-b and unassigned timjarsky Aug 29, 2024
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.

Assertion after using pressure pulse
2 participants