CONF: Clear HS association on DAEphys window restore for HS w/o amp #1804
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MichaelHuth
force-pushed
the
bugfix/1804-restore_unassocDA_state_on_config_load
branch
2 times, most recently
from
e2e74f5
to
078dfc7
Compare
MichaelHuth
force-pushed
the
bugfix/1804-restore_unassocDA_state_on_config_load
branch
from
078dfc7
to
cd0aa98
Compare
|
@t-b Based on our chat yesterday, may I be unassigned from this PR for now? |
MichaelHuth
force-pushed
the
bugfix/1804-restore_unassocDA_state_on_config_load
branch
from
cd0aa98
to
c71f89b
Compare
|
d83d874 (Tests: Fix of FixupJSONConfigImplRig to replace only existing amp serials, 2023-07-07) Already reviewed. 186ddcd (PGC: Add optional argument noRangeCheck to PGC_SetAndActivateControl, 2023-07-21) No tests because??? I'm also not convinced that this is the right approach. Why not teach the popup menu control have 4f13d36 (CONF: For DAEPhys save DA/AD gain/unit/channels for all active HS, 2023-07-21) Missing tests. The changed handling of the title entry should also be its own commit. 0ba284d (CONF: For DAEPhys load the AD/DA channels are restored for all active Headstages, 2023-07-21) I also don't understand
does it make sense to write NaN as DA/DA channel number out? The following window configuration was stored with HS 7 active and no DA/AD channels associated. And loading that as config fails with Note the missing filename. Missing tests and please also check the wording in the commit message. c71f89b (DAEPhys: Increased width of AD/DA gain setVar fields, 2023-07-21) Looks good. I also saw that the hiding and restoring of the DAEphys panel has changed in this PR. Now it is shown when the controls are configured on load. Are the changes in DAP_CheckHeastage missing? |
MichaelHuth
force-pushed
the
bugfix/1804-restore_unassocDA_state_on_config_load
branch
from
c71f89b
to
638b46f
Compare
|
@timjarsky Feel free to test already as the changes Michael needs to do are not functionality wise. |
MichaelHuth
force-pushed
the
bugfix/1804-restore_unassocDA_state_on_config_load
branch
from
ca36a2f
to
c25f929
Compare
timjarsky
previously approved these changes
Aug 3, 2023
|
@MichaelHuth Please rebase and assign me. |
…ials - Before we assumed to have a HS0 and HS1 with amplifier for the configuration tests, future configuration tests have more versatile amplifier settings. Thus, a more general approach for the fixup is to replace amplifier serials only for amplifiers that are already present in the configuration.
- Some gains are as small as 0.0005 and were cut off by the field width these are now fully visible - The daephys panel version was increased
- Previously this was cleared by "Clear Headstage Association" to an invalid PopupMenu index that resulted in an empty PopupMenu. this has two disadvantages: 1. The user can not set No-Channel himself 2. The code must handle invalid list indices - The panel controls were updated and repositioned to fit the larger size - The DAEPhys panel version was increased - The update and readout of the DA/AD channel popup menus was adapted
…ings - outputs now also the headstage number that has problematic settings as it might not be the currently visible headstage configuration and, thus visually hidden
- Instead of setting the control content directly, use PGC_SetAndActivateControl because this also calls the associated GUI procedure and updates GUI state waves. - Due to setting the controls triggers a resynchronisation of GUI to CAA wave, it works on a copy of the CAA wave.
The consistency of the GetChannelClampMode gets corrupted. The function DAP_RemoveClampModeSettings clears unused clamp mode settings. However, it is only called if a headstage is unchecked or the clamp mode is switched. For the case the DAEPhys panel is closed and reopened the clamp mode wave with the previous setting is kept in the former and now again current device data folder. Though, the default headstage state in the panel is unchecked. If one was using headstage 2 and 3 before and starts using head stage 0 and 1 in the current DAEPhys panel with at least partially the same DA/AD channels then the setup of the new DAQ config wave fails in DC_PlaceDataInDAQConfigWave. This is due to remaining entries in the clamp mode wave from headstage 2 and 3 that are now not used. The remaining entries cause DC_GetFilteredChannelState to create an invalid mapping because the twice appearing DAC channel is counted twice for a single headstage (through AFH_GetHeadstageFromDAC). Fix: Kill clamp mode wave on locking of DAEPhys panel.
Since d4b24fc (DA_Ephys: Make "Save amplifier settings" checkbox dependent on "Require amplifier", 2022-07-29) the control check_Settings_SaveAmpSettings was setup as dependent control of check_Settings_RequireAmpConn where is gets unchecked and disabled if the main control gets unchecked. However, if the main control is unchecked the user can still setup amplifiers for head stages that were not saved to the LNB then. The fix is to remove the dependency as amplifier information should always be saved if an amplifier was enabled and check_Settings_SaveAmpSettings was checked.
- Previously the on restore the amplifiers were set plain by headstage.
The default configuration of the DAEphys panel is that unless
"Clear Associations" is clicked all HS are associated.
Also MIES implements that associated HS need to have an amplifier set.
On saving the DAEphys window there is no GUI control that state explicitly
if a HS is associated or not.
Solution:
Before the solution is introduced some details to the previous behavior.
When a DAEPhys panel is configured some settings are stored in the MIES
DF and restored if the DAEPhys panel is opened at a later time again.
To prevent any influence by that the test kills the MIES DF after saving
the configuration.
When restoring the DAEPhys panel the specific code ran in the following
order:
1. Restore the DAEPhys panel as generic window respecting the control
priority settings
2. Restore HeadStage associations
3. Restore UserPressure settings
At point 1 the HeadStage checkboxes and the DA/AD channel checkboxes have
both the same priority (Inf). By default a "virgin" DAEPhys panel has
HS 0 - 3 associated. Thus, depending on the order of HS or Channel
checkboxes activates they influence the setting of the other automatically.
Thus, it makes no sense to attribute a priority order for HS vs Channel CBs.
A better solution is to set the HeadStage association first before generally
restoring the window state. If the association is set, then enabling a
channel results in enabling a headstage if associated. And enabling an
associated headstage checkbox enables the DA/AD channels. This is consistent.
Considering unassociated headstages, then enabling a DA channel does not
influence the headstage setting and vice versa.
To apply the HeadStage association first, the DAEPhys panel must be locked
first. Some code has been added for that. the generic window restore will
attempt to lock again with the same values, but that is ok.
In the headstage association function if no amplifier for a headstage is set
it is treated as unassociated and the HS association is cleared.
since introduction of configuration module
Move DAEPhys window minimize to earlier point in restore logic
- Due to the earlier headstage association restore the window minimize has to be
also done earlier
…e headstages - also Gain and Unit is restored. - if originally no DA/AD channel was set, then NaN is saved as channel number from the chanAmpAssign wave, this is restored properly to an "None" channel number. - To stay compatible with older config files, the existence of all these config entries is checked before evaluation. For that two utility function were added: CONF_OnExistSetAndActivateControlVar and CONF_OnExistSetAndActivateControlStr
- The abort error message only showed a file name if a file name was set as argument of the function. Thus, when calling the function with an empty file name and then using the file dialog, no file name was shown in the error message. - Fix: use the fullFilePath the LoadTextFile returns for the error
- checks if the chanAmp values are in line with the settings in the DA/AD channel tabs - there was no check before for active HS without amplifier
MichaelHuth
force-pushed
the
bugfix/1804-restore_unassocDA_state_on_config_load
branch
from
c25f929
to
0fd90a4
Compare
t-b
approved these changes
Aug 3, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously the on restore the amplifiers were set plain by headstage. The default configuration of the DAEphys panel is that unless "Clear Associations" is clicked all HS are associated. Also MIES implements that associated HS need to have an amplifier set.
On saving the DAEphys window there is no GUI control that state explicitly if a HS is associated or not.
Solution: We assume that the headstages without a set amplifier are unassociated and clear the association for these HS.
since introduction of configuration module
close #1763
close #1811