feat(example): Generic synchronization example #242

lilesj · 2018-03-27T17:14:17Z

This contribution adheres to CONTRIBUTING.md.
New tests have been created for any new features or regression tests for bugfixes.
tox successfully runs, including unit tests and style checks (see CONTRIBUTING.md).

TODO: Check the above boxes with an 'x' to indicate what steps you have taken in preparing this Pull Request.

What does this Pull Request accomplish?

Proposes a generic synchronization example to demonstrate how to setup and use start triggers with multiple sessions on different interfaces.

Why should this Pull Request be merged?

Improve ease of use and getting started experience for new users.

What testing has been done?

Additional testing is required.

coveralls · 2018-03-27T17:19:19Z

Coverage increased (+0.4%) to 68.067% when pulling c7347f0 on lilesj:add-generic-synchronization-example into efe3f4e on ni:master.

coveralls · 2018-03-27T17:19:19Z

Coverage increased (+9.0%) to 67.649% when pulling 7271e6d on lilesj:add-generic-synchronization-example into 4004cd3 on ni:master.

jashnani

Thanks for getting this example together.

Looks like flake8 caught some of the problems and failed the Travis CI, please fix those. I'd recommend running tox locally before pushing.

Few more things:

We should add a unit test for this. See Add CAN and LIN database creation examples #219 for an example on how to do this. Feel free to reach out to me if you have any questions.
We should update/add rst files for documentation (for sphinx to generate the appropriate html) Add CAN and LIN database creation examples #219 also has examples of this.

jashnani · 2018-03-27T17:47:26Z

nixnet_examples/generic_synchronization.py

+    synchronized_input.intf.can_term = constants.CanTerm.ON
+
+    #Connect the start trigger terminals for the synchronized interface
+    synchronized_input.connect_terminals(nixnet._cconsts.NX_TERM_FRONT_PANEL0, nixnet._cconsts.NX_TERM_START_TRIGGER)


nixnet._cconsts is internal-only. Consider making these constants public. For ideas, see constants.py

jashnani · 2018-03-27T18:02:55Z

nixnet_examples/generic_synchronization.py

+        for frame in receivedFrames:
+            rxTimestamp = frame.timestamp
+            print('Received frame: {}'.format(frame))            
+            print('    payload={}'.format(list(six.iterbytes(frame.payload))))


Not sure if the loop is needed here. Looks like we're just writing one frame at line 72.

Even if we modified the example to write multiple frames, I'm not sure if we should overwrite the timestamps this way.

Ditto for echoedFrames

jashnani · 2018-03-27T18:17:10Z

nixnet_examples/generic_synchronization.py

+        inp = six.moves.input('Hit enter to continue (q to quit): ')
+        if inp.lower() == 'q':
+            break
+    print('Data acquisition stopped.')


We're likely leaking memory and/or not giving up session resources here (because SessionBase.close never got called). For reasons mentioned in an issue above, we should encourage the use of with-statements when creating XNET sessions.

jashnani · 2018-03-27T19:59:35Z

nixnet_examples/generic_synchronization.py

+
+    #Create the objects for the synchronized interface
+    synchronized_input = nixnet.FrameInStreamSession(interface1)
+    synchronized_output = nixnet.FrameOutStreamSession(interface1)


We should use a with-statement when creating XNET objects because it's context manager is responsible for invoking the built-in __enter__ / __exit__ functions at the appropriate times. For XNET sessions in particular, we call close during __exit__ which is responsible for giving up resources and cleaning up memory.

I noticed you're creating four sessions, in which case nesting with-statements can make the code hard to read. You could use ExitStack for combining other context managers and cleanup functions. Let me know if you have other ideas or questions about using ExitStack.

jashnani · 2018-03-27T20:19:00Z

nixnet_examples/generic_synchronization.py

+
+    #Create the session objects for the synchronizing interface
+    synchronizing_input = nixnet.FrameInStreamSession(interface2)
+    synchronizing_output = nixnet.FrameOutStreamSession(interface2)


Are synchronizing and synchronized names appropriate here? I don't know enough about session synchronization to know what's suitable here. Would original_input and echo_input make more sense?

Also, is synchronized_output being used anywhere? I'm wondering if we could get away with just 3 sessions for this example.

epage · 2018-04-03T14:03:22Z

nixnet/_enums.py

@@ -2029,3 +2029,26 @@ class FrameType(enum.Enum):
    SPECIAL_DELAY = _cconsts.NX_FRAME_TYPE_SPECIAL_DELAY
    SPECIAL_LOG_TRIGGER = _cconsts.NX_FRAME_TYPE_SPECIAL_LOG_TRIGGER
    SPECIAL_START_TRIGGER = _cconsts.NX_FRAME_TYPE_SPECIAL_START_TRIGGER
