-
Notifications
You must be signed in to change notification settings - Fork 49
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
Add testing of rs232.SerialData #164
Conversation
feature that allows a handler to be used based on the url of the serial "device".
I think that the module is being loaded twice, once by test_rs232.py, and another time by serial.serial_for_url(). As a result, the buffers set by the test aren't visible to the BuffersSerial instance.
…rial. This shows how we can develop mock serial devices for testing.
pocs/utils/rs232.py
Outdated
# Properties have been set to reasonable values, ready to open the port. | ||
self.ser.open() | ||
|
||
# TODO(jamessynge): Consider eliminating this sleep period, or making |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still need to dig back through my notes about why the delay is there in the first place but would prefer to capture these TODOs in an Issue unless it will be resolved as part of this PR>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. Sorry, I'm in the habit of capturing these things in the code where their context is readily apparent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issue added, TODO removed.
pocs/utils/rs232.py
Outdated
""" | ||
assert not self._start_needed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where this comes from or get what it is doing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
pocs/utils/rs232.py
Outdated
# (i.e. toss out any buffered input, and then read the next full line... | ||
# which likely requires tossing out a fragment of a line). | ||
count = self.ser.in_waiting | ||
self.ser.reset_input_buffer() | ||
# self.logger.debug('Cleared {} bytes from buffer'.format(count)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC this was generating too many log entries and so was commented out. Could consider eliminating along with count
variable above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do so now if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, let's get rid of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
pocs/utils/rs232.py
Outdated
|
||
# Not worrying here about bytes arriving between reading in_waiting | ||
# and calling reset_input_buffer(). | ||
# Note that Wilfred reports that the Arduino input can seriously lag behind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lag of 10 seconds would probably apply more to the output buffer. Once it is lagging the only way to catch-up would be to clear output buffer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
output meaning writing from POCS to the Arduino? That sounds like a flush call needs to be made on the device to ensure it is making it out of buffers and into the OS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output meaning from the Arduino to POCS. Unless we connect to and start reading from the arduino immediately after it is reset the sensor readings lag behind.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to be specific when we use the words input and output (I wasn't sufficiently in the comment). So, "input from Arduino" rather than "Arduino input", which was ambiguous; or "POCS output to the mount".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the comment accordingly.
Codecov Report
@@ Coverage Diff @@
## develop #164 +/- ##
=======================================
Coverage 80.6% 80.6%
=======================================
Files 36 36
Lines 2527 2527
Branches 319 319
=======================================
Hits 2037 2037
Misses 386 386
Partials 104 104 Continue to review full report at Codecov.
|
With a big addition to this I would like to get into the habit of adding a quick user-friendly entry to the Changelog.md file. This should be added to the |
Added Changelog.md. Will test and push. |
PTAL |
Changelog.md
Outdated
@@ -1,5 +1,12 @@ | |||
## [Unreleased] | |||
|
|||
- PR#164 (no sha yet) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we should first clarify what the purpose of the changelog is. I'm mostly following http://keepachangelog.com/en/0.3.0/
I don't think we care about the PR or the SHA. Things like "...doesn't hide exceptions..." also seem like too much detail for how I think about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
This is a re-do of PR #147 after merging in the big changes from develop.