feat(API): Support lin_sched #142

bcornish · 2017-07-25T23:13:34Z

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).
Document example
Integration tests: refactor can frame parameters
Add integration tests: lin frame

What does this Pull Request accomplish?

Added read and write frame for LIN. Added LIN frame object.

Why should this Pull Request be merged?

Stepping stone for LIN support.

What testing has been done?

Doc tests only

coveralls · 2017-07-25T23:17:13Z

Coverage increased (+0.02%) to 63.99% when pulling b1dfd76 on bcornish:linframe into 6af98b1 on ni:master.

coveralls · 2017-07-25T23:17:14Z

Coverage increased (+0.02%) to 63.99% when pulling b1dfd76 on bcornish:linframe into 6af98b1 on ni:master.

epage

Thanks for getting this together. Overall, it looks good!

epage · 2017-07-26T14:54:36Z

nixnet/_session/frames.py

+    def read_lin(
+            self,
+            number_to_read,
+            timeout=constants.TIMEOUT_NONE):


Please apply a type annotation

I'm guessing it'd be

# type: (int, float) -> typing.Iterable[types.LinFrame]

ditto for all other functions

epage · 2017-07-26T14:55:53Z

nixnet/types.py

+        payload: A byte string representing the payload.
+        eventid: Identifier for an event-triggered slot.
+        eventslot: A boolean indicating whether the frame was received within an
+            event-triggered slot or an unconditional or sporadic slot.


Please document the expected type for each of these

epage · 2017-07-26T14:57:19Z

nixnet/types.py

+
+    _FRAME_ID_MASK = 0x000000FF
+
+    def __init__(self, identifier, type, eventid=b"", payload=b""):


why do you default eventid to a bytestring here but pass in an int elsewhere? What should its type be?

(also, this is the failure that Travis caught)

epage · 2017-07-26T14:59:33Z

nixnet/types.py

