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

Implement for Waveshare 6 inch HD screen #48

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FutureTravelATM
Copy link

For English and Vietnamese.

For English and Vietnamese.
@FutureTravelATM
Copy link
Author

To Waveshare 6 in HD screen, it requires the driver from https://github.com/GregDMeyer/IT8951

Copy link
Collaborator

@lightisfaster lightisfaster left a comment

Choose a reason for hiding this comment

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

The PR brings some interesting things to discuss. Overall, the commit is huge and i think it would be nicer to make some smaller PR that are easier to review.
The PR contains several local configurations that have no global added value. In my opinion, PR cannot be merged like this local defines. Local settings need to be configurable without touch the code. I still think it's nice that the atm is used in vn. Have fun.

config.COINCOUNT += 1
config.SATS = utils.get_sats()
config.SATSFEE = utils.get_sats_with_fee()
config.SATS -= config.SATSFEE
logger.info("2 cents added")
logger.info("10,000 VND added")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Affects all devices, must be configurable and have cents as the default value. Otherwise it will confuse users.

display.update_amount_screen()
if config.PULSES == 3:
config.FIAT += 0.05
config.FIAT += 20000
Copy link
Collaborator

Choose a reason for hiding this comment

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

Affects all devices, must be configurable. Otherwise it will confuse users. Is fixed with #37

import math
from shutil import copyfile
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that absolutely necessary or just style?

@@ -135,10 +163,18 @@ def create_config(config_file=None):
SATS = 0
SATSFEE = 0
INVOICE = ""
# This markup will make the rate in VN nearer the market
MARKUP = 1.08
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure with this variables. A fixed definition of MARKUP is not a good idea, better was is to config this because VN is not of global importance, sorry.

restart_screen_1 = "ATM rebooting"
restart_screen_2 = "Please wait for"
restart_screen_3 = "some seconds."
restart_screen_4 = "ATM đang khởi động"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Breaks the current concept. We can discuss this but my idea would be to use different message files for different languages.

@@ -1,14 +1,14 @@
[atm]
# Set your fiat currency with the three letter
# currency code (https://www.xe.com/symbols.php)
cur = eur
cur = vnd
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a local change that has no global added value.


# Define what a cent is called in the currency
# of your choice for price display (singular).
centname = cent
centname = Dong
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a local change that has no global added value


# Set the Fee in %
fee = 2
fee = 8.5
Copy link
Collaborator

Choose a reason for hiding this comment

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

It is a local change that has no global added value. Changing default value is not necessary

@@ -19,13 +19,13 @@ dangermode = on
# Current options are:
# display = papiruszero2in
# display = waveshare2in13
display = papiruszero2in
display = waveshare6in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Changing default value is not necessary


# Automatically set during initial setup to LND or LNTXBOT
# Current options are:
# activewallet = btcpay_lnd
# activewallet = lntxbot
activewallet =
activewallet =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nothing changed

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.

2 participants