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

Add TTL support to SF #1822

Merged
merged 18 commits into from
Aug 3, 2023
Merged

Add TTL support to SF #1822

merged 18 commits into from
Aug 3, 2023

Conversation

MichaelHuth
Copy link
Collaborator

close #1817

@MichaelHuth MichaelHuth requested a review from timjarsky as a code owner July 15, 2023 03:24
@MichaelHuth MichaelHuth self-assigned this Jul 15, 2023
@MichaelHuth MichaelHuth requested a review from t-b as a code owner July 15, 2023 03:24
@MichaelHuth MichaelHuth force-pushed the feature/1822-Add_TTL_support_to_SF branch 4 times, most recently from 363203c to ed4065a Compare July 20, 2023 01:52
@MichaelHuth
Copy link
Collaborator Author

@t-b The CI seems to fail when signing the installer atm.

@MichaelHuth MichaelHuth assigned t-b and unassigned MichaelHuth Jul 20, 2023
@t-b
Copy link
Collaborator

t-b commented Jul 20, 2023

@t-b The CI seems to fail when signing the installer atm.

Thanks. I've thrown out the RDP user. With an RDP user logged in installer signing is not possible (security feature !!1elf)

@t-b t-b assigned MichaelHuth and unassigned t-b Jul 20, 2023
@MichaelHuth MichaelHuth force-pushed the feature/1822-Add_TTL_support_to_SF branch 2 times, most recently from 3e19a95 to 1340c20 Compare July 21, 2023 02:17
@t-b
Copy link
Collaborator

t-b commented Jul 21, 2023

Review:

7c8c65e (Util: Add GetUsedHWDACFromLNB helper function to retrieve hardware type from LNB, 2023-07-20)
d5512a4 (Util: Introduce variable for channel type in CreateTiledChannelGraph, 2023-07-15)
adc3ca4 (Util: Add GUI channel number to TUD in CreateTiledChannelGraph, 2023-07-15)
b604f78 (BSP: Use GUI channel number only in BSP_AddTracesForEpochs, 2023-07-15)
efc73ad (BSP: fix splitTTL epochs dependent control not properly toggled by VisEpochs, 2023-07-15)
8077678 (BSP: Always set axis range for TTL channels to 0 - 1 with 2 ticks, 2023-07-15)
6ae4864 (Util: Use GUI channel number for axis names in CreateTiledChannelGraph, 2023-07-15)

Looks all good to me.

1964894 (SF: Add TTL channel support to select operation and unassoc DA support, 2023-07-17)

When you create selectData as in

WAVE selectData = SFH_NewSelectDataWave(numSweeps, NUM_DA_TTL_CHANNELS + NUM_AD_CHANNELS)

the size should now be incremented by NUM_DA_TTL_CHANNELS as we can have now all AD/DA/TTL channels active or?

Missing tests for TTL and unassoc DA/AD. Please also make them not require hardware as those run so much faster.

f42e444 (Util: Add GetDAQDataSingleColumnWaveNG as more generic function, 2023-07-18)
cde09a7 (SF: Adapt SFH_GetSweepsForFormula to support epochs for TTL channels, 2023-07-18)
cb4a774 (SF: Use EP_FetchEpochs in SF_OperationEpochsImpl instead of direct retrieval, 2023-07-18)
da7dd1d (Tests: Adapt FillFakeDatabrowserWindow to not setup invalid channel numbers, 2023-07-18)
1340c20 (SFH: Remove trailing ; in old epoch name fallback in SFH_GetEpochNamesFromInfo, 2023-07-20)

Looks all good.

I'm wondering if the code for mapping AD to DA when fetching epochs

            fetchEpChanType = chanType == XOP_CHANNEL_TYPE_ADC ? XOP_CHANNEL_TYPE_DAC : chanType
            fetchEpChannelNumber = chanType == XOP_CHANNEL_TYPE_ADC ? SFH_GetDAChannel(graph, sweepNo, chanType, chanNr) : chanNr
            if(IsNaN(fetchEpChannelNumber))
                continue
            endif
            WAVE/T/Z epochInfo = EP_GetEpochs(numericalValues, textualValues, sweepNo, fetchEpChanType, fetchEpChannelNumber, allEpochsRegex)

should be part of EP_GetEpochs? Maybe as opt-in with an optional parameter? We do have extensive tests in UTF_EpochsWoHardware.ipf. We could then also factor out the channel mappting from SFH_GetDAChannel as fetching the LBN is quite wasteful if we have it already.

@t-b t-b mentioned this pull request Jul 25, 2023
Closed
26 tasks
@MichaelHuth MichaelHuth force-pushed the feature/1822-Add_TTL_support_to_SF branch 3 times, most recently from 0fe3568 to 561a02d Compare July 27, 2023 12:30
@t-b
Copy link
Collaborator

t-b commented Jul 27, 2023 •
edited
Loading

Please refrain from touching the SF notebook, as I'm doing that in #1468 as well. I've created #1827 to track that task. (Note: You are not doing that in the current PR)