+                eventid=0x0, eventslot=False, payload=...)
+        """
+        return """LinFrame(identifier=0x{:x}, echo={}, type={}, timestamp=0x{:x},
+        eventid=0x{:x}, eventslot={}, payload=...)""".format(


We should probably not have a multi-line repr

epage · 2017-07-26T15:30:50Z

nixnet/types.py

+        "eventslot",
+        "payload"]
+
+    _FRAME_ID_MASK = 0x000000FF


Is this mask correct?

When Type specifies a LIN frame, this element contains a number in the range 0–63 (inclusive). This number is the LIN frame's ID (unprotected).

http://zone.ni.com/reference/en-XX/help/372841L-01/nixnet/rawframeformat_c/

epage · 2017-07-26T15:32:55Z

nixnet/types.py

+        lin_frame = LinFrame(identifier, constants.FrameType(frame.type), frame.payload)
+        lin_frame.timestamp = frame.timestamp
+        lin_frame.echo = bool(frame.flags & _cconsts.NX_FRAME_FLAGS_TRANSMIT_ECHO)
+        lin_frame.eventid = frame.info


Shouldn't you only read from frame.info if eventslot is true?

For LIN frames, if bit 0 of the Flags element is clear, the Info element is unused (0). If bit 0 of the Flags element is set (event-triggered entry), the Info element contains the event-triggered frame ID, and the Identifier element contains the Unconditional ID from the first payload byte.

epage · 2017-07-26T15:36:18Z

nixnet/types.py

+        "timestamp",
+        "eventid",
+        "eventslot",
+        "payload"]


Reading the documentation, it seems like eventslot impacts the meaning of identifier and eventid

We should at least re-order it so eventslot is always referenced before eventsid to communicate this. I'd suggest putting it before identifier but that'll mess up __init__s defaulting of parameters.

http://zone.ni.com/reference/en-XX/help/372841L-01/nixnet/rawframeformat_c/

epage · 2017-07-26T15:38:03Z

nixnet/types.py

+        self.type = type
+        self.timestamp = 0  # Used only for Read
+        self.eventid = eventid  # Used only for Read
+        self.eventslot = False  # Used only for Read


eventid is allowed to be passed in but not eventslot? If eventid has anything, then eventslot should be True.

Maybe not include eventid in the parameter list and hard code it to None so that it works well for people creating a LinFrame for writing (where they'll want a payload)

epage · 2017-07-26T17:33:43Z

Note: I made fixes to __eq__ and __ne__ in #143. Please reflect this in your PR.

coveralls · 2017-07-28T08:19:44Z

Coverage increased (+0.09%) to 64.327% when pulling 2bcbf0f on bcornish:linframe into 13a2757 on ni:master.

epage · 2017-07-28T22:02:17Z

./nixnet/__init__.py:10:1: F403 'from nixnet.session import *' used; unable to detect undefined names
from nixnet.session import *  # NOQA: F401, F403
^
ERROR: InvocationError: '/home/travis/build/ni/nixnet-python/.tox/flake8/bin/flake8'

I submitted a PR that should fix that. Rebase and re-push.

Also, when you rewrite the history for your PR, we don't get a notification on push so you'll have to let us know when there are changes you want us to look over.

coveralls · 2017-07-28T22:42:25Z

Coverage increased (+0.09%) to 64.327% when pulling ef41eb8 on bcornish:linframe into cce79f2 on ni:master.

bcornish · 2017-07-28T22:43:23Z

Rebased and repushed as requested.

epage · 2017-07-31T13:19:38Z

nixnet/types.py

@@ -6,6 +6,8 @@
 import collections
 import typing  # NOQA: F401

+# flake8: NOQA: E501
+


We should isolate suppressions to the lines they impact

So we know if they can be removed

So we still get reports on other lines

epage · 2017-07-31T13:20:44Z

nixnet/_session/frames.py

@@ -222,7 +222,7 @@ def write(
            frames,
            timeout=10):
        # type: (typing.Iterable[types.Frame], float) -> None
-        """Write raw CAN frame data.
+        """Write raw CAN or LIN frame data.


Most of these comments are out of date, sorry for that.

Please change them to be "Read frames" (no reference to "raw", "CAN", or "LIN").

epage · 2017-07-31T13:22:30Z

nixnet/types.py

        }.get(frame.type)
        if frame_type is None:
            raise NotImplementedError("Unsupported frame type", frame.type)
        return frame_type.from_raw(frame)
+
+
+class LinFrame(object):


Organizationally, I'd move LinFrame before the XnetFrame

I moved LinFrame before XnetFrame and all of the special frames.

epage · 2017-07-31T13:25:19Z

nixnet/types.py

@@ -388,7 +391,115 @@ def from_raw(cls, frame):
            constants.FrameType.CANFD_DATA: CanFrame,
            constants.FrameType.CANFDBRS_DATA: CanFrame,
            constants.FrameType.CAN_REMOTE: CanFrame,
+            constants.FrameType.LIN_DATA: LinFrame,
+            constants.FrameType.LIN_BUS_ERROR: LinFrame,


Looking at the documentation for LIN_BUS_ERROR, it seems like we should create a custom frame for it.
http://zone.ni.com/reference/en-XX/help/372841L-01/nixnet/specialframes_c/

Like CAN, it might be reasonable to defer it.

epage · 2017-07-31T13:26:15Z

nixnet/types.py

@@ -388,7 +391,115 @@ def from_raw(cls, frame):
            constants.FrameType.CANFD_DATA: CanFrame,
            constants.FrameType.CANFDBRS_DATA: CanFrame,
            constants.FrameType.CAN_REMOTE: CanFrame,
+            constants.FrameType.LIN_DATA: LinFrame,
+            constants.FrameType.LIN_BUS_ERROR: LinFrame,
+            constants.FrameType.LIN_NO_RESPONSE: LinFrame,


Where is LIN_NO_RESPONSE documented? Trying to decide if it makes sense to map it to a LinFrame

It's also a special frame:
http://zone.ni.com/reference/en-XX/help/372841N-01/nixnet/specialframes/

Should it be deferred with the rest of the special frame types?

Didn't scroll far enough :)

Let's defer it to a future PR.

coveralls · 2017-08-02T00:18:19Z

Coverage increased (+0.08%) to 65.235% when pulling 8a2c948 on bcornish:linframe into e1bf22e on ni:master.

epage · 2017-08-02T15:44:14Z

Thinking some more about it, if you add equality tests and increased your to/from raw coverage, this becomes sufficiently tested for what is being added that we might be able to submit it now.

(these tests will need to be added anyways).

epage · 2017-08-07T17:07:59Z

nixnet/types.py

+        else:
+            return NotImplemented
+
+    def __repr__(self):


FYI in #170, I changed the other frame reprs based on feedback.

bcornish · 2017-08-08T15:27:51Z

Added lin_sched and diagnostic_lin_sched functions. Submitting updates to reprs and equality tests in separate commits.

coveralls · 2017-08-08T15:48:22Z

Coverage increased (+0.03%) to 64.204% when pulling 3fcdedb on bcornish:linframe into 5590c92 on ni:master.

coveralls · 2017-08-08T15:48:22Z

Coverage increased (+0.03%) to 64.204% when pulling 3fcdedb on bcornish:linframe into 5590c92 on ni:master.

epage

Glad to see how well you were able to look at the other code and API and figure out what to do

epage · 2017-08-08T15:50:18Z

nixnet/_session/base.py

@@ -335,6 +335,37 @@ def disconnect_terminals(self, source, destination):
        """
        _funcs.nx_disconnect_terminals(self._handle, source, destination)