+
+class Terminals(enum.Enum):


Since the constants are prefixed with TERM, should this enum be called Term?

iirc that was how I decided the name for all of the other enums

"Term" is ambiguous since we have both "Termination" and "Terminal" in the mix. For readability improvements mentioned below I suggest dropping the NX and spelling out terminal i.e TERMINAL_FRONTPANEL0.

Also, iirc we don't tend to name the enums as a plural, so if it is expanded, it'd be Terminal.

epage · 2018-04-03T14:04:08Z

nixnet/_enums.py

+
+class Terminals(enum.Enum):
+    "Terminals to import/export from the XNET device"
+    NX_TERM_FRONTPANEL0 = _cconsts.NX_TERM_FRONT_PANEL0


Because these are in an enum, we shouldn't have the NX_TERM / NXTERM prefixes

Why did FRONT_PANEL0 get turned into FRONTPANEL0? And should that be _0?

For the first pass I matched the names with their C counterparts for consistency. I don't have a strong argument for this approach and have no problem diverging to improve readability.

epage · 2018-04-03T14:05:09Z

nixnet/_enums.py

+    NXTERM_PXI_STAR = _cconsts.NX_TERM_PXI_STAR
+    NXTERM_PXI_CLK10 = _cconsts.NX_TERM_PXI_CLK10
+    NXTERM_10MHZTIMEBASE = _cconsts.NX_TERM_10_M_HZ_TIMEBASE
+    NXTERM_1MHZTIMEBASE = _cconsts.NX_TERM_1_M_HZ_TIMEBASE


What should be the word splitting on these?

1MHZ_TIMEBASE

1_MHZ_TIMEBASE

1_M_HZ_TIMEBASE

I lean towards: 1_MHZ_TIMEBASE

Agreed. I second going with #_MHZ_TIMEBASE.

epage · 2018-04-03T14:05:35Z

nixnet/_enums.py

+    NXTERM_MASTERTIMEBASE = _cconsts.NX_TERM_MASTER_TIMEBASE
+    NXTERM_COMMTRIGGER = _cconsts.NX_TERM_COMM_TRIGGER
+    NXTERM_STARTTRIGGER = _cconsts.NX_TERM_START_TRIGGER
+    NXTERM_LOGTRIGGER = _cconsts.NX_TERM_LOG_TRIGGER


How come the words were combined (STARTTRIGGER rather than START_TRIGGER)?

Both the XNET manual and the equivalent constant in the C API have the words joined as one. I will separate them for readability.

I think where the confusion is coming in is definition of "word". In programming, there are several conventions for splitting words beyond " ".

Capital letters (camelCase, PascalCase)

underscores (snake_case, SCREAMING_CASE)

dashes (arrow-case).

So we're switching from PascalCase to SCREAMING_CASE, so we replace the capital letter split with an underscore is the appropriate thing to do.

When I bootstrapped the API, I had a script automatically change the convention for us. It mostly worked (for example nxTerm_LogTrigger was correctly mapped to NX_TERM_LOG_TRIGGER). In some cases, it couldn't quite tell what the word boundaries are and got it very wrong (PDUs -> PD_Us -> pd_us). Other cases require some more thought on our part (should MHz be M_HZ or MHZ?).

For general reference, the constants look like

nxTerm_FlexRayStartCycle

nxTerm_PXI_Star

nxTerm_PXI_Trig4

The odd thing is that the C API is sometimes splitting with _. In most cases, it is used to separate phrases ("NIXNET Terminal" from "FlexRay Start Cycle") but "PXI" and "Star" are part of the same phrase.

epage · 2018-04-03T14:06:49Z

nixnet_examples/generic_synchronization.py

@@ -25,7 +25,7 @@ def main():
    synchronized_input.intf.can_term = constants.CanTerm.ON

    #Connect the start trigger terminals for the synchronized interface
-    synchronized_input.connect_terminals(nixnet._cconsts.NX_TERM_FRONT_PANEL0, nixnet._cconsts.NX_TERM_START_TRIGGER)
+    synchronized_input.connect_terminals(constants.Terminals.NX_TERM_FRONTPANEL0, constants.Terminals.NXTERM_STARTTRIGGER)


Don't you need to modify these functions

To convert from enum to numeric value

To document the change in data type?