@t-b
Copy link
Collaborator

t-b commented Jul 27, 2023

Review:
7c8c65e (Util: Add GetUsedHWDACFromLNB helper function to retrieve hardware type from LNB, 2023-07-20)

The changes to NWB_GenerateDeviceDescription are not 100% correct. You removed the ParseDeviceString logic which would kick in for ITC hardware and no "Digitizer Hardware Name" LBN entry.

d5512a4 (Util: Introduce variable for channel type in CreateTiledChannelGraph, 2023-07-15)

Good.

adc3ca4 (Util: Add GUI channel number to TUD in CreateTiledChannelGraph, 2023-07-15)

Jeep.

b604f78 (BSP: Use GUI channel number only in BSP_AddTracesForEpochs, 2023-07-15)

very nice.

efc73ad (BSP: fix splitTTL epochs dependent control not properly toggled by VisEpochs, 2023-07-15)

Jeep.

8077678 (BSP: Always set axis range for TTL channels to 0 - 1 with 2 ticks, 2023-07-15)

Okay.

6ae4864 (Util: Use GUI channel number for axis names in CreateTiledChannelGraph, 2023-07-15)

Hmmh.

b1c8922 (SF: Add TTL channel support to select operation and unassoc DA support, 2023-07-17)

RST docu:

The clampMode selection is only applied for AD/DA channels.

I would say

The clampMode selection is only applied for associated AD/DA channels.

dd01f03 (Util: Add GetDAQDataSingleColumnWaveNG as more generic function, 2023-07-18)

Nice!

55d8a76 (SF: Adapt SFH_GetSweepsForFormula to support epochs for TTL channels, 2023-07-18)

Looks good.

449ec57 (SF: Use EP_FetchEpochs in SF_OperationEpochsImpl instead of direct retrieval, 2023-07-18)

typo in commit message: fot -> for

7549ed3 (Tests: Adapt FillFakeDatabrowserWindow to not setup invalid channel numbers, 2023-07-18)
764d2b0 (SFH: Remove trailing ; in old epoch name fallback in SFH_GetEpochNamesFromInfo, 2023-07-20)

All good

1f8ca0c (EP: Remove unnecessary code in EP_GetEpochs, 2023-07-26)

Jeep.

1872640 (EP: Move AD to DA channel conversion into EP_FetchEpochs, 2023-07-26)

Can you add a test for the AD->DA channel mapping to UTF_EpochswoHardware.ipf?

The test should test EP_GetEpochs but as that uses EP_FetchEpochs we will
ensure that the latter works.

This should be relatively straightforward as PrepareEpochsTable_IGNORE also
uses your now enhanced PrepareLBN_IGNORE.

561a02d (Changes select test, 2023-07-27)

Jeep.

From playing around:

  • The TTL data in SF should have the same colors as in the DB/SB. For AD/DA we
    do preserve the colors, so SF_GetTraceColor needs adaptation for TTL I guess.

Please assign to Tim once done.

@t-b t-b assigned t-b and MichaelHuth and unassigned MichaelHuth and t-b Jul 27, 2023
@t-b
Copy link
Collaborator

t-b commented Jul 27, 2023

A screenshot for showing the color coding issue:

image

@MichaelHuth MichaelHuth force-pushed the feature/1822-Add_TTL_support_to_SF branch from 561a02d to 5d33d71 Compare July 28, 2023 12:44
@MichaelHuth MichaelHuth force-pushed the feature/1822-Add_TTL_support_to_SF branch from 5d33d71 to 8a9abff Compare July 28, 2023 16:05
@MichaelHuth MichaelHuth assigned timjarsky and unassigned MichaelHuth Jul 28, 2023
MichaelHuth added 18 commits July 28, 2023 18:21
@MichaelHuth
Util: Add GetUsedHWDACFromLNB helper function to retrieve hardware ty…
9e85429 
…pe from LNB

- to simplify the task to determine the hardware type at the point when a sweep was
  measured and the associated LNB entries were created, the helper function
  GetUsedHWDACFromLNB was created.

- Unify method in other locations that retrieve the hardware type from the LNB
@MichaelHuth
Util: Introduce variable for channel type in CreateTiledChannelGraph
5dda065 
- better code readability
- no functional change
@MichaelHuth
Util: Add GUI channel number to TUD in CreateTiledChannelGraph
31e201b 
- Having the GUI channel number simplifies code that use trace user data
  information
@MichaelHuth
BSP: Use GUI channel number only in BSP_AddTracesForEpochs
978a987 
- previously the hardware channel number from TUD needed to be mapped back to
  the GUI channel number. Now TUD also contains the GUI channel number.
@MichaelHuth
BSP: fix splitTTL epochs dependent control not properly toggled by Vi…
37f9f5c 
…sEpochs

- AdaptDependentControls logic was setup wrong

