Conversation
Fixes a off by one error
Just put a comment instead of an actual call in case we want to restore the calls later.
Jax/inky impression
| {:ok, reset_pid} = gpio.open(pin_mappings[:reset_pin], :output, initial_value: 1) | ||
| IO.puts("opening busy pin") | ||
| {:ok, busy_pid} = gpio.open(pin_mappings[:busy_pin], :input) | ||
| IO.puts("opening SPI device") |
There was a problem hiding this comment.
Do we want to keep the prints in here?
There was a problem hiding this comment.
I left them in because it seemed useful and credo wasn't opposed to it either.
lib/inky.ex
Outdated
|
|
||
| IO.puts("Seting pixels...") | ||
| Inky.set_pixels(Inky.Foo, %{}) | ||
| end |
There was a problem hiding this comment.
Looks like a dev function snuck its way in.
There was a problem hiding this comment.
Yep, removing this block during my cleanup.
lib/hal/hal.ex
Outdated
| state :: Inky.IOCommands.State.t() | ||
| ) :: | ||
| :ok | {:error, :device_busy} | ||
| io_state() | :ok | {:error, :device_busy} |
There was a problem hiding this comment.
Not cool, hehe... why leak any() from here?
There was a problem hiding this comment.
Good call, reverting in cleanup.
lib/hal/impression/rpihal.ex
Outdated
| :yellow => 5, | ||
| :orange => 6, | ||
| # Not a color, but is used to clear the display | ||
| :clean => 7 |
There was a problem hiding this comment.
Should probably be named clear
|
|
||
| @type t() :: %__MODULE__{} | ||
|
|
||
| @enforce_keys [:type, :width, :height, :packed_dimensions, :rotation, :accent, :luts] |
There was a problem hiding this comment.
If these are required for one display but not the other, we should consider refactoring things so that we don't force re-usability where it is not required/meaningful (maybe that's exactly what you did here? :D). "False generalisation" only makes things unclear since you don't knew when/if something is required/missing.
There was a problem hiding this comment.
I've reverted this change too, since these keys can peacefully coexist in the struct the defines the impression display. We're not using packed dimensions because it's redundant/unnecessary, but if someone wants to use them in the future, it'll be possible.
As for luts, it's not used at all for the impression. Not sure we need to define a separate struct+key enforcement just for that though.
There was a problem hiding this comment.
This is good food for thought, though... if the devices are diverting, perhaps our configurations should, too.
One way of dealing with it would be to have a protocol common for all types, for the things that are common to all displays and then have the specific drivers check if it is a value of the correct underlying type at runtime during init. But that might be a bit more than what you signed up for at this point and a bit of over-engineering before we have several displays where the fit is off... thoughts?
There was a problem hiding this comment.
That sounds like it might be a worthwhile refactor to open a separate issue for.
| defp write_command(state, command, data) do | ||
| io_call(state, :handle_command, [command, data]) | ||
| state | ||
| end |
There was a problem hiding this comment.
This is why we don't copy-paste stuff 😅
There was a problem hiding this comment.
These are all being used. Is there an outstanding issue here?
jasonmj
left a comment
jasonmj
left a comment
There was a problem hiding this comment.
Got a quick look at the work on this branch. Not a lot to add here, but excited to see progress. Looking forward to getting my impression installed on a pi soon to join in on the fun :)
Impression
There's no code changes, only the version number has been updated: https://github.com/elixir-circuits/circuits_gpio/blob/cc7d77479a4d2ffba1a24962ad067f4680003de4/CHANGELOG.md#v100---10-20-2021
Support circuits_gpio 1.0
Add support for buttons on Inky Impression
|
This can probably be closed in favor of #49 |
4 participants
No description provided.