Resize the transfer group index map to the total number of transfer groups

[rtzoeller]

• This contribution adheres to CONTRIBUTING.md.

What does this Pull Request accomplish?

Make the transfer group index map array size the total number of transfer groups, rather than the number of transfer groups in that specific thread. This array appears to be used to convert between the global transfer group id to the relative position of the transfer group in the owning thread.

We're being slightly wasteful with memory here: each thread is now getting an array equal in size to the total number of transfer groups, instead of either sharing a global copy of this array or shifting which index we look at in the originally sized array.

If t is the number of threads, g the total number of groups, and g_t is the number of groups in that thread, we previously allocating t * g_t = g bytes (and probably could continue to do so if we reworked some of the surrounding infrastructure), and are now allocating t * g bytes. That said, this change seemed to match the spirit of the existing code, and also seemed like the easiest implementation.

Why should this Pull Request be merged?

Fixes #39. Threads with multiple transfer groups were accidentally having the first transfer group configuration applied to all transfer groups (except possibly in certain cases, like being in the first thread) due to an out-of-bounds indexing operation returning zero.

What testing has been done?

• Tested a configuration with more than one transfer group per thread on VeriStand 2020 R3 (Windows).
• Tested a configuration with more than one transfer group per thread on VeriStand 2020 R3 (Linux x64).
• Validated with @agomez08 that this fixes the issue he is seeing.

Notes for reviewers

The modified VIs are responsible for building the transfer group index map. This map is consumed in DSF Core.lvlib:Plugin Thread.lvclass:Cast Command to Transfer Group.vi (and only there, as far as I can tell).

My understanding of Cast Command to Transfer Group.vi is that the incoming Transfer Group ID is the global index of the transfer group, and that the outgoing Transfer Group id is the local index within the thread. For example, if there are two threads, the first with one group and the second with two groups, an incoming Transfer Group ID of 1 would correspond to the first group in the second thread, and the expected output would be 0 as it is the first group in that thread.

[niveristand-diff-bot]

Bleep bloop!

LabVIEW Diff Robot here with some diffs served up hot for your pull request.

Notice something funny? Help fix me on my GitHub repo.

DSF Core.lvlib--Plugin Thread.lvclass--Initialize Transfer Groups.vi.png

DSF Core.lvlib--Plugin.lvclass--Initialize Threads.vi.png

[dbendele]

Changes to both VIs look good. Array Size does not change within For Loop in Initialize Threads VI, but there is no performance hit, and I like your clean, readable code.

[dbendele]

As an alternative to initializing the array of indices in Initialize Transfer Group VI, you could also initialize one array before the For Loop in Initialize Threads VI and pass that initialized array into Initialize Transfer Group VI.

[dbendele]

We should create a tech debt item to catch the error if number of transfer groups exceeds 255.

[rtzoeller]

As an alternative to initializing the array of indices in Initialize Transfer Group VI, you could also initialize one array before the For Loop in Initialize Threads VI and pass that initialized array into Initialize Transfer Group VI.

I'd prefer to keep the implementation details of Initialize Transfer Groups.vi encapsulated, rather than expose them to Initialize Threads.vi. There might be a slight performance cost (although somewhat doubtful, since the array would be copied/allocated either way), but it's a cost we only pay at startup.

[rtzoeller]

We should create a tech debt item to catch the error if number of transfer groups exceeds 255.

See work item 1172889.

[dbendele]

As an alternative to initializing the array of indices in Initialize Transfer Group VI, you could also initialize one array before the For Loop in Initialize Threads VI and pass that initialized array into Initialize Transfer Group VI.

I'd prefer to keep the implementation details of Initialize Transfer Groups.vi encapsulated, rather than expose them to Initialize Threads.vi. There might be a slight performance cost (although somewhat doubtful, since the array would be copied/allocated either way), but it's a cost we only pay at startup.

It was actually because you had to pass the size of the array to initialize into the subVI that prompted the thought. We now pass total number of groups into the subVI which seems like more information than it should need to initialize the index map for the subset of groups, but I agree that more of the details about initialization are encapsulated with current implementation.

[rtzoeller]

It was actually because you had to pass the size of the array to initialize into the subVI that prompted the thought. We now pass total number of groups into the subVI which seems like more information than it should need to initialize the index map for the subset of groups, but I agree that more of the details about initialization are encapsulated with current implementation.

Ahh, now I see what you're saying. That makes sense.

I'm going to leave it as-is, pending @niphilj's review.

[niphilj]

It was actually because you had to pass the size of the array to initialize into the subVI that prompted the thought. We now pass total number of groups into the subVI which seems like more information than it should need to initialize the index map for the subset of groups, but I agree that more of the details about initialization are encapsulated with current implementation.

Ahh, now I see what you're saying. That makes sense.

I'm going to leave it as-is, pending @niphilj's review.

I thought about this a lot, and tried to way the options. I do like the clarity of the code and even talked myself out of other options. Really, I appreciate the naming of the new terminal to be so clear, and ok with the code as-is.

[agomez08]

I tested this fix with 2 different configurations that exhibited the issue and the previously observed problem is not showing up anymore.
FYI @rtzoeller