+    def write_state_lin_schedule_change(self, state_value):
+        # type: () -> None


# type: (typing.Text) -> None

epage · 2017-08-08T15:51:43Z

nixnet/_session/base.py

+        because it cannot change to master while running.
+
+        Args:
+            state_value(str): Writes the desired state.


You have state_value documented as a str but the nxWriteState documentation says that it is an int`

http://zone.ni.com/reference/en-XX/help/372841L-01/nixnet/nxwritestate/

epage · 2017-08-08T15:52:44Z

nixnet/_session/base.py

+        Args:
+            state_value(str): Writes the desired state.
+        """
+        _funcs.nx_write_state(self._handle, constants.WriteState.LIN_SCHEDULE_CHANGE, state_value)


nx_write_state is expecting state_value to be to one of the types defined in _ctypedefs.

So

_funcs.nx_write_state(self._handle, constants.WriteState.LIN_SCHEDULE_CHANGE, _ctypedefs.u32(state_value))

epage · 2017-08-08T15:54:42Z

nixnet/_session/base.py

+
+        Args:
+            state_value(str): Writes the desired state.
+        """


state_value(str): Writes the desired state.

I feel like the parameter name and documentation could be improved.

From the nxWriteState documentation

index to the schedule table that the LIN master executes. The schedule tables are sorted the way they are returned from the database with the XNET Cluster Schedules property.

So my recommendation

sched_index(int): Index to the schedule table that the LIN master executes. The schedule tables are sorted the way they are returned from the database with the XNET Cluster Schedules property.

epage · 2017-08-08T15:55:44Z

nixnet/_session/base.py

+        _funcs.nx_write_state(self._handle, constants.WriteState.LIN_SCHEDULE_CHANGE, state_value)
+
+    def write_state_lin_diagnostic_schedule_change(self, state_value):
+        # type: () -> None


# type: (constants.LinDiagnosticSchedule) -> None

Also, since this will be using a yet-to-be-documented enum, this change should also include documenting the enum.

You can take the documentation from nxWriteState for the values and document the LinDiagnosticSchedule class similarly to how we documented the other enums.

epage · 2017-08-08T15:56:39Z

nixnet/_session/base.py

+        Args:
+            state_value(str): Writes the desired state.
+        """
+        _funcs.nx_write_state(self._handle, constants.WriteState.LIN_DIAGNOSTIC_SCHEDULE_CHANGE, state_value)


Ditto about making it a u32

_ctypedefs.u32(state_value.value)
(.value to convert from enum to int)

epage · 2017-08-08T15:58:34Z

nixnet/_session/base.py

+        the diagnostic schedule.
+
+        Args:
+            state_value(str): Writes the desired state.


Again, I think we can do better about the parameter name. One reason this is important is the parameter names are part of the API definition and changing them breaks compatibility

For example, you can do

session.write_state_lin_diagnostic_schedule_change(state_value=constants.NULL)

From reading the documentation, my recommendation

schedule(:any:`nixnet._enums.LinDiagnosticSchedule`): diagnostic schedule that the LIN master executes

epage · 2017-08-08T15:59:42Z

nixnet/_session/base.py

@@ -335,6 +335,37 @@ def disconnect_terminals(self, source, destination):
        """
        _funcs.nx_disconnect_terminals(self._handle, source, destination)

+    def write_state_lin_schedule_change(self, state_value):


Like the other functions, I don't think we need "write_state" in the name.

So the question is what is a name that makes sense for just this function?

Would change_lin_schedule work?

change_lin_schedule makes sense and still conveys the essence of the function (at least from my understanding of it). @lilesj any thoughts?

change_lin_schedule or something similar sounds good to me.

epage · 2017-08-08T16:00:49Z

nixnet/_session/base.py

+        """
+        _funcs.nx_write_state(self._handle, constants.WriteState.LIN_SCHEDULE_CHANGE, state_value)
+
+    def write_state_lin_diagnostic_schedule_change(self, state_value):


Like the other functions, I don't think we need "write_state" in the name.

So the question is what is a name that makes sense for just this function?

Would change_lin_diagnostic_schedule work? My main concern is that is a similar name to change_lin_schedule but they take completely different types of parameters.

Related to ni#142 Updated repr and added equality tests

Add linframe in types ni#142 Added coverage Add LinFrame equality test

Add linframe in types ni#142 Added more test coverage Added LinFrame equality test

coveralls · 2017-08-11T06:02:24Z

Coverage decreased (-0.06%) to 64.447% when pulling d1f7cf9 on bcornish:linframe into 67976b6 on ni:master.

Updated test_examples

bcornish · 2017-08-11T13:54:28Z

The addition of LIN examples looks like it's going to require a slight restructuring of the examples.rst so that the CAN and LIN examples are clearly differentiated and can have their own headers. Thoughts?

coveralls · 2017-08-11T14:03:03Z

Coverage increased (+0.5%) to 65.033% when pulling cd67ed0 on bcornish:linframe into 67976b6 on ni:master.

epage · 2017-08-11T15:07:34Z

The addition of LIN examples looks like it's going to require a slight restructuring of the examples.rst so that the CAN and LIN examples are clearly differentiated and can have their own headers. Thoughts?

Was this question from before we talked in person and I suggested us having the CAN/LIN examples in the same files?

Yep. You answered my questions for next steps on this.

coveralls · 2017-08-11T16:16:22Z

Coverage increased (+0.5%) to 65.033% when pulling 7d25789 on bcornish:linframe into 67976b6 on ni:master.

coveralls · 2017-08-11T16:33:58Z

Coverage increased (+0.5%) to 65.033% when pulling 3b1fef9 on bcornish:linframe into 67976b6 on ni:master.

coveralls · 2017-08-11T18:04:28Z

Coverage increased (+0.5%) to 65.033% when pulling 215b767 on bcornish:linframe into 67976b6 on ni:master.

Made Can specific

coveralls · 2017-08-11T19:55:11Z

Coverage increased (+0.5%) to 65.033% when pulling c98072d on bcornish:linframe into 67976b6 on ni:master.

coveralls · 2017-08-11T20:34:11Z

Coverage increased (+0.5%) to 65.033% when pulling 8ba8702 on bcornish:linframe into 67976b6 on ni:master.

epage · 2017-08-11T20:43:36Z

tox-integration.ini

@@ -9,12 +9,14 @@ skip_missing_interpreters=True
 [testenv]
 description = Run tests
 commands =
-    test: pytest --junit-xml=junit/{envname}.xml --junit-prefix={envname} {posargs: --nixnet-in-interface {env:NIXNET_FIXTURE_IN_INTERFACE:CAN1} --nixnet-out-interface {env:NIXNET_FIXTURE_OUT_INTERFACE:CAN2}}
+    test: pytest --junit-xml=junit/{envname}.xml --junit-prefix={envname} {posargs: --can-in-interface {env:CAN_FIXTURE_IN_INTERFACE:CAN1} --can-out-interface {env:CAN_FIXTURE_OUT_INTERFACE:CAN2}}


LIN interfaces aren't being passed in on the command line

epage · 2017-08-11T20:45:22Z

nixnet/_session/base.py

@@ -335,6 +335,41 @@ def disconnect_terminals(self, source, destination):
        """
        _funcs.nx_disconnect_terminals(self._handle, source, destination)

+    def change_lin_schedule(self, sched_index):
+        # type: (typing.Text) -> None


# type: (int) -> None

epage · 2017-08-11T20:45:53Z

nixnet/_session/intf.py

@@ -776,6 +776,26 @@ def lin_break_length(self, value):

    @property
    def lin_master(self):
+        '''boolean: LIN Master?


# type: () -> bool

coveralls · 2017-08-11T22:15:03Z

Coverage increased (+0.5%) to 65.033% when pulling e7795f1 on bcornish:linframe into 67976b6 on ni:master.

coveralls · 2017-08-11T22:49:05Z

Coverage increased (+0.5%) to 65.033% when pulling 81f3850 on bcornish:linframe into 67976b6 on ni:master.

bcornish requested a review from epage July 25, 2017 23:13

epage suggested changes Jul 26, 2017

View reviewed changes

epage mentioned this pull request Jul 26, 2017

feat(API): Simplify read/write calls #144

Merged

3 tasks

bcornish force-pushed the linframe branch from b1dfd76 to 2bcbf0f Compare July 28, 2017 08:15

bcornish force-pushed the linframe branch from 2bcbf0f to ef41eb8 Compare July 28, 2017 22:37

epage suggested changes Jul 31, 2017

View reviewed changes

bcornish force-pushed the linframe branch from ef41eb8 to 8a2c948 Compare August 2, 2017 00:13

epage approved these changes Aug 2, 2017

View reviewed changes

epage suggested changes Aug 7, 2017

View reviewed changes

bcornish force-pushed the linframe branch from 8a2c948 to 3fcdedb Compare August 8, 2017 15:23

epage suggested changes Aug 8, 2017

View reviewed changes

bcornish added a commit to bcornish/nixnet-python that referenced this pull request Aug 8, 2017

feat(API): Finish and update LinFrame

cb66142

Related to ni#142 Updated repr and added equality tests

epage mentioned this pull request Aug 8, 2017

feat(API): Implement Linframe type #183

Merged

3 tasks

bcornish added a commit to bcornish/nixnet-python that referenced this pull request Aug 9, 2017

feat(API): Implement LinFrame

391c9a1

Add linframe in types ni#142 Added coverage Add LinFrame equality test

bcornish added a commit to bcornish/nixnet-python that referenced this pull request Aug 9, 2017

feat(API):Implement LinFrame

c9d4e4c

Add linframe in types ni#142 Added more test coverage Added LinFrame equality test

feat(API): Add lin scheduling

d1f7cf9

bcornish force-pushed the linframe branch from 3fcdedb to d1f7cf9 Compare August 11, 2017 05:57

feat(examples): LIN Frame Stream IO

cd67ed0

Updated test_examples

epage changed the title ~~feat(API): Implement reading and writing LIN frames~~ feat(API): Support lin_sched Aug 11, 2017

bcornish force-pushed the linframe branch from 7d25789 to 3b1fef9 Compare August 11, 2017 16:28

bcornish force-pushed the linframe branch from 3b1fef9 to 215b767 Compare August 11, 2017 17:58

fix(tests): Refactor CAN test frames to prep for LIN tests

c98072d

Made Can specific

bcornish force-pushed the linframe branch from 215b767 to c98072d Compare August 11, 2017 19:50

epage suggested changes Aug 11, 2017

View reviewed changes

bcornish force-pushed the linframe branch from 8ba8702 to e7795f1 Compare August 11, 2017 22:10

feat(tests): Add LIN integration tests and configuration

81f3850

bcornish force-pushed the linframe branch from e7795f1 to 81f3850 Compare August 11, 2017 22:44

epage approved these changes Aug 11, 2017

View reviewed changes

epage merged commit e25b721 into ni:master Aug 11, 2017


		_FRAME_ID_MASK = 0x000000FF

		def __init__(self, identifier, type, eventid=b"", payload=b""):

feat(API): Support lin_sched #142

feat(API): Support lin_sched #142

Conversation

bcornish commented Jul 25, 2017 • edited by epage

What does this Pull Request accomplish?

Why should this Pull Request be merged?

What testing has been done?

coveralls commented Jul 25, 2017

coveralls commented Jul 25, 2017 • edited

epage left a 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

Choose a reason for hiding this comment

epage commented Jul 26, 2017

coveralls commented Jul 28, 2017 • edited

epage commented Jul 28, 2017

coveralls commented Jul 28, 2017 • edited

bcornish commented Jul 28, 2017

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

bcornish Jul 31, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 2, 2017 • edited

epage commented Aug 2, 2017

Choose a reason for hiding this comment

bcornish commented Aug 8, 2017

coveralls commented Aug 8, 2017 • edited

coveralls commented Aug 8, 2017

epage left a 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

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 Aug 11, 2017 • edited

bcornish commented Aug 11, 2017

coveralls commented Aug 11, 2017 • edited

epage commented Aug 11, 2017 • edited by bcornish

coveralls commented Aug 11, 2017 • edited

coveralls commented Aug 11, 2017 • edited

coveralls commented Aug 11, 2017 • edited

coveralls commented Aug 11, 2017 • edited

coveralls commented Aug 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 11, 2017 • edited

coveralls commented Aug 11, 2017 • edited

bcornish commented Jul 25, 2017 •

edited by epage

coveralls commented Jul 25, 2017 •

edited

coveralls commented Jul 28, 2017 •

edited

coveralls commented Jul 28, 2017 •

edited

bcornish Jul 31, 2017 •

edited

coveralls commented Aug 2, 2017 •

edited

coveralls commented Aug 8, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

epage commented Aug 11, 2017 •

edited by bcornish

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited

coveralls commented Aug 11, 2017 •

edited