Updated Protocol + Scan worker #70

dcyoung · 2017-04-14T23:00:16Z

This PR brings a few changes:

Updated protocol to match the new firmware that is installed on devices shipping out today.
Updated to perform checks for status byte codes indicating failures that do not result in errors.
Provide methods (and update existing methods) to wait until the device is ready before attempting to change motor speed or start scanning.
Add scan worker to accumulate scans in a queue using a background thread.
Return complete scans between 0-360 degrees (other than first scan which is inherently partial).
Offer alternative non-blocking functions where possible (currently only available in C library).
Increase delay between stop commands in the stop_scanning function to account for the device logging session data after responding to the first stop.

I have tested the examples, and the bindings on windows. I have tested the examples and bindings on ubuntu 14.04, but @kent-williams mentioned he might have experienced an issue with the python bindings on 16.04. The travis builds might not indicate anything as they're using the dummy build. So i'd love to get additional testing feedback if you have the time @daniel-j-h

This PR needs get merged ASAP, as people will be receiving devices very soon (early/mid next week). I'll go ahead and merge this in a few days if there aren't any complaints. But please add your feedback if you have it!

daniel-j-h · 2017-04-16T09:46:30Z

Thanks for the heads up, I'll be on a plane the next day(s), can jump back into this not earlier than maybe Tuesday or Wednesday. What's the Ubuntu 16.04 issue with Python? Check the open PRs there should be one with Docker (sorry on a phone). You could use a local 16.04 container and try to reproduce.

…

On April 14, 2017 4:00:16 PM PDT, dcyoung ***@***.***> wrote: This PR brings a few changes: 1. Updated protocol to match the new firmware that is installed on devices shipping out today. 2. Updated to perform checks for status byte codes indicating failures that do not result in errors. 3. Provide methods (and update existing methods) to wait until the device is ready before attempting to change motor speed or start scanning. 4. Add scan worker to accumulate scans in a queue using a background thread. 5. Return complete scans between 0-360 degrees. 6. Offer alternative non-blocking functions where possible (currently only available in C library). I have tested the examples, and the bindings on windows. I have tested the examples and bindings on ubuntu 14.04, but @kent-williams mentioned he might have experienced an issue with the python bindings on 16.04. The travis builds might not indicate anything as they're using the dummy build. So i'd love to get additional testing feedback if you have the time @daniel-j-h This PR needs get merged ASAP, as people will be receiving devices very soon (early/mid next week). I'll go ahead and merge this in a few days if there aren't any complaints. But please add your feedback if you have it! You can view, comment on, or merge this pull request online at: #70 -- Commit Summary -- * Protocol and SDK updated to check for Motor Stabilization * Added blocking and non blocking verisons of functions that fail when motor is not ready * Accumulate scans in a detached background thread * Background scan worker thread accumulates entire ordered scan * Updated doc to reflect change in Motor ready command symbols -- File Changes -- M libsweep/README.md (63) M libsweep/doc/serial_protocol_spec.md (124) M libsweep/examples/example.c (7) M libsweep/examples/example.cc (8) M libsweep/examples/net.cc (1) M libsweep/examples/viewer.cc (1) M libsweep/include/protocol.h (22) M libsweep/include/sweep/sweep.h (34) M libsweep/include/sweep/sweep.hpp (27) M libsweep/src/dummy.cc (151) M libsweep/src/protocol.cc (37) M libsweep/src/sweep.cc (455) M sweepjs/README.md (8) M sweepjs/index.js (3) M sweepjs/sweepjs.cc (24) M sweepjs/sweepjs.h (2) M sweeppy/README.md (6) M sweeppy/sweeppy/__init__.py (26) M sweeppy/sweeppy/__main__.py (4) -- Patch Links -- https://github.com/scanse/sweep-sdk/pull/70.patch https://github.com/scanse/sweep-sdk/pull/70.diff

daniel-j-h · 2017-04-18T17:24:10Z

libsweep/README.md

