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

Add methods to set/get devices time zone #1971

Closed
wants to merge 3 commits into from

Conversation

ashthespy
Copy link
Collaborator

@ashthespy ashthespy commented May 18, 2020

Added some methods to set and get the current time zone.
There is lot of scope of improvement, as timedatectl requires currently needs sudo perms.
If only I had read the docs, instead of skimming down to what I needed. --no-ask-password
That also doesn't work, we just have to use the tried and tested NOPASSWD

Roundabout method

We could set up policykit to allow the volumio user to use timedatectl, but I need to explore that more. Something along these lines should work..

$ sudo cat /etc/polkit-1/localauthority/10-vendor.d/10-time-policy.pkla
[Allow volumio user to set time zone]
Identity=unix-user:volumio
Action=org.freedesktop.timedate1.set-timezone
ResultActive=yes

On the FE side, we could pick up the current time zone from the browser in the wizard with
Intl.DateTimeFormat().resolvedOptions().timeZone. Or, for a more wholesome experience, I am sure we can also find some angular lib for time zone selection..

Partially fixes #373 #741

PS: Also included preliminary tests that can be run on the device (with Volumio running)

@ashthespy
Copy link
Collaborator Author

I was going about this all backwards, it just hit me we can just add /usr/bin/timedateclt/ to sudoers.d/volumio-user

Ready for testing :-)

$ node check_TimeZone.js
Emitting getTimeZone
Response pushTimeZone
 { name: 'Europe/London', acronym: 'BST', offset: '+0100' }
Emitting setTimeZone --> Europe/Rome
Response pushset TimeZone
success
Emitting setTimeZone --> CEST
Response pushset TimeZone
Error setting time zone: Error: Command failed: sudo timedatectl set-timezone CEST
Failed to set time zone: Invalid time zone 'CEST'

Exiting

Corresponding BE logs responses

info: CoreCommandRouter::executeOnPlugin: system , getTimeZone
info: setting timezone {"name":"Europe/Rome"}
info: CoreCommandRouter::executeOnPlugin: system , setTimeZone
info: Setting time zone to Europe/Rome
info: setting timezone {"name":"CEST"}
info: CoreCommandRouter::executeOnPlugin: system , setTimeZone
info: Setting time zone to CEST
error: Cannot set TimeZone: Error: Command failed: sudo timedatectl set-timezone CEST
Failed to set time zone: Invalid time zone 'CEST'

@ashthespy ashthespy marked this pull request as ready for review May 19, 2020 13:32
@volumio
Copy link
Owner

volumio commented May 19, 2020

Sorry, just saw it now.
What, in your opinion are the benefits of saving time zone?
Let me explain: our rationale for not having it is that a setting that is not striclty necessary. When we setup alarms, we automatically diff the browser time, so they are accurate without setting up the TZ.

What benefit would this bring? (considering we will have to change how alarm works)

@ashthespy
Copy link
Collaborator Author

To be honest, it's just convenience/nice to have. I am sure a lot of users like me set this up manually right now on each instance..

Some SSL certificate errors pop up at times when the clock is not synced, but I don't see any core functionality issues of sticking to (ntp synced) UTC.

Another advantage is supporting display plugins that have a clock while on standby. Else each plugin has to individually configure the time zone. ( E.g pydpiper.volumio.service )

@Darmur
Copy link
Contributor

Darmur commented Jun 7, 2020

it's useful for clock display on mdp_oled

@volumio
Copy link
Owner

volumio commented Jun 7, 2020

@Darmur I suggest not to use mpd oled: this will show only MPD metadata, and not all other services.
If there is a library for node I can help to hook up the necessary functions

@Darmur
Copy link
Contributor

Darmur commented Jun 9, 2020

I'll have a look to mpd_oled, thanks for the suggestion.

BTW, I think even if it will be ported to node, proper timezone setting will be required to show the clock on display while not playing

@volumio
Copy link
Owner

volumio commented Jun 9, 2020

If we add this method, then we need to refactor the whole alarm plugin time calculation...

@Darmur
Copy link
Contributor

Darmur commented Jun 9, 2020

OK, I didn't look to the alarm plugin implementation to be honest. What I don't understand, if the plugin diff the browser time, why is it an issue to set a different time zone? In theory if you set your alarm after setting new time-zone and after a reboot, the delta should still be computed properly.

@Darmur
Copy link
Contributor

Darmur commented Jun 9, 2020

I can confirm changing time-zone does not affect Alarm plugin. This is the test I did:

  1. Clear all alarms
  2. Set Amsterdam time zone from command line (sudo dpkg-reconfigure tzdata)
  3. Reboot
  4. Set some new alarms

It works fine, alarms were triggered right on time

@ashthespy
Copy link
Collaborator Author

Probably still valid, but given the time that has passed, it is probably better to rebase onto /buster/* branches anyway..

ashthespy added a commit to volumio/volumio3-backend that referenced this pull request Dec 28, 2022
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.

Set TimeZone
3 participants