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.

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

Enable multiple synchronized bitfiles in the RT engine #28

Merged
merged 12 commits into from
Jan 29, 2019

Conversation

buckd
Copy link
Collaborator

@buckd buckd commented Jan 25, 2019

What does this Pull Request accomplish?

Allows a single APU to be split across multiple FPGAs so users requiring higher channel count and/or more FPGA fabric can still use the custom device.

Why should this Pull Request be merged?

There are several users that have requested this capability. It will allow NI to close more opportunities and make customers more successful.

What testing has been done?

Cursory testing ensuring FPGAs remain synchronized through use of the custom device and monitoring the newly added synchronization channels. The behavior of the channels mirrors my benchmarks outside of the custom device, just testing synchronization of the FPGAs using LabVIEW.

More testing still needs to be done, including validating I/O signal values and synchronization and automated tests still need to be written. I am currently working on this, but want to get the review going.

Note: There are other files that need to be updated due to typedef changes, but I'm not including them here because it would make this review even more unmanageable. It doesn't affect functionality, but build and load times will be longer because each of those VIs will need to recompile. I will update those with a separate pull request after this goes in.

@buckd buckd requested a review from csjall as a code owner January 25, 2019 13:29
@buckd buckd requested a review from rtzoeller January 25, 2019 13:29
@rtzoeller
Copy link
Contributor

@buckd do we want to bump the version here? From my conversation with @csjall, I was under the impression we would bump the patch version for now (we did already, 1.3.3 -> 1.3.4), and bump the feature version (i.e. to 1.4.0) when we pulled it out from behind a feature toggle.

@buckd
Copy link
Collaborator Author

buckd commented Jan 25, 2019

We can do it that way. I must have missed that discussion (and the fact it was already bumped).

@rtzoeller
Copy link
Contributor

rtzoeller commented Jan 25, 2019

  • Should the input and output references use an in place element structure here? image
  • Can you explain to me why this warning might occur?
    image
  • Is FPGA run method.vi unused? You did not modify it, but it is adjacent to VIs you were touching.

@rtzoeller
Copy link
Contributor

rtzoeller commented Jan 25, 2019

  • Remove the pop-up behavior of the compiled data on deployment

@buckd
Copy link
Collaborator Author

buckd commented Jan 25, 2019

  • Should the input and output references use an in place element structure here? image

They could, but it's not necessary. This was copied from the single FPGA VI. The unbundle is only used to get the type for describing the refnum and the bundle then goes into the rest of the RT data. Since this is during initialization, it's not time critical.

  • Can you explain to me why this warning might occur?
    image

Let's say the Engine Simulation Toolkit IP isn't the only thing running on the FPGA. Maybe there's an SLSC backend or some other custom IP compiled to the same bitfile in order to utilize a single FPGA board to accomplish multiple things. During deployment, something else could run the bitfile before the Engine Simulation custom device gets to this code.

  • Is FPGA run method.vi unused? You did not modify it, but it is adjacent to VIs you were touching.

This appears to be unused. Removed from the library and repo.

@csjall
Copy link
Contributor

csjall commented Jan 25, 2019

  • Add a system test that deploys multi-FPGA system definition and check synchronization. Look at the EDS System tests for an example.

@rtzoeller
Copy link
Contributor

rtzoeller commented Jan 25, 2019

  • To enable testing, @rtzoeller needs to make the multi-FPGA feature toggle take the system definition feature toggle flag into consideration, i.e. make the custom device use the multi-fpga version regardless of the ini file's setting if it is already configured for multi-fpga.

rtzoeller and others added 2 commits January 28, 2019 09:27
When opening pages, we should use the multi-fpga key stored in the system definition. This prevents us from getting into weird states if the ini file is changed while the system explorer is open, and should allow us to use the multi-fpga codepaths in tests without setting the ini key.
@buckd
Copy link
Collaborator Author

buckd commented Jan 28, 2019

@csjall Test added. Results below.
Note there is no config override of the IP Address due to this issue.

testspass

@buckd buckd added enhancement New feature or request multi fpga Work for multiple FPGA support labels Jan 28, 2019
Copy link
Contributor

@csjall csjall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the test hardware is only supported under LabVIEW 2018 or later, we should skip the test if we are running in earlier versions. Be sure to skip deployment and undeployment as well. Add a skip message for more information.

@csjall
Copy link
Contributor

csjall commented Jan 29, 2019

Since the test hardware is only supported under LabVIEW 2018 or later, we should skip the test if we are running in earlier versions. Be sure to skip deployment and undeployment as well. Add a skip message for more information.

Go ahead and submit changes as is. Create a separate tech debt issue to skip this test on earlier year versions.

@buckd buckd merged commit f3b73b5 into master Jan 29, 2019
@buckd buckd deleted the dev/multi-bitfile-engine branch January 29, 2019 14:46
@buckd
Copy link
Collaborator Author

buckd commented Jan 29, 2019

Since the test hardware is only supported under LabVIEW 2018 or later, we should skip the test if we are running in earlier versions. Be sure to skip deployment and undeployment as well. Add a skip message for more information.

Issue filed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request multi fpga Work for multiple FPGA support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants