elixir-circuits / circuits_uart

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

Flush framing when closing a port #22

Closed johnc219 closed 7 years ago

johnc219 commented 7 years ago

Aims to fix the following issue:

Repro Steps

On master,

  1. Open a port with framing, e.g.
    Nerves.UART.open(uart1, path, framing: Nerves.UART.Framing.Line)
  2. Close the port
    Nerves.UART.close(uart1)
  3. Attempt to write to the closed port
    Nerves.UART.write(uart, "A")

expected (this branch) {:error, :ebadf}

actual

16:02:14.974 [error] GenServer #PID<0.180.0> terminating
** (UndefinedFunctionError) function nil.separator/0 is undefined (module nil is not available)
    nil.separator()
    (nerves_uart) lib/uart/framing/line.ex:45: Nerves.UART.Framing.Line.add_framing/2
    (nerves_uart) lib/nerves_uart.ex:357: Nerves.UART.handle_call/3
    (stdlib) gen_server.erl:636: :gen_server.try_handle_call/4
    (stdlib) gen_server.erl:665: :gen_server.handle_msg/6
    (stdlib) proc_lib.erl:247: :proc_lib.init_p_do_apply/3
Last message (from #PID<0.179.0>): {:write, "B", 5000}
State: %Nerves.UART.State{controlling_process: #PID<0.179.0>, framing: Nerves.UART.Framing.Line, framing_state: nil, is_active: false, name: nil, port: #Port<0.4991>, queued_messages: [], rx_framing_timeout: 0, rx_framing_tref: nil}

When closing, the framing_state is set to nil, but framing is left intact (Nerves.UART.Framing.Line) The second test I added breaks in master with this error, but passes on this branch.

Proposed solution

The goal is to reset the framing_state when closing, rather than clearing it to nil. When we clear it to nil we lose info about the Framing options, e.g. :separator.

Flushing the framing clears the :processed and :in_process binary while leaving the client-supplied options intact.


I'm not certain flushing is the best long-term approach however as the flush callback now has the added the responsibility of knowing what to do when the port is closed. So perhaps when closing you could do something like

new_framing_state = apply(state.framing, :reset, [state.framing_state])

where :reset is a Framing callback. Here is the diff of a separate branch that fixes this issue with this reset approach instead. I'm guessing that would break anyone who's implemented their own framings though, so it would require a major version bump. Therefore i think the flush approach is the (possibly less correct) safer alternative.

Current workaround

After closing i explicitly configure the uart like such:

Nerves.UART.configure(framing: Nerves.UART.Framing.None)
sourcelevel-bot[bot] commented 7 years ago

Hello, @johnc219! This is your first Pull Request that will be reviewed by Ebert, an automatic Code Review service. It will leave comments on this diff with potential issues and style violations found in the code as you push new commits. You can also see all the issues found on this Pull Request on its review page. Please check our documentation for more information.

fhunleth commented 7 years ago

Thank you so much for your detailed write-up and the PR!

I agree with your analysis and the choice to use the flush approach especially since quite a few people have custom framers. I appreciate you writing up the reset version, though. Part of me likes it better and I'll probably think about it again when/if I make a more major change to the framer interface.

johnc219 commented 7 years ago

Cheers! Thanks for making this project 👍