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

New option for Waveshare 2.13 (D) display #44

Merged
merged 4 commits into from
Sep 2, 2021

Conversation

lightisfaster
Copy link
Collaborator

Based on the implementation of waveshare2in13, the unsupported commands FULL_UPDATE and PART_UPDATE have been eliminated. Furthermore, the alignment of the messages has been adjusted.

@k9ert
Copy link
Contributor

k9ert commented Aug 29, 2021

What's missing here for this to get merged?

BUG: Replace displayPartial call, because it doesn't work.
BUG: Replace displayPartBaseImage call, because it crashes, it doesn't exist in the driver
@lightisfaster
Copy link
Collaborator Author

Some texts on the screens were not finally positioned. This is done, now.

@@ -135,6 +134,15 @@ def create_config(config_file=None):
logger.warning("Waveshare display library not installed.")
sys.exit("Exiting...")

# Display - Waveshare 2.13 (D) is 212 * 104 pixels
if "waveshare2in13d" in conf["atm"]["display"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

These options are mutually exclusive. I'm not sure how to install the library (maybe i did that wrong) but why not using elif constructs? This fixed my issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's correct. I took the normal if so as not to touch the papirus block. If the correct display is choose in the config.ini, there are no problems either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We can make the change with a separate PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure i understand the approach. Why don't you want to touch the papirus block? This is a configuration procedure and there is some kind of fallback-mode via "waveshare" (as it's not 100% specified). However, that works with elif as well and these values are mutually exclusive. Using "if", not elif, makes it error-prone: I had issues with the normal if-block. Maybe i missed the point somewhere: Where is the description of the installation of the waveshare-library? Maybe i did that wrong.
But that should not prevent doing elifs?!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If you look at the existing PRs, you will see the touch of the papirus block will results in additional merge conflicts. I would therefore prefer to have a new PR with the elif. Then we can choose the time when we approve the adjustment. I have no problems using the normal if.

@k9ert
Copy link
Contributor

k9ert commented Aug 30, 2021

Ok, so yesterday i could prove that it's working in "hat-mode". When i started the app.py it resulted in "LightningATM" Logo and "insert coins". However, when i started to wire as described in the video, it stopped working. I rewired twice and triple-checked each time but couldn't get it to work.
I'm not sure where this is coming from. Maybe one of my wires have a defect. It's very unlikely to have anything to do with this PR but wanted to mention it nevertheless.

After solving the elif-issue, i think this is ready to merge, but what do i know.

@lightisfaster
Copy link
Collaborator Author

It's really strange.
Unfortunately I can't comment on that.
@21isenough has write access and can edit the code before merge. If he think it is a good idea to use elif, he can do this. Otherwise we will apply for the elif change afterwards.

@21isenough 21isenough merged commit 378b004 into 21isenough:master Sep 2, 2021
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.

3 participants