fix(examples): Added a Terminals enum and modified the synchronization example to use it Modified example to include feedback. Added enum names for terminals. Modified the connect terminal class to accept new terminal enums. Added unit tests Cleaned up files to pass Flake8 tests. Replaced .Next() function with for loop to pull out a single frame. Modified example to use contextlib2 and Lists Adding contextlib2 to testing dependencies Modifying print text to use for loop Adding contextlib2 dependency to requirements_mypy Added contextlib2 to setup.py to resolve jenkins test failures Added comma to seperate six and context lib requirements Removed tests for Generic Synchronization Removed imports for Generic Synchronization Extending line in documentation Added contextlib2 to travis.yml Created synchronization example with tests and documentation

…achine

d-bohls · 2018-09-25T15:42:19Z

nixnet/_enums.py

+
+
+class Terminal(enum.Enum):
+    "Terminals to import/export from the XNET device"


Use triple quotes for docstrings.

https://www.python.org/dev/peps/pep-0257/#one-line-docstrings

d-bohls · 2018-09-25T15:42:27Z

docs/examples/generic_synchronization.rst

+Interface properties need only be set once per interface. The connect_terminals function
+acts like an interface property and needs to be set only once per interface. Listening
+sessions can then be started with the "Session Only" scope. The last session to be started
+on a particul interface can be started with the normal scope which will cause it to start


particular*

Might be nice to actually link to docstrings for things like "connect_terminals", "SESSION_ONLY", and any other relevant properties.

If possible, it's a good idea to get input from techcomms on docstrings.

d-bohls · 2018-09-25T15:46:52Z

nixnet_examples/generic_synchronization.py

+    interface2 = 'CAN2'
+
+    # Create the sessions. ExitStack() can be used for high session counts
+    with nixnet.FrameInStreamSession(interface1) as interface1_input:


More of a suggestion here; would this naming be a little more descriptive?

input_interface = 'CAN1' output_interface = 'CAN2' with nixnet.FrameInStreamSession(input_interface) as input_session: with nixnet.FrameOutStreamSession(output_interface) as output_session: ...

d-bohls · 2018-09-25T16:00:50Z

nixnet/_enums.py

+
+class Terminal(enum.Enum):
+    "Terminals to import/export from the XNET device"
+    FRONTPANEL_0 = _cconsts.NX_TERM_FRONT_PANEL0


Should FRONTPANEL_0 and FRONTPANEL_1 be removed? They are duplicates of FRONT_PANEL_0 and FRONT_PANEL_1 below.

jashnani self-requested a review March 27, 2018 17:39

jashnani assigned lilesj Mar 27, 2018

jashnani suggested changes Mar 27, 2018

View reviewed changes

epage suggested changes Apr 3, 2018

View reviewed changes

lilesj force-pushed the add-generic-synchronization-example branch from a2cc128 to b89e94f Compare June 19, 2018 23:39

lilesj added 2 commits September 23, 2018 12:55

Modified trigger routing to PXI_Trig0 to be compatible with CI test m…

c6a2ce9

…achine

Updated tests in test_session.py to use new connect terminal enums

c7347f0

d-bohls reviewed Sep 25, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(example): Generic synchronization example #242

feat(example): Generic synchronization example #242

lilesj commented Mar 27, 2018

coveralls commented Mar 27, 2018 •

edited

Loading

coveralls commented Mar 27, 2018

jashnani left a comment •

edited

Loading

jashnani Mar 27, 2018

jashnani Mar 27, 2018

jashnani Mar 27, 2018

jashnani Mar 27, 2018 •

edited

Loading

jashnani Mar 27, 2018 •

edited

Loading

epage Apr 3, 2018

lilesj Apr 3, 2018

epage Apr 3, 2018

epage Apr 3, 2018

lilesj Apr 3, 2018

epage Apr 3, 2018 •

edited

Loading

jashnani Apr 4, 2018

epage Apr 3, 2018

lilesj Apr 3, 2018

epage Apr 3, 2018

epage Apr 3, 2018

d-bohls Sep 25, 2018

d-bohls Sep 25, 2018

d-bohls Sep 25, 2018

d-bohls Sep 25, 2018



		class Terminal(enum.Enum):
		"Terminals to import/export from the XNET device"

feat(example): Generic synchronization example #242

Are you sure you want to change the base?

feat(example): Generic synchronization example #242

Conversation

lilesj commented Mar 27, 2018

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

coveralls commented Mar 27, 2018 • edited Loading

coveralls commented Mar 27, 2018

jashnani left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jashnani Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

jashnani Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 27, 2018 •

edited

Loading

jashnani left a comment •

edited

Loading

jashnani Mar 27, 2018 •

edited

Loading

jashnani Mar 27, 2018 •

edited

Loading

epage Apr 3, 2018 •

edited

Loading