dhylands / rshell

Remote Shell for MicroPython
MIT License
941 stars 133 forks source link

Problem setting time on Pycom devices #124

Closed amotl closed 4 years ago

amotl commented 4 years ago

Dear Dave,

thanks for conceiving and maintaining rshell. We are using it excessively for building the sandbox environment around the Terkin Datalogger.

When investigating why rshell would upload files over and over again while using its rsync primitive, we found some details we would like to share with you.

Observations I

We found that the time struct (at least) on Pycom devices might have a different layout than what rshell expects.

We believe the time struct is

["tm_year", "tm_mon", "tm_mday", "tm_hour", "tm_min", "tm_sec", "tm_wday", "tm_yday"]

as outlined on [1]. Also, rtc.init() would accept the same format, optionally without the tm_wday and tm_yday slots, which will be derived from the current date anyway.

Example

>>> import time, machine
>>> rtc = machine.RTC()
>>> rtc.init((2020, 2, 28, 22, 32, 41))
>>> time.localtime(time.time())
(2020, 2, 28, 22, 32, 42, 4, 59)

Thoughts

Our observations outlined above are in contrast to the implementation of the sync_time implementation within rshell, where now.tm_wday + 1 is placed between the date and the time. https://github.com/dhylands/rshell/blob/f38a3509a571d014e2e38108c1bc4b07f2fbb969/rshell/main.py#L1509-L1514

Observations II

Directly after invoking rshell, the device outputs a wrong date through time.localtime.

>>> import time
>>> time.localtime(time.time())
(2020, 2, 24, 1, 0, 42, 0, 55)

where the time actually should be 00:29:xx hours something.

Assumption

So, it would be obvious that the rsync primitive will experience some hiccups when comparing timestamps, thus uploading all files over and over again.

Thanks already for looking into this!

With kind regards, Andreas.

[1] https://github.com/micropython/micropython-lib/blob/master/time/time.py#L20-L21

amotl commented 4 years ago

We have made a respective patch just submitted through #125. When combined with https://github.com/pycom/pycom-micropython-sigfox/pull/413, it tremendously speeds up file syncing through rsync, especially with larger MicroPython programs.

dhylands commented 4 years ago

Hi Andreas,

The pyb.RTC.datetime function takes the arguments in the order: (year, month, day, weekday, hours, minutes, seconds, subseconds) http://docs.micropython.org/en/latest/library/pyb.RTC.html#pyb.RTC.datetime

It looks like machine.RTC takes the arguments in a slightly different order: http://docs.micropython.org/en/latest/library/machine.RTC.html#machine.RTC.init

so your patch would wind breaking on the original pyboard. I think that the correct fix would need to be done inside the set_time function: https://github.com/dhylands/rshell/blob/f38a3509a571d014e2e38108c1bc4b07f2fbb969/rshell/main.py#L942-L960

and have the except block build a rearranged rtc_time for passing to the machine class methods.

On Sun, Feb 23, 2020 at 3:34 PM Andreas Motl notifications@github.com wrote:

Dear Dave,

thanks for conceiving and maintaining rshell. We are using it excessively for building the sandbox environment around the Terkin Datalogger https://github.com/hiveeyes/terkin-datalogger.

When investigating why rshell would upload files over and over again while using its rsync primitive, we found some details we would like to share with you. Observations I

We found that the time struct (at least) on Pycom devices might have a different layout than what rshell expects.

We believe the time struct is

["tm_year", "tm_mon", "tm_mday", "tm_hour", "tm_min", "tm_sec", "tm_wday", "tm_yday"]

as outlined on [1]. Also, rtc.init() would accept the same format, optionally without the tm_wday and tm_yday slots, which will be derived from the current date anyway. Example

import time, machine rtc = machine.RTC() rtc.init((2020, 2, 28, 22, 32, 41)) time.localtime(time.time()) (2020, 2, 28, 22, 32, 42, 4, 59)

Thoughts

Our observations outlined above are in contrast to the implementation of the sync_time implementation within rshell, where now.tm_wday + 1 is placed between the date and the time.