+Signals the `sweep_device_s` to start scanning. 
+Will only succeed if the motor speed is \>0Hz and stable.
+User is responsible for checking these conditions before calling. User can check that motor speed has stabilized using `sweep_device_get_motor_ready` and that motor speed is \> 0Hz using `sweep_device_get_speed`.
+Will NOT start an internal background thread. User is responsible for keeping up with incoming scans by calling `sweep_device_get_scan_direct`.


Hmm why do we mix waiting for motor to stabilize and worker thread here? (And in the PR in general)
Shouldn't the two be orthogonal concepts?

What I'm a bit worried about is the API is starting to get much more confusing than just start scanning - get scans.

The idea behind the non-blocking versions of certain methods, was to provide options for low-level development, where blocking or threading wasn't available.

I've gone ahead and removed the alternative methods from the API for now. We can merge this PR, and worry about the naming conventions etc later. The only changes to the public API will be the addition of two new methods (get_motor_ready and wait_until_motor_ready). As @MikeGitb suggested, we can work on the implementation details and naming conventions for the alternative bits AFTER this PR... and merge the updated API later when we're comfortable with it.

daniel-j-h · 2017-04-18T17:24:44Z

libsweep/README.md

 void sweep_device_stop_scanning(sweep_device_s device, sweep_error_s* error)
 ```

 Signals the `sweep_device_s` to stop scanning.
+Blocks for ~20ms to allow time for trailing data stream to collect and flush internally.


Does this depend on motor speed? 20ms is the upper bound?

daniel-j-h · 2017-04-18T17:25:54Z

libsweep/README.md

+```
+
+Blocks until the `sweep_device_s`'s motor speed has stabilized. This is useful whenever the motor speed is changed. The worst case is a 6 second wait for motor stabilization and calibration. If the motor is already spinning at the desired speed, this returns almost instantly. For visual reference, the blue LED on the device will blink while the motor speed is changing, and stop blinking when the speed has stabilized. If the motor speed has NOT yet stabilized, the device will respond to certain commands (`DS` or `MS`) with a status code indicating a failure to execute the command. Therefore, it is best practice to avoid this entirely by calling `sweep_device_wait_until_motor_ready` before calling a command that requires motor speed is stabilized.
+In case of error a `sweep_error_s` will be written into `error`. Will timeout.


What does "will timeout" mean here? That there is an internal timeout of 6s and it will error out if this limit is exceeded?

daniel-j-h · 2017-04-18T17:27:22Z

libsweep/README.md

+```
+
+Returns 0 if `sweep_device_s`'s motor speed has stabilized. 
+Returns 1 if not. 


We require C99+ in the API, so we have stdbool.h - please use bool here.

https://github.com/scanse/sweep-sdk/blob/master/libsweep/include/sweep/sweep.h#L42

bool sweep_device_is_motor_ready(..)

daniel-j-h · 2017-04-18T17:29:22Z

libsweep/README.md

@@ -245,7 +289,16 @@ Opaque type representing a single full 360 degree scan from a `sweep_device_s`.
 sweep_scan_s sweep_device_get_scan(sweep_device_s device, sweep_error_s* error)
 ```

-Blocks waiting for the `sweep_device_s` to accumulate a full 360 degree scan into `sweep_scan_s`.
+Returns the ordered readings (1st to last) from a single scan.
+Retrieves the oldest scan from a queue of scans accumulated in a background thread. Blocks until a scan is available. To be used after calling `sweep_device_start_scanning`, NOT after `sweep_device_attempt_start_scanning`.


This and below is what I meant - there now is a direct dependency between functions. And the naming

get_scan vs get_scan_direct start_scan vs attempt_start_scan

does not really make it clear when to use which version.

Do we give any guarantees wrt. the queue? How large is it? Should the user be able to specify?

For non low-level development, the API should be unchanged. The "attempt_..." methods are provided as niche case alternatives... and not meant to be advertised. I went ahead and separated these methods out of the main README so that the distinction is clear. See if that makes more sense.

If you have ideas for a better naming convention, please share! I'm not very content with the current "attempt_" and "direct_" pattern.

daniel-j-h · 2017-04-18T17:41:07Z

