Update, document and test SerialData #180

jamessynge · 2017-12-09T20:19:40Z

Make the API of rs232.SerialData a bit more regular, and document
it much more thoroughly. Updated the calls accordingly.
Increased test coverage of SerialData.

it much more thoroughly. Updated the calls accordingly. Increased test coverage of SerialData.

codecov · 2017-12-09T20:34:37Z

Codecov Report

Merging #180 into develop will increase coverage by 0.87%.
The diff coverage is 86.36%.

@@             Coverage Diff             @@
##           develop     #180      +/-   ##
===========================================
+ Coverage    80.39%   81.27%   +0.87%     
===========================================
  Files           37       37              
  Lines         2642     2654      +12     
  Branches       330      330              
===========================================
+ Hits          2124     2157      +33     
+ Misses         409      392      -17     
+ Partials       109      105       -4

Impacted Files	Coverage Δ
pocs/utils/rs232.py	`92.85% <86.36%> (+25.41%)`	⬆️

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 25b4cc7...75aedeb. Read the comment docs.

jamessynge · 2017-12-09T20:48:09Z

I'm perplexed about the lack of coverage of pocs/utils/rs232.py. Running coverage.py locally, I see coverage of it, but codecov doesn't show it here. The .coveragerc file does not mark it to be omitted.

wtgee

Approving because I might have limited availability and don't want to block you. Thanks for adding a bunch of missing docstrings.

wtgee · 2017-12-10T07:29:36Z

pocs/mount/serial.py

+            # This won't be triggered because SerialData is created even if the mount port
+            # can't be opened. We can't tell the difference between not currently present
+            # and totally invalid (e.g. com1: on a Linux box, or /dev/tty7 on Windows),
+            # TODO(wtgee): What would you like to do about this?


This is why I was a little skeptical about creating the SerialData object regardless. The error.BadSerialConnection is a good replacement, but it is a big annoying when in use via pocs_shell, because the error right here will trigger before anything else is set up and tell the user to use a simulator.

However, I think that could also be handled in a better manner. I think we could simple remove the try/except here.

I've removed that (and will send another PR).

I'll create an issue to discuss the issue you raise.

wtgee · 2017-12-10T07:38:48Z

pocs/tests/test_rs232.py

@@ -32,9 +50,16 @@ def test_detect_bogus_scheme(handler):
        rs232.SerialData(port='bogus://')


+def test_detect_bogus_scheme(handler):
+    """When our handlers are installed, will still detect unknown scheme."""


What are valid schemes? Have we documented these handlers and schemes as they apply to us?

Rather than document all of those (mostly provided by PySerial), I documented what the scheme must be, and then violated that so that the exception will be raised.

wtgee · 2017-12-10T07:40:10Z

pocs/tests/test_rs232.py

+        # never produces any output.
+        assert '' == ser.read(retry_delay=0.01, retry_limit=2)
+        assert b'' == ser.read_bytes(size=1)
+        assert 0 == ser.write('abcdef')


So does write ever return a valid number or is it always 0?

The no_op handler never returns anything, but other handles do report the size written.

wtgee · 2017-12-10T07:44:38Z

pocs/utils/rs232.py

+    or to force a reset, this wrapper may or may not have an open connection to the underlying
+    serial device. Note that for most devices, is_connected will return true if the device is
+    turned off/unplugged after a connection is opened; the code will only discover there is a
+    problem when we attempt to interact with the device.


If this is the case we should have a method on pocs or observatory that tests the connections and can be checked from the ready state. We can wait to do this when we write the methods that actually do the shutting down (probably in the housekeeping state.)

Submitted #185 to track this issue.

wtgee · 2017-12-10T07:49:13Z

pocs/utils/rs232.py

+            BadSerialConnection if unable to open the connection.
+        """
+        if self.is_connected:
+            return


Does it generate too much noise if we have a log message here saying connection was already open?

Added appropriate message.

wtgee · 2017-12-10T07:52:29Z

pocs/utils/rs232.py

        try:
+            # Get the timestamp after the read so that a long delay on reading doesn't make it
+            # appear that the read happened much earlier than it did.
+            line = self.read()
            ts = time.strftime('%Y-%m-%dT%H:%M:%S %Z', time.gmtime())
            info = (ts, self.read())


Should this be line instead of another self.read()?

Doh! Test added that detected this bug, and bug fixed.

wtgee · 2017-12-10T07:54:00Z

pocs/utils/rs232.py

            ts = time.strftime('%Y-%m-%dT%H:%M:%S %Z', time.gmtime())
            info = (ts, self.read())
        except IndexError:
+            # TODO(wtgee): When might this occur?


Good question but I think it was when dealing the threaded and reading from a queue. Not entirely sure though so perhaps we should remove until becomes a problem. Or at least add a log message such that we can easily find when it does happen.

Removed try/except.

wtgee

LGTM. Approving before laptop battery dies.

wtgee · 2017-12-10T22:01:24Z

pocs/tests/test_rs232.py



 def test_basic_io(handler):
-    protocol_buffers.ResetBuffers(b'abc\r\n')
-    ser = rs232.SerialData(port='buffers://', open_delay=0, threaded=False)
+    protocol_buffers.ResetBuffers(b'abc\r\ndef\n')


So does this consume either \r\n or \n?

Make the API of rs232.SerialData a bit more regular, and document

25c4c1d

it much more thoroughly. Updated the calls accordingly. Increased test coverage of SerialData.

Add missing docstring.

3587039

jamessynge requested a review from wtgee December 9, 2017 22:54

wtgee previously approved these changes Dec 10, 2017

View reviewed changes

jamessynge added 2 commits December 10, 2017 14:53

Address review feedback.

e984024

Merge branch 'develop' into rs232

3129a01

jamessynge dismissed wtgee’s stale review via 3129a01 December 10, 2017 19:59

Remove try/except around creation of SerialData in AbstractSerialMount.

75aedeb

wtgee approved these changes Dec 10, 2017

View reviewed changes

jamessynge merged commit f1124a6 into panoptes:develop Dec 11, 2017

jamessynge deleted the rs232 branch December 11, 2017 15:21

wtgee mentioned this pull request Dec 29, 2017

Release 0.6.0 #271

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update, document and test SerialData #180

Update, document and test SerialData #180

jamessynge commented Dec 9, 2017

codecov bot commented Dec 9, 2017 •

edited

jamessynge commented Dec 9, 2017

wtgee left a comment

wtgee Dec 10, 2017

jamessynge Dec 10, 2017

wtgee Dec 10, 2017

jamessynge Dec 10, 2017

wtgee Dec 10, 2017

jamessynge Dec 10, 2017

wtgee Dec 10, 2017

jamessynge Dec 10, 2017

wtgee Dec 10, 2017

jamessynge Dec 10, 2017

wtgee Dec 10, 2017

jamessynge Dec 10, 2017

wtgee Dec 10, 2017

jamessynge Dec 10, 2017

wtgee left a comment

wtgee Dec 10, 2017

Update, document and test SerialData #180

Update, document and test SerialData #180

Conversation

jamessynge commented Dec 9, 2017

codecov bot commented Dec 9, 2017 • edited

Codecov Report

jamessynge commented Dec 9, 2017

wtgee 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wtgee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 9, 2017 •

edited