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

display of day of the week is off #17

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

Conversation

the-it
Copy link
Contributor

@the-it the-it commented Oct 13, 2021

No description provided.

@cpyarger
Copy link
Contributor

Functionally this is identical. only needlessly complicated. and not logically sound, as Sunday is the first day of the week, therefore 0-6 was correct.
Title of the commit is also misleading. My previous commit correctly displayed the correct day of the week. unless you have changed the rtc to start with 1 based numbering instead of 0 based numbering.

@the-it
Copy link
Contributor Author

the-it commented Oct 14, 2021

For me, it just shows the wrong day of the week (today on Thursday it shows Tuesday). Maybe we should implement the correct setting for day of the week somewhere in our code?

@malcolmholmes
Copy link
Owner

@the-it I take it you have set the date correctly? The day of the week is derived from the date (Asking the stupid questions first)

@the-it
Copy link
Contributor Author

the-it commented Oct 14, 2021

@the-it I take it you have set the date correctly? The day of the week is derived from the date (Asking the stupid questions first)

yes done that correctly. Even get a dump of the date in the console

@cpyarger
Copy link
Contributor

I set the day of the week manually when I first set the RTC. and once the DOW is set it is pulled directly from the RTC not calculated.

@the-it
Copy link
Contributor Author

the-it commented Oct 19, 2021

I think we should set the DOW in the init routine of the RTC object. If code relies on a correct setting ... we should set it accordingly.

@malcolmholmes
Copy link
Owner

Is the DOW just an incrementing number in the RTC? i.e. is it down to code to make sure that the DOW is correct in relation to the date? In which case, correcting the DOW from the date in code makes sense.

@cpyarger
Copy link
Contributor

I believe you only need to set the DOW when setting the time. and Yes its an incrementing 1:1 integer coming from the RTC

@malcolmholmes
Copy link
Owner

@cpyarger so we need to check if the time setting code is correct regarding weekday. @the-it can you take a peek?

@twisst
Copy link
Contributor

twisst commented Aug 3, 2022

Hi, I had the same problem. I think the cause is that the weekday number is shifted up and down in several places in the code.

On line 79 in ds3231_port.py it saves the weekday to the DS3231 RTC after adding 1.

When converting to localtime format (I'm not sure why that is necessary), ds3231_port.py on line 64 subtracts 1 from the weekday.
(It also subtracts 1 on line 116 and line 128 when doing rtc_test().)

The list 'day_of_week' in display.py (line 184) is in the wrong order, with Sun as 0 and Mon as 1. I'd call that wrong because the clock shows the icons Monday-first, and all datetime functions in Python and MicroPython report the weekday between 0 and 6, where 0 is Monday.

So when cleaning that up, my clock shows the right icon for today's date.
The RTC module, the system time (import time; time.localtime()) and the time the clock is working with are all in agreement now what the weekday is (I checked by printing to the shell).

I did have to reset weekday in the RTC manually, because --if I'm following the code correctly-- the RTC module will only be updated when it is not already set. So if you have a wrong weekday in there, it will only be corrected when the date changes I guess? Or when you take the battery out of the RTC module :-) The easier way is to run a script from Thonny to change the Pico's systems time (and maybe other settings). I will add that script and turn this whole comment into a PR later.

@twisst
Copy link
Contributor

twisst commented Aug 3, 2022

Just to add some background, I think what may be the source of confusion is that there seems to have been a bug in MicroPython's port for Pico:

micropython/micropython#7889
micropython/micropython@5679fe6

That means that an offset was needed before, in the original software for this clock, but now that can be left out.

twisst added a commit to twisst/pico-clock-green-python that referenced this pull request Aug 4, 2022
A bug in MicroPython for Pico was fixed, which means this clock doesn't have to offset the day of the week anymore.

See discussion in malcolmholmes#17
@twisst twisst mentioned this pull request Aug 4, 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.

4 participants