https://github.com/dhylands/rshell/blob/f38a3509a571d014e2e38108c1bc4b07f2fbb969/rshell/main.py#L1509-L1514 Observations II

Directly after invoking rshell, the device outputs a wrong date through time.localtime.

import time time.localtime(time.time()) (2020, 2, 24, 1, 0, 42, 0, 55)

where the time actually should be 00:29:xx hours something. Assumption

So, it would be obvious that the rsync primitive will experience some hiccups when comparing timestamps, thus uploading all files over and over again.

Thanks already for looking into this!

With kind regards, Andreas.

[1] https://github.com/micropython/micropython-lib/blob/master/time/time.py#L20-L21

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/dhylands/rshell/issues/124?email_source=notifications&email_token=AAA7EDENX4QZLELRCDGSNHDREMBX3A5CNFSM4KZ62UQ2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4IPTCTRQ, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA7EDDUTXX7W2BVYWEQJLTREMBX3ANCNFSM4KZ62UQQ .

-- Dave Hylands Peachland, BC, Canada http://www.davehylands.com

amotl commented 4 years ago

Hi Dave,

thank you for pointing me towards the correct documentation about this. I totally see your point and will try to amend the PR #125 appropriately.

have the except block build a rearranged rtc_time for passing to the machine class methods

The best way to achieve this would probably be to amend the set_time method to obtain its arguments as **kwargs in order to be capable of arranging it appropriately for passing it down to either pyb.RTC().datetime(...) or otherwise machine.RTC().{datetime,init}(...)? Like

self.remote(set_time,
    year=now.tm_year, month=now.tm_mon, day=now.tm_mday, weekday=now.tm_wday + 1,
    hours=now.tm_hour, minutes=now.tm_min, seconds=now.tm_sec, subseconds=0)

Otherwise, we could stuff the information of the rtc_time tuple within a dictionary to pass it as a single argument when invoking set_time - like:

self.remote(set_time, dict(
    year=now.tm_year, month=now.tm_mon, day=now.tm_mday, weekday=now.tm_wday + 1,
    hours=now.tm_hour, minutes=now.tm_min, seconds=now.tm_sec, subseconds=0))

The alternative for rearrangement would be to amend the tuple itself within set_time by stripping out the 4th tuple item before passing it down to machine.RTC().{datetime,init}(...). While this amendment might have the lowest amount of diff, it will add a certain amount of obfuscation to the code, so I thought about asking you first before making the change.

With kind regards, Andreas.

davehylands commented 4 years ago

What's currently passed to set_time is (year, month, day, weekday, hours, minutes, seconds, subseconds) and for the machine variant, we want (year, month, day[, hour[, minute[, second[, microsecond[, tzinfo]]]]]) so we could construct an rtc_time2 from rtc_time which has the elements in the correct order by using something like:

rtc_time2 = (rtc_time[0], rtc_time[1], rtc_time[2], rtc_time[4], rtc_time[5], rtc_time[6])

and pass rtc_time2 into the machine functions.

amotl commented 4 years ago

And then we will pass both of rtc_time and the rtc_time2 variant as positional arguments into set_time like self.remote(set_time, rtc_time, rtc_time2)?

davehylands commented 4 years ago

No we just need to pass rtc_time and in the except block where it's decided to use machine, just add the line to build rtc_time2 and pass rtc_time2 into the machine.datetime or machine.init

amotl commented 4 years ago

Ok then. I will amend the PR to do it like that. Thanks for your guidance!

amotl commented 4 years ago

Hi Dave,

I've just updated #125 as requested. Please let me know if this needs further adjustments.

Thanks already and with kind regards, Andreas.

dhylands commented 4 years ago

Fixed by #129

amotl commented 4 years ago

Hi Dave,

thank you so much. May I humbly ask if you could push this out as a new release to PyPI?

With kind regards, Andreas.

davehylands commented 4 years ago

Version 0.0.27 should be available now.

amotl commented 4 years ago

That was quick. Thank you again!

-- https://github.com/hiveeyes/terkin-datalogger/commit/492082a5e804addf75f501c155dd5d049f7d3ae4