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

[RFC] Add NI-TClk support #1005

Closed
wants to merge 71 commits into from
Closed

[RFC] Add NI-TClk support #1005

wants to merge 71 commits into from

Conversation

texasaggie97-zz
Copy link
Contributor

@texasaggie97-zz texasaggie97-zz commented Jun 17, 2019

  • This contribution adheres to CONTRIBUTING.md.
  • I've updated CHANGELOG.md if applicable.
  • I've added tests applicable for this pull request

What does this Pull Request accomplish?

  • Add nitclk to nimi-python - see below for details

List issues fixed by this Pull Request below, if any.

What testing has been done?

  • Flake8

Attached code does/does not reflect this description

Public API

Definitions

  • driver session - Class that encapsulates the session in Python - for example niscope.Session
  • driver session num - this is the actual number that represents the session - for IVI drivers, this is returned when creating the IVI session
  • tclk session num - this is the actual number that represents the session to tclk - this may or may not be the same actual number as driver session num

SessionReference

New class that encapsulates the tclk session num. This class does not open or close any sessions. It is instantiated with the tclk session num.

nimi-python based driver sessions will include a tclk member that is the SessionReference associated with that session.

import niscope
scope = niscope.Session('dev1')
print(type(scope.tclk))

Will produce something like nitclk.SessionReference

Any place that needs a tclk session num in the C API can accept:

  • nimi-python Session (assuming NI-TClk support enabled)
  • nitclk.SessionReference
  • tclk session num

Any place that returns a tclk session num from the C API will return a nitclk.SessionReference

Function

NI-TClk is a stateless API, I.e. there is no session that ties all the different function calls together like other IVI based drivers. Because of this there is no Session object in the public API. Instead, the API consists of functions that take the list of sessions to act on. For example:

def synchronize(sessions, min_time):
    ...
import nifgen
import niscope
import nitclk

scope = niscope.Session('dev1')
fgen = nifgen.Session('dev2')
# scope1 configure
fgen.write_waveform(calculated_sine)
nitclk.configure_for_homogeneous_triggers([scope, fgen])
nitclk.synchronize([scope, fgen], 0.00042)
nitclk.initiate([fgen, scope])
wfm = scope.fetch()
# Do something with data
scope.close()
fgen.close()

sessions is an iterable where each item is one of the list above.

The reason for not using a session based API is that different calls to nitclk functions may need to act on a different subset of the sessions.

Properties

Because properties are set per session, we need to handle them differently.

Properties will be added to the SessionReference class. They will then be set just like properties on driver sessions are.

Then any nimi-python driver that claims support for nitclk will add a tclk attribute which will be a nitclk.SessionReference for that driver session.

import niscope

scope1 = niscope.Session('dev1')
scope2 = niscope.Session('dev2')
scope1.nitclk.sample_clock_delay = .0000042
scope2.nitclk].sample_clock_delay = .0000043

At this point we will not support repeated capabilities on NI-TClk properties. We will open an issue regarding the one property that does use repeated capabilities and use that to track priority and need for this feature.

Packaging

Any driver that declares in metadata that it supports nitclk will have a dependency added for the nitclk package. This way when you pip install niscope you will also get nitclk support. We know that if the driver is installed, then NI-TClk is installed as well, and we know that other ADEs also install NI-TClk support when the driver support is installed.

We could make nitclk being installed optional, but the cost of installing it is very small and it would complicate the generated code quite a bit in order to handle it not being installed. Unlike numpy which is optional, nitclk from PyPI doesn't install anything else whereas numpy installs DLLs in addition to the Python code.

Implementation details

Functions

Internally, there is a class that contains the actual implementation of the functions. The main purpose of this is to allow reusing the default function template as is. Not doing this would require either a separate template whose only difference is indentation, or adding variable indentation to the existing template which would make it very messy. @marcoskirsch didn't like variable indenting when originally attempted for something in the pre 1.0 time frame.

This class will be instantiated as a singleton and each call to the function will get this singleton and then call the actual implementation.

def _get_session_class():
    '''Internal function to return session singleton'''
    global _session_instance
    global _session_instance_lock

    with _session_instance_lock:
        if _session_instance is None:
            _session_instance = _Session()

        return _session_instance


def wait_until_done(sessions, timeout):
    '''wait_until_done

    <doc string snipped for brevity>
    '''
    return _get_session_class().wait_until_done(sessions, timeout)

* Enable nidmm build

* config_addon.py is for module_version only (so far)

* Remove all addon information

* Update generated metadata from hapigen

* Update generated files

* Don't use numpy 1.16.x - broken on pypy

* Don't use quotes

* Update metadata based on chanes in extraction

* Update generated files
The original way quit working for some reason that I was not able to track down
* Update metadata

* Add config_addon

* Remove unused addn information

* Workaround for pypy test failing

* Enable niscope

* Update generated files

* Update changelog with changed enum values
* Enable niswitch

* Workaround for numpy with pypy

* Add config_addon.py for module_version

* Update metadata

* Update metadata

* Update generated files
* Add api version information to status documentation

* Move 'last_test_version' to config_addon

This will have to be manually updated as nimi-bot or other testing happens

* Version from generated metadata is really the version of the API metadata, not the version tested with

* Update generated files

* Make key name more descriptive

* Update

* Update generated files
@codecov
Copy link

codecov bot commented Jun 24, 2019

Codecov Report

Merging #1005 into master will decrease coverage by 0.36%.
The diff coverage is 76.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1005      +/-   ##
==========================================
- Coverage   91.33%   90.97%   -0.37%     
==========================================
  Files          23       23              
  Lines        3601     3611      +10     
==========================================
- Hits         3289     3285       -4     
- Misses        312      326      +14
Impacted Files Coverage Δ
generated/nifake/session.py 97.74% <100%> (ø) ⬆️
generated/nimodinst/session.py 95.95% <100%> (ø) ⬆️
build/helper/codegen_helper.py 87.64% <18.75%> (-4.24%) ⬇️
build/helper/metadata_add_all.py 79.23% <50%> (ø) ⬆️
build/helper/documentation_helper.py 88.83% <0%> (+0.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ea3ec1...cabe80f. Read the comment docs.

@texasaggie97-zz
Copy link
Contributor Author

I believe all comments have been addressed and the code has been updated.

If there are no other concerns, is NI-TClk's Python API approved?

@marcoskirsch
Copy link
Member

If there are no other concerns, is NI-TClk's Python API approved?

I think it's ready for a more formal review.

@texasaggie97-zz
Copy link
Contributor Author

Closed by other PRs culminating with #1054

@texasaggie97-zz texasaggie97-zz deleted the bug293/add_tclk branch October 21, 2019 15:31
@texasaggie97-zz texasaggie97-zz restored the bug293/add_tclk branch December 18, 2019 21:53
@texasaggie97-zz texasaggie97-zz deleted the bug293/add_tclk branch December 18, 2019 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add NI-TClk Support
5 participants