adkron / grovepi

Use the GrovePi in Elixir
Apache License 2.0
47 stars 9 forks source link

Replace Elixir/ALE with Elixir Circuits #44

Closed axelclark closed 5 years ago

axelclark commented 5 years ago

Update Home Weather Display example project

Replace ElixirAle with Elixir Circuits

I converted GrovePi.Board to a GenServer to hold the I2C ref. See porting instructions.

I haven't figured out the best way to update the GrovePi.I2C and tests. One approach I've considered is to add all of the test-specific GrovePi.I2C functions (e.g. get_last_write/1) to the GrovePi.Board then send and request the data from the GrovePi.I2C GenServer started when GrovePi.Board calls @i2c.open/1 in handle_continue/2.

axelclark commented 5 years ago

I added the test helper functions to GrovePi.Board then sent request to I2C. Had to go through GrovePi.Board because it holds the pid/ref for I2C in its state.

Remaining failing test is because retries now happen within Circuits rather than GrovePi. When the poller receives an error from the I2C board, should it fail (current implementation) or somehow handle the error?

https://github.com/adkron/grovepi/blob/f76df053cdfcbad7cff887414a751178c8c5a3d4/lib/grovepi/poller.ex#L168-L173

Current test has 1 error which is discarded during the 1st retry in GrovePi.Board.

https://github.com/adkron/grovepi/blob/f76df053cdfcbad7cff887414a751178c8c5a3d4/test/grovepi/ir_reflective_test.exs#L20-L30

https://github.com/adkron/grovepi/blob/f76df053cdfcbad7cff887414a751178c8c5a3d4/lib/grovepi/board.ex#L78-L85

Here is the new implementation:

  def handle_call({:read, bytes_to_read}, _from, state) do
    reply =
      case(@i2c.read(state.i2c_bus, state.address, bytes_to_read, retries: @i2c_retry_count)) do
        {:ok, response} -> response
        {:error, error} -> {:error, error}
      end

    {:reply, reply, state}
  end
axelclark commented 5 years ago

This was my best attempt at updating, but I welcome any feedback for a better implementation.

adkron commented 5 years ago

I’ve been offline the last few days. This is on my to do for tomorrow and the next day. Thanks.

Amos King CEO Binary Noggin

On May 21, 2019, at 20:21, Frank Hunleth notifications@github.com wrote:

@fhunleth commented on this pull request.

In lib/grovepi/board.ex:

  • def handle_call({:write, message}, _from, state) do
  • reply = @i2c.write(state.i2c_bus, state.address, message, retries: @i2c_retry_count)
  • {:reply, reply, state}
  • end
  • @impl true
  • def handle_call({:write_device, address, message}, _from, state) do
  • reply = @i2c.write(state.i2c_bus, address, message, retries: @i2c_retry_count)
  • {:reply, reply, state}
  • end
  • @impl true
  • def handle_call({:read, bytes_to_read}, _from, state) do
  • reply =
  • case(@i2c.read(state.i2c_bus, state.address, bytes_to_read, retries: @i2c_retry_count)) do
  • {:ok, response} -> response Looks like this was the old behavior, so I take back my comment for this PR.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.

adkron commented 5 years ago

I think if there is an error from the i2c we can let the poller fail and restart. If we can come back from the failure, I believe that is best because we might not know the state of why, and a restart is probably a good idea.

I think we can always change our minds on that and go from failing to error handling later, but it is hard to go the other way once people start to depend on the implementation.

axelclark commented 5 years ago

I made the updates based on your comments.

One additional note. The compiler sends warning messages for the GrovePi.Board test helper callbacks when compiling for prod because those @i2c functions don't exist in Circuits.I2C. I'm not sure how to remove those warnings, change the implementation to eliminate them, or if those should be ignored. For example:

  def handle_call({:get_last_write}, _from, state) do
    {:reply, @i2c.get_last_write(state.i2c_bus), state}
  end

Let me know if you see anything else. Thanks for the feedback and the opportunity to contribute to the project!

adkron commented 5 years ago

I might need to rework the testing and how we are doing that part of the project. I think we can pull this in with the warnings and work on those later. Moving to Elixir Circuits is important work. Thank you very much. 🤖