libsweep/src/sweep.cc

+  // create a thread
+  std::thread th = std::thread(sweep_device_accumulate_scans, device);
+  // detach the thread so that it runs in the background and cleans itself up
+  th.detach();


Hmm, shouldn't we block (i.e. .join()) in the device's destructor? That is, set the stop scanning atomic flag, then wait until done.

As I understand it, you cannot join a detached thread. The detached background thread cleans itself up properly when it's method returns or terminates. Seeing how the behavior of the thread exists in the "background" with no need to synchronize, it seems like a much simpler solution to avoid explicitly managing the thread. I could be wrong, but I believe this is the exact kind of intended use for detached threads.

daniel-j-h · 2017-04-18T17:41:32Z

libsweep/src/sweep.cc

+  sweep_error_s readyerror = nullptr;
+  int32_t motor_ready;
+  // Only check for 8 seconds (16 iterations with 500ms pause)
+  for (auto i = 0; i < 16; ++i) {


Why 16? Why 8 seconds?

daniel-j-h · 2017-04-18T17:41:37Z

libsweep/src/sweep.cc

+      return;
+    }
+    // pause for 500ms
+    std::this_thread::sleep_for(std::chrono::milliseconds(500));


daniel-j-h · 2017-04-18T17:42:38Z

sweeppy/sweeppy/__main__.py

        sweep.start_scanning()

-        # get_scans is coroutine-based generator lazily returning scans ad infinitum
+        # get_scans is coroutine-based generator lazily returning scans ad
+        # infinitum


Why break here?

daniel-j-h · 2017-04-18T17:43:29Z

libsweep/src/sweep.cc

