Rewrite

[pengowen123]

This rewrites most of the code and brings the bridge to a more functional state. The main additions are:

• Ryder device output is read asynchronously as it is received instead of assuming that it follows a command. Fixes Protocol issue with WAIT_FOR_USER_CONFIRM #12.
• Client and Ryder device disconnects are handled gracefully, and the bridge closes all connections properly when it shuts down itself. Fixes Bridge goes into error state if the Ryder is disconnected #8.
• Various responses are sent to the client to notify it of errors and disconnects. I picked some unused values for these responses for testing, but their names/values should definitely be changed and formalized at some point.
• The client has some improvements to make it easier to test the bridge.

Remaining features to implement:

• Additional client connections past the first should be queued up instead of refused, and they should be sent RESPONSE_WAIT_IN_QUEUE and later RESPONSE_BEING_SERVED when the device becomes available. This will fix Bridge cannot handle multiple incoming WebSocket connections #10.
• The async tokio_serial interface should be used more fully. Currently, the sync interface is being used without issue, but the code could probably be made cleaner with more futures rather than threads. tokio_serial has issues with re-opening the serial port, and its async interface cannot detect device disconnections (On FreeBSD tokio-serial seems not to notice, that port is (physically) removed during wait for inbound data berkowski/tokio-serial#56).
• The code should be refactored into smaller modules to make it cleaner, better organized, and easier to test.
• Tests of some sort should be added. The code is currently not organized in a way that makes testing easy, so this will probably wait for the refactor. This will fix Missing tests #11.

[MarvinJanssen]

Unfortunately I cannot run the branch as-is.

% cargo build
   Compiling ryder-bridge-rust v0.1.0 (.../ryder-bridge-rust)
error[E0599]: no method named `is_finished` found for struct `tokio::task::JoinHandle` in the current scope
   --> src/main.rs:272:37
    |
272 |                     if !conn.handle.is_finished() {
    |                                     ^^^^^^^^^^^ method not found in `tokio::task::JoinHandle<()>`

For more information about this error, try `rustc --explain E0599`.
error: could not compile `ryder-bridge-rust` due to previous error
warning: build failed, waiting for other jobs to finish...

[pengowen123]

Strange, that function doesn't seem to be platform-specific, so I'm not sure why it doesn't exist for you. Either way, that part of the code was kind of a mess and has been redone entirely. Could you try compiling it again?

[MarvinJanssen]

Builds fine now!

The next issue seems to be the "Not a typewriter" bug. I ran into it before and it has something to do with a particular serial function when used on macOS. I do not know if this is the same issue, but in that case the fix was to set the baud rate to 0 which caused the library to do an auto detect somewhere. Not sure.

In any case, the bug occurs only when using the simulator and not the physical device. Arguably it could also be a problem in the simulator firmware and it the virtual serial port needs a flag set by the simulator.

cargo run /dev/ttys002 localhost:8888
    Finished dev [unoptimized + debuginfo] target(s) in 0.04s
     Running `target/debug/ryder-bridge-rust /dev/ttys002 'localhost:8888'`
Listening on: localhost:8888
Ryder port: /dev/ttys002
Incoming TCP connection from: [::1]:52025
WebSocket connection established: [::1]:52025
Error opening serial port: Not a typewriter, Unknown

[pengowen123]

The bridge uses tokio_serial now, which depends on serialport under the hood still, which was previously being overridden to include this patch. I'll just reimplement the override until the patch is released on crates.io.

[pengowen123]

Alright, it should work now.

[pengowen123]

This is good to go pending reviews @MarvinJanssen @friedger.

[MarvinJanssen]

Works great on my m1 Mac! However, tests seem to fail. Looks like it's trying to use an address/port that is in use. Any idea how we could make that mock more resilient on different systems? At some point we'll be running cargo test on GitHub Actions.

% cargo test                                                                                                  
    Finished test [unoptimized + debuginfo] target(s) in 0.04s
     Running unittests src/lib.rs (target/debug/deps/ryder_bridge-4e5063ffb401e266)
             
running 15 tests                                                                                     
test queue::tests::test_queue_serve_next_abandoned ... ok
test queue::tests::test_queue_find ... ok
test queue::tests::test_connection_try_serve ... ok
test queue::tests::test_queue_add_connection ... ok
test queue::tests::test_queue_remove_and_serve_next ... ok
test queue::tests::test_queue_remove ... ok
test queue::tests::test_queue_serve_next_without_removing - should panic ... ok
test serial::port::tests::test_is_accessible ... ok
test serial::port::tests::test_io ... ok
test serial::server::tests::test_write ... ok                                                    
test serial::port::tests::test_try_open ... ok
test serial::port::tests::test_new ... ok       
test serial::port::tests::test_update_port_state ... ok                                                                                          
test tests::test_bridge_handle_terminate ... ok
test queue::tests::test_queue_serve_next ... ok

test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.17s

     Running unittests src/bin/client.rs (target/debug/deps/client-fe4cfafc149c835a)

running 1 test
test tests::test_parse_input ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running unittests src/main.rs (target/debug/deps/ryder_bridge-c81fde3fa46fc86e)

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

     Running tests/tests.rs (target/debug/deps/tests-e06e6d98ed4bcd2a)

running 5 tests
test test_echo ... FAILED
test test_bridge_shutdown ... FAILED
test test_device_not_connected ... FAILED
test test_device_disconnected ... FAILED
test test_queue ... ok

failures:

---- test_echo stdout ----
Listening on: 127.0.0.1:8080
Ryder port: ./nonexistent
thread 'test_echo' panicked at 'Failed to bind: Os { code: 48, kind: AddrInUse, message: "Address already in use" }', src/lib.rs:94:31
thread 'test_echo' panicked at 'assertion failed: next_response_timeout(&mut client).await.is_err()', tests/tests.rs:82:9
thread 'test_echo' panicked at 'called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(3), ...)', tests/tests.rs:62:30

---- test_bridge_shutdown stdout ----
Listening on: 127.0.0.1:8080
Ryder port: ./nonexistent
thread 'test_bridge_shutdown' panicked at 'Failed to bind: Os { code: 48, kind: AddrInUse, message: "Address already in use" }', src/lib.rs:94:31
thread 'test_bridge_shutdown' panicked at 'assertion failed: `(left == right)`
  left: `Text("RESPONSE_WAIT_IN_QUEUE")`,
 right: `Text("RESPONSE_BRIDGE_SHUTDOWN")`', tests/tests.rs:199:9
thread 'test_bridge_shutdown' panicked at 'called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(2), ...)', tests/tests.rs:62:30

---- test_device_not_connected stdout ----
Listening on: 127.0.0.1:8080
Ryder port: ./nonexistent
thread 'test_device_not_connected' panicked at 'Failed to bind: Os { code: 48, kind: AddrInUse, message: "Address already in use" }', src/lib.rs:94:31
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'test_device_not_connected' panicked at 'assertion failed: `(left == right)`
  left: `Text("RESPONSE_WAIT_IN_QUEUE")`,
 right: `Text("RESPONSE_DEVICE_NOT_CONNECTED")`', tests/tests.rs:110:9
thread 'test_device_not_connected' panicked at 'called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(4), ...)', tests/tests.rs:62:30

---- test_device_disconnected stdout ----
Listening on: 127.0.0.1:8080
Ryder port: ./nonexistent
thread 'test_device_disconnected' panicked at 'Failed to bind: Os { code: 48, kind: AddrInUse, message: "Address already in use" }', src/lib.rs:94:31
thread 'test_device_disconnected' panicked at 'assertion failed: `(left == right)`
  left: `Text("RESPONSE_WAIT_IN_QUEUE")`,
 right: `Text("RESPONSE_DEVICE_DISCONNECTED")`', tests/tests.rs:141:9
thread 'test_device_disconnected' panicked at 'called `Result::unwrap()` on an `Err` value: JoinError::Panic(Id(1), ...)', tests/tests.rs:62:30


failures:
    test_bridge_shutdown
    test_device_disconnected
    test_device_not_connected
    test_echo

test result: FAILED. 1 passed; 4 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.61s

error: test failed, to rerun pass '--test tests'

[pengowen123]

The test runner uses multiple threads by default, which does not work when all the tests use the same port. The tests must be run with cargo test --all -- --test-threads 1 to work around this, or you can just run make test if you have Make installed.

[MarvinJanssen]

The more you know! Works great, good to merge for me. @friedger?

[MarvinJanssen]

I'm merging this because it has been open for a while @friedger.