elixir-circuits / circuits_uart

Discover and use UARTs and serial ports in Elixir
Apache License 2.0
188 stars 48 forks source link

Distinguish reading empty binaries from timeout #33

Open pallix opened 6 years ago

pallix commented 6 years ago

Nerves.UART.read returns {:ok, ""} upon timeout. When testing some code I noticed it is possible to use Nervers.UART.write with an empty string ("") and it is possible to read it: {:ok, ""} will also be returned. So it is not possible to distinguish between empty binaries being written and timeout.

I do not need to write empty binaries on the serial but noticed this while using propcheck and tty0tty.

Would it make sense to distinguish timeout and empty binaries? (by returning :timeout for example)

fhunleth commented 6 years ago

At first I thought it was a little weird to write an empty string, but I suppose that if you have a framer that allows it, then it would be an ok thing to do. Given that, I agree that Nerves.UART.read should return {:error, :timeout} so that you can tell the difference.

Also, that's awesome that propcheck caught this case!

adkron commented 6 years ago

@pallix, do you have the propcheck test for this? I would love to see it.

pallix commented 6 years ago

Really there is nothing special to the test. We have a wrapper around Nerves (called Nerves in the code below) in the project and the test looks like this:

  property "can read and write on the serial cable", [:quiet], ctx do
    # exclude empty bitstring from test as we never want to write
    # that on the serial and Nerves use it for timeout.
    forall buffer <- not_empty(binary()) do
      Nerves.write(ctx.test_nerves_out, buffer)
      # tty0tty is slow, wait a litte bit
      :timer.sleep(@rw_timeout - 10)
      {ret, data} = Nerves.read(ctx.test_nerves_in, 1024)
      ret == :ok and data == buffer
    end
  end

Basically the test would fail because ret would equal :to if we allow to write empty buffers.

Setup is done with:

    {:ok, nerves_in} = start_supervised({Nerves, [device: device_in, speed: 9600,
                                                  name: :test_nerves_in]},
      id: :test_nerves_in,  restart: :temporary)
    {:ok, nerves_out} = start_supervised({Nerves, [device: device_out, speed: 9600,
                                                   name: :test_nerves_out]},
    [test_nerves_in: nerves_in, test_nerves_out: nerves_out]

I hope this help.