+  const uint8_t status_bytes[2] = {response.cmdStatusByte1, response.cmdStatusByte2};
+  int32_t status_code = sweep::protocol::ascii_bytes_to_integral(status_bytes);
+  switch (status_code) {
+  case 11:


What is 11? Can we symbolize this

11 is a failure status code that indicates an invalid parameter. These details are outlined in the protocol docs, but I think it might be best to include them in the protocol implementation. Perhaps making a container of possible status codes for each type of response?

MikeGitb · 2017-04-19T07:58:24Z

If this is to be released soon, may I suggest to not put the low level functions into the public API just yet?
As changing the API afterwards is usually undesirable I think it would better to have one or two iterations on it before settling for a design, but as there are probably only very few users of it it should imho not block the merge of this PR.

Talking about blocking the merge: For substantial changes like this I often take a triaging approach: Only hold of the merge, until the API is OK and the code appears to be bugfree (that is the tricky part) and then address implementation issues in separate (smaller) PRs.

dcyoung · 2017-04-19T17:35:55Z

@MikeGitb I like the idea of waiting to add these "alternative" methods to the API until they are more fleshed out and the naming convention is settled. The rest of the API should remain unchanged. The implementation differs, and we've added new methods relevant to "motor ready", but the core start/stop methods etc are the same. I'll remove the alternative methods from the API in the next few hours and update the PR.

Edit: Updated!

dcyoung

Minor fixes

dcyoung · 2017-04-19T17:40:22Z

libsweep/src/dummy.cc

@@ -34,6 +34,65 @@ void sweep_error_destruct(sweep_error_s error) {
  delete error;
 }

+// ------------------ Blocking Methods (blocking) ---------------- //


reorganize the methods to match sweep.h/sweep.cc and remove the outdated comments

dcyoung · 2017-04-19T17:41:20Z

sweepjs/README.md

@@ -42,10 +42,18 @@ sweep = new Sweep('/dev/ttyUSB0');
 sweep.startScanning();
 sweep.stopScanning();

+// waits until motor is ready
+sweep.waitUntilMotorReady();
+// code === 0 -> ready (motor ready), code !== 0 -> not ready


update sweepjs bindings reflect boolean return, instead of a motor ready code

dcyoung · 2017-04-19T17:42:08Z

sweeppy/README.md

@@ -62,10 +62,12 @@ class Sweep:
    def start_scanning(self) -> None
    def stop_scanning(self) -> None

-    def get_motor_speed(self) -> int
+    def wait_until_motor_ready(self) -> None
+    def get_motor_ready(self) -> int (0 indicates motor is ready)


indicate that a bool is returned

…d thread

…p firmware v1.1

dcyoung · 2017-04-21T17:24:15Z

Just pushed an update to the MAJOR version, and added a table for firmware compatibility in the README. If the appveyor + travis builds look good, I'm going to go ahead and merge this.

dcyoung · 2017-04-22T00:30:42Z

Unfortunately, I realized after merging this that the "wait_until_motor_ready" method was not working properly when being called externally. It works fine internally. This might have something to do with the error handling.

Luckily, this method is not actually necessary, given that "start_scanning" and "set_motor_speed" call it internally. Therefore, I stripped the method from the public API and quickly pushed the fix to master. I avoided re-basing master in case anyone already grabbed it. But my apologies if anyone has been working with it during the few hours before the fix.

This was referenced Apr 14, 2017

Worker Thread Discussion #31

Closed

Communicating Failure in Updated Protocol #68

Closed

daniel-j-h requested changes Apr 18, 2017

View reviewed changes

dcyoung force-pushed the scan-worker branch from 9cd2de6 to a80a5b1 Compare April 18, 2017 21:13

dcyoung commented Apr 19, 2017

View reviewed changes

Updated Protocol for firmware v1.1, and accumulate scans in backgroun…

fab0612

…d thread

dcyoung force-pushed the scan-worker branch from dca2eff to fab0612 Compare April 21, 2017 17:05

Incremented libsweep MAJOR version to reflect compatibility with swee…

2ba6d8e

…p firmware v1.1

dcyoung merged commit b867c37 into master Apr 21, 2017

dcyoung deleted the scan-worker branch April 21, 2017 18:13

daniel-j-h mentioned this pull request Apr 30, 2017

Check Firmware Compatibility #77

Open

dcyoung mentioned this pull request May 8, 2017

Refactores Error Translation, resolves #9 #80

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated Protocol + Scan worker #70

Updated Protocol + Scan worker #70

dcyoung commented Apr 14, 2017 •

edited

Loading

daniel-j-h commented Apr 16, 2017 via email

daniel-j-h Apr 18, 2017

dcyoung Apr 18, 2017

dcyoung Apr 19, 2017 •

edited

Loading

daniel-j-h Apr 18, 2017

daniel-j-h Apr 18, 2017

daniel-j-h Apr 18, 2017

daniel-j-h Apr 18, 2017

daniel-j-h Apr 18, 2017

dcyoung Apr 18, 2017

daniel-j-h Apr 18, 2017

dcyoung Apr 18, 2017 •

edited

Loading

daniel-j-h Apr 18, 2017

daniel-j-h Apr 18, 2017

daniel-j-h Apr 18, 2017

daniel-j-h Apr 18, 2017

dcyoung Apr 18, 2017

MikeGitb commented Apr 19, 2017

dcyoung commented Apr 19, 2017 •

edited

Loading

dcyoung left a comment

dcyoung Apr 19, 2017

dcyoung Apr 19, 2017

dcyoung Apr 19, 2017

dcyoung commented Apr 21, 2017

dcyoung commented Apr 22, 2017

Updated Protocol + Scan worker #70

Updated Protocol + Scan worker #70

Conversation

dcyoung commented Apr 14, 2017 • edited Loading

daniel-j-h commented Apr 16, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcyoung Apr 19, 2017 • 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

dcyoung Apr 18, 2017 • 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

MikeGitb commented Apr 19, 2017

dcyoung commented Apr 19, 2017 • edited Loading

dcyoung 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

dcyoung commented Apr 21, 2017

dcyoung commented Apr 22, 2017

dcyoung commented Apr 14, 2017 •

edited

Loading

dcyoung Apr 19, 2017 •

edited

Loading

dcyoung Apr 18, 2017 •

edited

Loading

dcyoung commented Apr 19, 2017 •

edited

Loading