CAN Frame Stream IO Example + Relevant OOP layer changes by jashnani · Pull Request #42 · ni/nixnet-python

jashnani · 2017-06-27T20:29:27Z

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).

What does this Pull Request accomplish?

Adds Output to the CAN Frame Stream Input example and enhances the OOP API layer as needed.

Why should this Pull Request be merged?

This is a stepping stone to fleshing out the API.

What testing has been done?

None

epage · 2017-06-27T20:46:26Z

nixnet/types.py



-class CanFrame(object):
+class CANFrame(object):


Please revert this back to standard PascalCase

epage · 2017-06-27T20:47:10Z

nixnet_examples/can_frame_stream_io.py

+    with nx.Session(database_name, cluster_name, list, interface1, input_mode) as input_session:
+        print('Input session created successfully.')
+        with nx.Session(database_name, cluster_name, list, interface2, output_mode) as output_session:
+            print('Output session created successfully.')


I don't think we need these "successful" prints

I was following what XNET has for the C examples but maybe these are not necessary.

I think it might make a little more sense in C which is compiled and doesn't have that great of error reporting mechanisms.

I don't mind leaving the console output verbose. Having the extra output can't hurt a new user.

epage · 2017-06-27T20:47:58Z

nixnet_examples/can_frame_stream_io.py

+            # Start the input session manually to make sure that the first
+            # frame value sent before the initial read will be received.
+            input_session.start(constants.StartStopScope.NORMAL)
+            print('Input session started manually.')


Are we sure we need this print?

Same as above

epage · 2017-06-27T20:48:59Z

nixnet_examples/can_frame_stream_io.py

+            try:
+                id = int(six.input('Enter identifier: '))
+            except ValueError:
+                print('Not a number')


This will still crash when ValueError is thrown but instead because id is assigned never assigned in the except clause

Maybe qualify the expected data format in the request. Something like 'Enter decimal identifier: 0d'

I've formatted the request to incorporate the type we expect.
I'm now setting id in except as well.

epage · 2017-06-27T20:49:07Z

nixnet_examples/can_frame_stream_io.py

+            try:
+                payload_list = [int(x) for x in six.input('Enter payload: ').split()]
+            except ValueError:
+                print('Not a number')


ditto about crashing

Same as above. Elaborate on the expected format of the user input in the string.

Fixed same as above

epage · 2017-06-27T20:49:55Z

nixnet_examples/can_frame_stream_io.py

+            while True:
+                for byte in payload:
+                    byte = byte + i
+                with types.CANFrame(id, extended, constants.FRAME_TYPE_CAN_DATA, payload) as frame:


why is this using a with-statement?

epage · 2017-06-27T20:50:43Z

nixnet_examples/can_frame_stream_io.py

+                for byte in payload:
+                    byte = byte + i
+                with types.CANFrame(id, extended, constants.FRAME_TYPE_CAN_DATA, payload) as frame:
+                    output_session.write_frame(list(frame), number_of_bytes_for_frames, 10)


why list(frame)? Do you mean [frame]?

Why are you passing the number of bytes?

What is 10?

Yes I meant [frame]

I thought I needed to based on incorrect assumption.

timeout - I've added a variable for it now

epage · 2017-06-27T21:08:31Z

nixnet/constants.py


 TIMEOUT_NONE = _cconsts.NX_TIMEOUT_NONE
 TIMEOUT_INFINITE = _cconsts.NX_TIMEOUT_INFINITE
+FRAME_TYPE_CAN_DATA = _cconsts.NX_FRAME_TYPE_CAN_DATA


#43 is adding a constants.FrameType.CAN_DATA that you should instead be using

epage · 2017-06-27T21:09:24Z

nixnet_examples/can_frame_stream_io.py

+            i = 0
+            while True:
+                for byte in payload:
+                    byte = byte + i


Whats this loop trying to do?

I've fixed the loop to do what I was intending. Increment the payload values before each write.

epage · 2017-06-27T22:58:57Z

nixnet/nx.py

+
+    @intf_can_term.setter
+    def intf_can_term(self, value):
+        _props.set_session_intf_can_term(self._handle, value)


I've gone ahead and merged #45. Can you rebase this onto it and use my version of these properties?

lilesj · 2017-06-28T16:27:33Z

nixnet_examples/can_frame_stream_io.py

+            else:
+                input_session.intf_can_term = constants.CanTerm.ON
+                output_session.intf_can_term = constants.CanTerm.ON
+


We should explicitly set termination for both interfaces for clarity. I.e
if terminated_cable.lower() == "y":
output_session.intf_can_term = constants.CanTerm.OFF
input_session.intf_can_term = constants.CanTerm.ON

lilesj · 2017-06-28T16:41:49Z

nixnet_examples/can_frame_stream_io.py

+                for frame in frames:
+                    print('Received frame: ')
+                    pp.pprint(frame)
+


We may want to slow the loop down a bit so we don't have a wall of text in the output. 1 Second should be sufficient.
We can also format the output to be on a single line. Maybe have a initial line that is a header with the following lines formatted with data like so:
Data Transmitted | Data Received
FF FF FF 0A 1B CF FF 00 FF FF FF 0A 1B CF FF 00
FF FF FF 0A 1B CF FF 01 FF FF FF 0A 1B CF FF 01...

I've slowed down the loop to 1 second.

I spoke to Jeff about changing the printing format and we decided that it's not critical at the moment and we'll handle example polishing in a different PR.

lilesj

The overall example structure looks good to use as a baseline for further examples.

jashnani added the enhancement label Jun 27, 2017

jashnani added this to the 0.1 milestone Jun 27, 2017

jashnani requested review from epage and lilesj June 27, 2017 20:29

jashnani self-assigned this Jun 27, 2017

epage suggested changes Jun 27, 2017

View reviewed changes

epage reviewed Jun 27, 2017

View reviewed changes

lilesj reviewed Jun 28, 2017

View reviewed changes

Ajay Jashnani added 4 commits June 28, 2017 15:10

Adding Output to the frame stream input example

19f9272

Rename example file

28fbb7d

Fixing compilation errors

d68ca80

Addressing review comments

517d1c4

jashnani force-pushed the frameStreamExample branch from b686b31 to 517d1c4 Compare June 28, 2017 20:29

Fix CanFrame reference

5b5faad

epage approved these changes Jun 28, 2017

View reviewed changes

jashnani merged commit 50bac2d into ni:master Jun 28, 2017

epage mentioned this pull request Jun 28, 2017

Create Frame Stream I/O Example #33

Closed



		class CanFrame(object):
		class CANFrame(object):

Conversation

jashnani commented Jun 27, 2017

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lilesj left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants