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.

Sign up for GitHub

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

Jump to bottom

Fix ReachTargetVoltage handling for too small resistance #2290

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

Fix ReachTargetVoltage handling for too small resistance #2290

wants to merge 9 commits into from

Conversation

t-b
Copy link
Collaborator

@t-b t-b commented Oct 25, 2024

We used to always use 100pA if the resistance is tool small, but the code obviously only wanted to do that only on the first sweep. Fix that.

@t-b t-b mentioned this pull request Nov 5, 2024
Open
48 tasks
@t-b t-b force-pushed the bugfix/2290-reach-target-voltage branch 3 times, most recently from 25b0267 to dbc993d Compare November 19, 2024 10:59
@t-b t-b mentioned this pull request Jan 6, 2025
Open
3 tasks
@timjarsky
Copy link
Collaborator

timjarsky commented Jan 9, 2025 •
edited
Loading

Acquisition continues after required sweeps have been acquired. It's been a while, but I recall it should stop automatically.

  (ITC1600_Dev_0): Skipping analysis function "ReachTargetVoltage".
  The stimset has too many sweeps, increase the size of DAScales.

2025_01_09_122405_2.zip

@timjarsky timjarsky assigned t-b and unassigned timjarsky Jan 9, 2025
t-b added 9 commits January 14, 2025 15:51
@t-b
tests: Factor MarkDeviceAsLocked out
021c82d
@t-b
Packages/tests: Add ConstantZero stimset
c38d189
@t-b
HARDWARE_ITC_BITS_PER_VOLT: Enhance comment
362bb1b
@t-b
HW_GetVoltageRange: Rename it
5787333 
We don't return voltages for all hardware types, so the current name is
confusing.

So let's rename it.
@t-b
tests/InitDAQSettingsFromString: Check headstage
8698bdd
@t-b
ReachTargetVoltage: Prefer sweepNo from the structure
63e78c1 
Missed in 85d6c5b (ReachTargetVoltage: Convert to V3 analysis function, 2020-11-20).
@t-b
ReachTargetVoltage: Fix initial response check
6f0280e 
We want to only act specially on the very first sweep. But as we reset the
index, we will land in this branch on every sweep if the resistance is too
small.

Let's prefer comparing to the number of acquired sweeps instead.
@t-b
ReachTargetVoltage: Prefer scaledDACWave from the structure
fe090a4 
Missed in 85d6c5b (ReachTargetVoltage: Convert to V3 analysis function, 2020-11-20).
@t-b
Tests/UTF_ReachTargetVoltage.ipf: Rework them
10f81b5 
We know have 6 sweeps in the stimset, which triggers the finish condition
as we need one sweep more than the targetVoltages wave. Also added is a
test with multiple headstages.
@t-b
Copy link
Collaborator Author

t-b commented Jan 14, 2025 •
edited
Loading

Acquisition continues after required sweeps have been acquired. It's been a while, but I recall it should stop automatically.

That's the "new" approach we do in the modern analysis functions. ReachTargetVoltage was one of the first ones (2017) and does not follow that approach. We can fix that but this is IMHO tangent to the fix in this PR.

In your pxp I see in the history some bug messages

 MIES BUG_TS: Encountered pending RTE: 1321, a wave read;Index out of range for wave "_free_".
  MIES BUG_TS: Encountered pending RTE: 1321, a wave read;Index out of range for wave "_free_".
  (ITC1600_Dev_0): Skipping analysis function "ReachTargetVoltage".
  The stimset has too many sweeps, increase the size of DAScales.
  (ITC1600_Dev_0): Skipping analysis function "ReachTargetVoltage".
  The stimset has too many sweeps, increase the size of DAScales.

which need fixing. I tried to reproduce that here, but that did not work. So I would need a screen share for that.

@t-b t-b force-pushed the bugfix/2290-reach-target-voltage branch from dbc993d to 10f81b5 Compare January 14, 2025 18:30
@timjarsky
Copy link
Collaborator

@t-b I also get the error in main, so I guess we designed it such that the number of sweeps needs to match the size of DAScales (not an analysis param).

  (ITC1600_Dev_0): Skipping analysis function "ReachTargetVoltage".
  The stimset has too many sweeps, increase the size of DAScales.

@timjarsky timjarsky removed their assignment Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timjarsky timjarsky timjarsky approved these changes

@MichaelHuth MichaelHuth Awaiting requested review from MichaelHuth MichaelHuth is a code owner

Assignees

@t-b t-b

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@t-b @timjarsky