-
-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add web UI notification for under voltage/throttling events #1203
Conversation
Fixes #1199 |
python/common/src/piscsi/sys_cmds.py
Outdated
state_msgs = { | ||
"0": ("error", "Under-voltage detected"), | ||
"1": ("warning", "Arm frequency capped"), | ||
"2": ("error", "Currently throttled"), | ||
"3": ("warning", "Soft temperature limit active"), | ||
"16": ("warning", "Under-voltage has occurred"), | ||
"17": ("warning", "Arm frequency capping has occurred"), | ||
"18": ("warning", "Throttling has occurred"), | ||
"19": ("warning", "Soft temperature limit has occurred"), | ||
} |
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.
As mentioned in Discord, these strings need to be internationalized with gettext before release (i.e. using the _()
alias function). The preferred solution in this codebase is to use return code constants that are interpreted by the implementing client, and then define the English language strings in the client code.
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 how to go about doing that since the current implementation is to return a list of tuples rather than a single return code. I'm open to ideas though.
I just committed a different approach which just uses the _()
alias in the base.html
template where the message is rendered. Is that sufficient or do you want me to refactor the function itself to somehow provide a return code?
Do the translations NEED to exist for a wrapped string to be rendered or will it just default to the original English message if no translation exists? If the translations are required then I'll need someone to create those for this.
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.
Using a tuple is perfectly fine, since what you do with return codes is to have a constant that resolves to a unique integer ID, which is then mapped by the client to a human readable string.
The codes are defined in:
https://github.com/PiSCSI/piscsi/blob/main/python/common/src/piscsi/return_codes.py
Then used for instance like this:
"return_code": ReturnCodes.ATTACHIMAGE_COULD_NOT_ATTACH, |
Then strings are mapped like this in the client: https://github.com/PiSCSI/piscsi/blob/main/python/web/src/return_code_mapper.py
Mapper is called like this:
Line 464 in 57fa874
process = ReturnCodeMapper.add_msg(process) |
I pretty sure your approach here to internationalize the variable messages will not work, since gettext will interpret the variable name as a string and not the value of the variable. You can test this by running the python/web/translation_update.sh script. The script will call gettext to extract all internationalized strings and generate a messages.pot file. If you inspect messages.pot you'll see all the extracted strings for translation, which validates whether your approach works for translations or not.
And to answer your last question, gettext will always fall back to the original (English) string in the code in absence of a translation. We have a process of announcing a code freeze to translators 2 weeks before a release, so that they can update their translations, so you don't have to worry about translating strings that you add yourself.
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 THINK I might have gotten it reworked with the internalization working in the last 2 commits. Would love for you to check it out. I could be doing something incredibly wrong/improper but the translation script does update the translation files.
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.
Scratch that - I still have bugs…
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: [2023-08-14 00:18:59,701] [ERROR] app.py:1741 Exception on / [GET]
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: Traceback (most recent call last):
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: File "/home/pi/piscsi/python/web/venv/lib/python3.9/site-packages/flask/app.py", line 2525, in wsgi_app
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: response = self.full_dispatch_request()
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: File "/home/pi/piscsi/python/web/venv/lib/python3.9/site-packages/flask/app.py", line 1822, in full_dispatch_request
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: rv = self.handle_user_exception(e)
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: File "/home/pi/piscsi/python/web/venv/lib/python3.9/site-packages/flask/app.py", line 1820, in full_dispatch_request
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: rv = self.dispatch_request()
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: File "/home/pi/piscsi/python/web/venv/lib/python3.9/site-packages/flask/app.py", line 1796, in dispatch_request
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: return self.ensure_sync(self.view_functions[rule.endpoint])(**view_args)
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: File "/home/pi/piscsi/python/web/src/web.py", line 260, in index
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: return response(
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: File "/home/pi/piscsi/python/web/src/web.py", line 165, in response
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: env_info = get_env_info()
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: File "/home/pi/piscsi/python/web/src/web.py", line 115, in get_env_info
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: [(s[0], ReturnCodeMapper.add_msg(s[1])) for s in throttled_statuses]
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: File "/home/pi/piscsi/python/web/src/web.py", line 115, in <listcomp>
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: [(s[0], ReturnCodeMapper.add_msg(s[1])) for s in throttled_statuses]
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: File "/home/pi/piscsi/python/web/src/return_code_mapper.py", line 76, in add_msg
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: if "return_code" not in payload:
Aug 14 00:18:59 octoprint PISCSIWEB[1336]: TypeError: argument of type 'int' is not iterable
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.
Ok, I think it’s fixed now. Please let me know if there’s a more proper/cleaner way to do this.
It looks like a clean and light-weight solution. Great job! The one major missing piece is the internationalization of the strings. You may also want to look into adding tests to the integration test suite. The main technical hurdle there would be the mocking of the undervoltage states, I think. |
ARM_FREQUENCY_CAPPED = 101 | ||
CURRENTLY_THROTTLED = 102 | ||
SOFT_TEMPERATURE_LIMIT_ACTIVE = 103 | ||
UNDER_VOLTAGE_HAS_OCCURED = 116 |
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.
Nitpick: Spelling of "OCCURRED"
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.
This was misspelled in a few places. They should all be fixed now.
python/web/src/web.py
Outdated
@@ -69,6 +69,8 @@ | |||
TEMPLATE_THEMES, | |||
TEMPLATE_THEME_DEFAULT, | |||
TEMPLATE_THEME_LEGACY, | |||
THROTTLE_NOTIFY_MODES, | |||
THROTTLE_TEST_MODES |
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.
Nitpick: It is the coding convention in this project to add a trailing comma to the final element in a list.
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.
Fixed this to follow convention in a few areas.
Great job getting the i18n job done! I think this will work, although I haven't tested it myself yet. Two things: |
I committed a revert that should reset the files back to what they were.
How can I modify values from the |
python/web/src/return_code_mapper.py
Outdated
ReturnCodes.SOFT_TEMPERATURE_LIMIT_ACTIVE: | ||
_("Soft-temperature limit active"), | ||
ReturnCodes.UNDER_VOLTAGE_HAS_OCCURRED: | ||
_("Under voltage has occured"), |
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.
One remaining typo here! (should say "occurred")
python/web/src/return_code_mapper.py
Outdated
ReturnCodes.UNDER_VOLTAGE_DETECTED: | ||
_("Under voltage detected"), | ||
ReturnCodes.ARM_FREQUENCY_CAPPED: | ||
_("Arm frequency capped"), |
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.
Should "Arm" be all uppercase?
Regarding tests, a simplistic approach would be to run the throttling test only when running in Docker, so that you leverage your mock. You can use |
As per Discord, the tests may not be overly beneficial for this change & thus it might be best to table them. I'll let the maintainers decide if I should pursue it more or not. Marking this PR as ready to review so others might test it out. |
I check out and ran the code. It basically works well! One thing I noticed is that I had to put the Another piece of feedback is that I think a more actionable message might be needed. What do you want users to do when they see those messages? Can you give them a little more context what this state actually means? And finally (this is kind of minor) since only two of the states are actually used right now, I don't think we should mark the rest for gettext i18n. It's kind of rude to ask translators to translate a bunch of obscure strings that will never be used, don't you think. ;) |
I did have some issues getting the module in the right place but I thought I had it. I'll make sure it's added to
I've added more descriptive messages that look like this:
I think that since we make it configurable which errors to display that they should all be configured with potential translations. I was under the understanding that if no translation is done for an item it will default to the English version anyhow so I'm not sure what we gain from hardcoding the English versions & complicating the logic with inconsistency. I think all the errors are potentially useful if someone wants full insight even though we only enable 2 of them by default. |
Should the |
I'm not sure to be frank... I'm not entirely sure when |
Fair enough, let's do it your way. :) |
Adds a banner under the header banner showing when an active or previous under voltage or other throttling events have occured on the Raspberry Pi. The changes use the vcgencmd Python module that provides an interface for the underlying
vcgencmd
utility that comes with the Rasperry Pi OS distribution.The modes that are displayed can be configured via the
python/web/src/settings.py
file.Example with current styling...
Classic Theme:
Modern Theme:
Potential TODOs
vcgencmd
for Docker development environment (non-Raspberry Pi environment)