Add tests for RGBLCD and I2C modules

[axelclark]

• Wrote tests for the GrovePi.RGBLCD and GrovePi.I2C modules.
• Added GrovePi.I2C.get_all_data to assist with the GrovePi.RGBLCD tests.
• Put GrovePi.RGBLCD.initialize/0 in alphabetical order.
• Made a couple of GrovePi.RGBLCD functions public to enable testing. I did not add them to docs since I don't expect them to be called directly by users of the library.
• Added example usage to GrovePi.RGBLCD docs.
• Added a line so the summary for GrovePi.Relay in the docs is only a sentence.

Let me know if you would like me to take a different approach with any of these. I know you are working on a different approach for testing, but I thought it would be a good idea to have a baseline of tests for any future updates.

[adkron]

I think the two modules sit under the single module. There are two components which is why we have two addresses. The thought is that separate modules deal with their separate concerns. We can still have a user facing module to handle interacting with these two as one.

Amos King Binary Noggin

On Nov 7, 2017, at 18:14, Axel Clark notifications@github.com wrote:

@axelclark commented on this pull request.

In lib/grovepi/rgblcd.ex:

@@ -430,15 +442,18 @@ defmodule GrovePi.RGBLCD do send_chars(rest) end

• defp send_lcd_cmd(cmd) do
• @doc false The RGB module would just have set_rgb, set_color_white, and send_rgb. Would you leave initialize in LCD? It will then call into the RGB module. I think the RGB functions might get lost in their own module.

I'm not sure I understand what you mean by the display having two components. The LCD commands control text on the screen and the RGB commands control the color of the screen.

I think it might be easier for users of the library to only need to reference docs in a single module when working with the display.

If you think the two modules will make everything easier to understand and maintain, I can definitely break them apart into two modules. Let me know what you think.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[axelclark]

So is this what you are thinking?

defmodule RGBLCD.LCD do
  @moduledoc false
  @lcd_address 0x3e

  alias GrovePi.Board

  def send_cmd(cmd) do
    Board.i2c_write_device(@lcd_address, <<0x80, cmd>>)
  end

  def send_write(text) do
    Board.i2c_write_device(@lcd_address, <<0x40, text>>)
  end
end

defmodule RGBLCD.RGB do
  @moduledoc false
  @rgb_address 0x62

  alias GrovePi.Board

  def send_cmd(address, value) do
    Board.i2c_write_device(@rgb_address, <<address, value>>)
  end
end

And then the RGBLCD functions will use these modules like this:

  def autoscroll(%{entry_mode: entry_mode} = config) do
    new_entry_mode =
      entry_mode
      |> set_config(@lcd_entry_mode)
      |> set_config(@lcd_display_shift_on)

    LCD.send_cmd(new_entry_mode)

    {:ok, RGBLCD.Config.update_entry_mode(config, new_entry_mode)}
  end

  def set_rgb(red, green, blue) do
    RGB.send_cmd(@reg_red, red)
    RGB.send_cmd(@reg_green, green)
    RGB.send_cmd(@reg_blue, blue)
  end

[adkron]

= WARNING: RAMBLING TO FOLLOW

Your code is what I was meaning, but I think it is pointing out even more. If we look at the autoscroll method, we are setting up some LCD configuration, and there are some LCD entry modes.

I'm looking at the original code and what is going on with the two different "components," and it seems like two components with RGBLCD handling config for each.

Although then when I look at the RGB, we have three devices there. Each one of the colors is a separate device.

The way that the RGBLCD is set up I feel should be mimicked all the way down, but it starts to seems little frivolous when I'm thinking about it.

If we continue down this path, I picture more pieces moving down into the individual modules and the top-level doing mostly delegation. The top-level config might even become a container for two other configs. We would then move each of the commands, like @lcd_entry_mode into their respective modules. LCD, for instance, would contain an @entry_mode and would be used internally to that module. The LCD module would still expose the send_cmd method for anything that we may have failed to support. Then any "advanced" user could utilize the LCD.send_cmd to do things we haven't thought of.

Does this make sense?

I'm okay with merging as is and revisiting this idea in a new pull request.

[axelclark]

I think the config could be moved under LCD since the config is only related to the LCD commands. I'm not sure each mode would justify its own module. The config for each mode is just a binary represented as as a hexadecimal number. However, there is some additional functionality that could be added by an advanced user who could modify the config and then send the new config using the LCD.send_cmd.

This may be just differing terminology, but I'm not sure I understand what you mean by the RGB having three devices. The RGB device address is 0x62 and then there are three registers (red, green, and blue) to write to to change each of those color values.

I'm interested in exploring these concepts further, but maybe we could merge this pull request then open an issue to discuss different approaches to refactoring the module. BTW, I really appreciate the opportunity to be involved in these types of design discussions. Being relatively new to programming, this has been a great learning experience to get feedback and hear other perspectives!

[adkron]

:robot: Thanks