since
df25647 (BSP: Add TTL channel support to BSP_AddTracesForEpochs, 2023-07-07)
@MichaelHuth
BSP: Always set axis range for TTL channels to 0 - 1 with 2 ticks
015c7a7 
- this removes the default ticks for TTL channels on NI hardware
- view composite TTL channel from ITC hardware is more for debugging purposes
@MichaelHuth
Util: Use GUI channel number for axis names in CreateTiledChannelGraph
79b3c4d 
- such that for ITC TTL channel the TTLx_y with x hw channel and y ttlBit
  is replaced by TTLx where x is the GUI channel number
@MichaelHuth
SF: Add TTL channel support to select operation and unassoc DA support
d404f13 
- clampMode filter is applied for DA/AD but ignored for TTL channels
@MichaelHuth
Util: Add GetDAQDataSingleColumnWaveNG as more generic function
e346d7f 
than GetDAQDataSingleColumnWave. The new function uses GUI channel numbers
and can be called independent of the hardware.
@MichaelHuth
SF: Adapt SFH_GetSweepsForFormula to support epochs for TTL channels
7566572 
- the new utility function GetDAQDataSingleColumnWaveNG wraps the channel
  number translation for TTL channels to retrieve the correct sweep wave.
- changed epoch fetching to allow TTL channels as well
@MichaelHuth
SF: Use EP_FetchEpochs in SF_OperationEpochsImpl instead of direct re…
bcd6d88 
…trieval

- use of generic utility function for epoch fetching
@MichaelHuth
Tests: Adapt FillFakeDatabrowserWindow to not setup invalid channel n…
466bc5c 
…umbers

FillFakeDatabrowserWindow was setting up 5 channels: 0, 2, 4, 6, 8 where 8
is an invalid channel number for DA.

We do not want to test against an invalid internal data state.
@MichaelHuth
SFH: Remove trailing ; in old epoch name fallback in SFH_GetEpochName…
5187b52 
…sFromInfo

- In EP_FetchEpochs we add a traling ; to the epoch tags if not present
  If we have a old non-list based epoch tag just with a name, this changes the
  name. Assuming a missing short name indicates an old epoch tag, the trailing
  semicolon is removed again to have a clean roundtrip.
@MichaelHuth
EP: Remove unnecessary code in EP_GetEpochs
6cedbf0 
- EP_FetchEpochs always returns a single channel epochs wave,
  thus, not Duplicate to create a single channel wave is required.

No functional change
@MichaelHuth
EP: Move AD to DA channel conversion into EP_FetchEpochs
790b03b 
- When epochs are retrieved AD channels are converted to the associated
  DA channels, as only DA channels carry epoch information.
  This was done in SFH_GetSweepsForFormula and in BSP_AddTracesForEpochs in
  the same way.
- This conversion was moved into EP_FetchEpochs and is done automatically now.
  The conversion in the other functions was removed.
- The utility function SFH_GetDAChannel was removed as it is now unused
@MichaelHuth
Util: Adapt TTL specific API of GetHeadstageColor, bugfix TTL colors
fb0b682 
- Previously the color indexing of the splitted TTL channels was wrongly
  offsetted by -1 for ITC and for NI it was channelNumber * 5 (also wrong)

  Since
6de687e (GetTraceColor: Increase the number of distinctive trace colors, 2015-07-04)

Changed API of GetHeadstageColor to take GUI channel numbers as argument and only a
flag to indicate splitted or unsplitted TTL channels.
The color table index is calculated by mapping the GUI channel numbers to the ITC
style channel numbers + ttlBit.
@MichaelHuth
SF: Add support for correct colors of TTL traces
186dc0c 
- TTL traces use the same color map as the SB/DB
@MichaelHuth
DB: Set "sep. TTL" checkbox by default to checked on DB/SB open
7da5363 
- increase DB panel version
@MichaelHuth MichaelHuth force-pushed the feature/1822-Add_TTL_support_to_SF branch from 8a9abff to 7da5363 Compare July 28, 2023 16:21
@timjarsky timjarsky assigned t-b and MichaelHuth and unassigned timjarsky Aug 3, 2023
@t-b t-b merged commit 628dd73 into main Aug 3, 2023
@t-b t-b deleted the feature/1822-Add_TTL_support_to_SF branch August 3, 2023 16:54
t-b added a commit that referenced this pull request Sep 5, 2023
@t-b
SweepFormula.ifn: Add missing text from #1822
3d9a21d 
Adding the text was postponed to avoid merge conflicts with #1468.

Close #1827.
t-b added a commit that referenced this pull request Sep 7, 2023
@t-b
SweepFormula.ifn: Add missing text from #1822
8e6d9e7 
Adding the text was postponed to avoid merge conflicts with #1468.

Close #1827.
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

@t-b t-b Awaiting requested review from t-b t-b is a code owner

Assignees

@t-b t-b

@MichaelHuth MichaelHuth

Labels
DataBrowser SweepFormula
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SweepFormula: Support TTL channels
3 participants
@MichaelHuth @t-b @timjarsky