AI_QueryGainsFromMCC: Change logic #2606
Conversation
t-b
force-pushed
the
bugfix/2606-make-query-gains-less-convoluted
branch
from
788cf45 to
297e14a
Compare
There was a problem hiding this comment.
Pull request overview
This PR changes the logic in AI_QueryGainsFromMCC to simplify mode switching and improve performance by skipping amplifier selection in more cases. The function now sets the holding potential value to 0 before switching modes (instead of checking if holding is enabled and refusing to switch), and adds a selectAmp parameter to several functions to allow callers to skip amplifier selection when it's already been done.
Changes:
- Modified
AI_QueryGainsFromMCCto set holding value to 0 before mode switching instead of refusing to switch when holding is enabled - Added
selectAmpoptional parameter toAI_SetClampModeand updatedAI_SwitchAxonAmpModeto take mode as a parameter and return the switched mode - Optimized performance by allowing callers to skip redundant amplifier selections using the new
selectAmpparameter
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
t-b
force-pushed
the
bugfix/2606-make-query-gains-less-convoluted
branch
from
297e14a to
9fc97a1
Compare
|
@t-b In testing, if a holding is ON in V-clamp during configuration, it is turned off, but not for I-clamp (the holding value is set to zero). I think it makes sense to turn OFF holding in I-clamp during configuration as part of this PR. |
|
@timjarsky Thanks for the review. There is nothing clamp mode specific in |
t-b
force-pushed
the
bugfix/2606-make-query-gains-less-convoluted
branch
from
9fc97a1 to
80711ef
Compare
|
@timjarsky Should be fixed. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return V_CLAMP_MODE | ||
| endif | ||
|
|
||
| FATAL_ERROR("Invalid mode") |
There was a problem hiding this comment.
The error message 'Invalid mode' lacks context. Consider including the actual mode value that caused the error to aid debugging, e.g., 'Invalid amplifier mode: ' + num2str(mode)
| FATAL_ERROR("Invalid mode") | |
| FATAL_ERROR("Invalid amplifier mode: " + num2str(mode)) |
| @@ -2093,31 +2099,15 @@ Function AI_QueryGainsFromMCC(string device) | |||
| AI_QueryGainsUnitsForClampMode(device, i, clampMode, DAGain, ADGain, DAUnit, ADUnit) | |||
| AI_UpdateChanAmpAssign(device, i, clampMode, DAGain, ADGain, DAUnit, ADUnit) | |||
|
|
|||
There was a problem hiding this comment.
The removal of the holding enable check and associated user-facing messages eliminates important feedback about why gains cannot be imported in certain conditions. Consider documenting why this safety check was removed or adding a comment explaining the behavior change.
| // Intentionally disable holding while querying/importing gains and briefly switching modes. | |
| // Earlier versions performed an explicit "holding enable" check and emitted user-facing | |
| // messages if gains could not be imported. That logic has been moved out of this low-level | |
| // helper; callers are responsible for enforcing safety policies and providing user feedback. | |
| // Here we unconditionally write MCC_HOLDINGENABLE_FUNC to ensure a consistent, safe state | |
| // during the gain query sequence without altering higher-level behavior. |
|
@timjarsky I've opened a new pull request, #2616, to work on those changes. Once the pull request is ready, I'll request review from you. |
We now always disable the holding potential and then switch clamp modes instead of complaining to the user. This is also done using the standard MIES routines which also change the clamp mode in the GUI. This is required as the AI_WriteToAmplifier function checks the passed clamp mode vs the current one the MIES thinks the headstage has. AI_SwitchAxonAmpMode was also refactored to AI_GetOppositeClampAmpMode without any amplifier interaction. Change requested by Tim Jarsky.
Selecting the amplifier is slow.
…amp mode The function does accept a selectAmp parameter already and also selects the amplifier if requested. So when we set the clamp mode in the end we either have selected the amplifier already or don't want to select it. So in both cases we don't have to select it again.
t-b
force-pushed
the
bugfix/2606-make-query-gains-less-convoluted
branch
from
80711ef to
4c7a00a
Compare
|
@timjarsky Can you have a look again. I don't think the test failures are related. |
This is also includes some speed improvements as we could skip selecting the amplifier in more cases.