Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor and add support for Inky Impression 7.3" #49

Open
wants to merge 41 commits into
base: master
Choose a base branch
from

Conversation

axelson
Copy link
Collaborator

@axelson axelson commented Aug 7, 2023

The Inky Impression 7.3" uses a different chip than the Impression 5.7" and 4" so it required a new HAL module.

To fit this new chip support into the library I also undertook a refactor

  • Rename the phat to phat_original
    • I would have used the chip name instead of "original" but I haven't been able to find the name of the chip anywhere
  • Segregate HAL for phat and impression based on chip type
  • Compile test/support when MIX_TARGET=test

I have tested this on (using https://github.com/axelson/inky_tester):

  • Inky pHAT SSD1608
  • Inky Impression 5.7"
  • Inky Impression 7.3"

2023-08-06 17 32 38

This merges the contents of the impression branch which also includes code/work from @lawik and @jasonmj ❤️

lawik and others added 30 commits October 10, 2021 14:44
Fixes a off by one error
Just put a comment instead of an actual call in case we want to restore
the calls later.
lawik and others added 4 commits August 1, 2022 17:16
Add support for buttons on Inky Impression
Rename the phat to phat_original
Segregate HAL for phat and impression based on chip type
Compile test/support when MIX_TARGET=test
axelson added 2 commits August 6, 2023 17:43
There's no breaking changes except for a small one in circuits spi
1.4.0 that says "Remove Erlang convenience functions since no one used them"
@jasonmj jasonmj self-requested a review August 9, 2023 00:55
@jasonmj
Copy link
Collaborator

jasonmj commented Aug 9, 2023

I would have used the chip name instead of "original" but I haven't been able to find the name of the chip anywhere

I did some searching and found IL91874 at the top of the original python code. Maybe that's it?

Copy link
Collaborator

@jasonmj jasonmj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lookin great! Here's a first pass with plenty of small suggestions and some questions. I don't think anything here is a blocker really, but I'd be happy to see some of these changes.

lib/buttons.ex Outdated Show resolved Hide resolved
lib/buttons.ex Outdated Show resolved Hide resolved
lib/buttons.ex Outdated Show resolved Hide resolved
}
end

# WARNING: This is untested on actual hardware
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I wonder if Pimoroni would be willing to send us one? I went ahead and wrote a quick note to their support team to see if there's a chance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pimoroni is sending me a 4" Impression for testing 🎉

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's great!!

README.md Show resolved Hide resolved
end

#
# pipe-able wrappers
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these are re-used, does it make sense to put them into a common helper module?


spi_address = "spidev0." <> to_string(pin_mappings[:spi])

IO.puts("opening DC pin")
Copy link
Collaborator

@jasonmj jasonmj Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other places, we're using Logger. Probably best to be consistent and use Logger here too.

spi_write(state, @spi_command, value)
end

require Logger
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move up.

lib/hal/io_impression.ex Show resolved Hide resolved
lib/hal/io_impression.ex Show resolved Hide resolved
Copy link
Collaborator

@jasonmj jasonmj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lookin great! Here's a first pass with plenty of small suggestions and some questions. I don't think anything here is a blocker really, but I'd be happy to see some of these changes.

@nyaray
Copy link
Collaborator

nyaray commented Aug 9, 2023

So glad to see this continues to get usage/updates, nothing constructive to add, really. ✌️

@axelson
Copy link
Collaborator Author

axelson commented Aug 13, 2023

I would have used the chip name instead of "original" but I haven't been able to find the name of the chip anywhere

I did some searching and found IL91874 at the top of the original python code. Maybe that's it?

Well spotted! Updated in 975d63b

@cloud8421
Copy link

Just wondering if this PR has fizzled? I’d be able to help from next week if there’s anything I can do. I have some spares I can test on (will have to check which models).

@lawik
Copy link
Collaborator

lawik commented Sep 3, 2024

I don't know if my Impression is broken or if the code is :D

@cloud8421 I believe this ran for @axelson at some point. Try and see if you can get it running if you can. I'd love for us to wrap this up but was not involved very much in the later developments :)

@cloud8421
Copy link

Bit of a hiccup - I just tried to boot my impression 7" with the original firmware and I think there's some hardware issues as it's not working as it was before. Have to dig more but I'm unable to test for now. Will keep you posted.

@cloud8421
Copy link

No luck in reviving the old hardware - I've ordered a new one, it normally takes very little so I HOPE I can test this early in the week.

@lawik
Copy link
Collaborator

lawik commented Sep 13, 2024

I would love if this finally goes over the finish line.

I should shove RPi OS on one of my units and just test my hardware.

@cloud8421
Copy link

cloud8421 commented Sep 18, 2024

Got the Impression 7.3 device and did initial testing.

As a testbed, I've generated a plain application with nerves.new, using a Raspberry PI Zero 2, and loaded a checked out local copy of this branch as a dependency.

I've then run the following code in the device SSH console:

{:ok, pid} = Inky.start_link(:impression_7_3, name: InkySample)

It throws the following error, and causes a device reboot:

opening DC pin
** (EXIT from #PID<0.1306.0>) shell process exited with reason: an exception was raised:
    ** (MatchError) no match of right hand side value: {:error, :export_failed}
        (inky 1.0.2) lib/hal/io_impression.ex:58: Inky.IO.Impression.init/1
        (inky 1.0.2) lib/hal/hal_impression_ac073tc1a.ex:110: Inky.HAL.ImpressionAC073TC1A.init/1
        (inky 1.0.2) lib/inky.ex:203: Inky.init/1
        (stdlib 6.0.1) gen_server.erl:2057: :gen_server.init_it/2
        (stdlib 6.0.1) gen_server.erl:2012: :gen_server.init_it/6
        (stdlib 6.0.1) proc_lib.erl:329: :proc_lib.init_p_do_apply/3

The error maps to:

{:ok, dc_pid} = gpio.open(pin_mappings[:dc_pin], :output, initial_value: 0)

I haven't touched any configuration, so following the code it should resolve to the default 22 pin, which according to the device schematics seems correct.

The impression device is new, but I don't think they've changed chipset: looking at Pimoroni Python's source, they map this model to the chipset implemented in this branch, and the PIN number seems correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants