-
Notifications
You must be signed in to change notification settings - Fork 86
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
base: master
Are you sure you want to change the base?
Implement for Waveshare 6 inch HD screen #48
Conversation
For English and Vietnamese.
To Waveshare 6 in HD screen, it requires the driver from https://github.com/GregDMeyer/IT8951 |
There was a problem hiding this 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") |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing changed
For English and Vietnamese.