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 timers using the ESP scheduler #1453

Closed
wants to merge 3 commits into from

Conversation

@mianos
Copy link

commented Sep 5, 2015

This leaks ram with the basic tests. I call the garbage collector at the end of the timer function.
It's probably not worth taking this pull until I work out what is prevented from being collected.

This module adds basic timer scheduling using the native ESP ets_timer_XXX functions that are scheduled outside the repl task thread.

network.connect('iot', 'noodlebrain')

def socket_printer(soc, data):
    print(data)

def on_connect(asoc):
    print("connected")
    asoc.send('GET / HTTP/1.0\r\n\r\n')

def on_disconnect(asoc):
    print("dsconnected")
    asoc.close()

def doit(arg):
    soc = esp.socket()
    soc.onconnect(on_connect)
    soc.ondisconnect(on_disconnect)
    soc.onrecv(socket_printer)
    soc.connect(('216.58.220.132', 80))

aa = esp.timer(doit, 2000, arg="hi")

then:

aa.cancel()
Rob Fowler
@@ -65,6 +65,7 @@ SRC_C = \
moduos.c \
utils.c \
$(BUILD)/frozen.c \
timer.c

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 10, 2015

Contributor

frozen.c should remain last.

}

STATIC const mp_arg_t esp_timer_init_args[] = {
{ MP_QSTR_arg, MP_ARG_KW_ONLY | MP_ARG_OBJ, {.u_obj = mp_const_none} },

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 10, 2015

Contributor

I'd recommend against usage of kw-only args.

};
#define PYB_TIMER_INIT_NUM_ARGS MP_ARRAY_SIZE(esp_timer_init_args)

STATIC mp_obj_t esp_timer_init_helper(esp_timer_obj_t *self, uint n_args, const mp_obj_t *args, mp_map_t *kw_args) {

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 10, 2015

Contributor

Why this 4-line, call-once function is separate, instead being inlined where it's called?

This comment has been minimized.

Copy link
@mianos

mianos Sep 12, 2015

Author

I just copied this verbatim from the other code that handles kwargs. I kind of like the way it makes it quite clear but it's a hack to initialise the vars twice just in case as well. I'll change it.

*
* The MIT License (MIT)
*
* Copyright (c) 2013, 2014 Damien P. George

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 10, 2015

Contributor

Please use your copyright.

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2015

This looks pretty good for 1st version, thanks. Besides inline comments, there's also issue pf function naming - "timer" can be easily confusing with actual hardware Timer, part of standard uPy API. So, other terminology would rather be used - e.g. vtimer, os_timer (I'd prefer the latter, as it names it exactly what it is). (Filename then should change too.)

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 11, 2015

A few points:

  1. You don't need to provide a way to pass an argument into the Python callback (your arg keyword in the constructor). Python provides closures and/or lambdas to do just this, eg esp.timer(lambda: doit("arg"), 2000)
  2. You may want to pass the timer instance to the callback as its only arg (this is how the pyb.Timer callbacks work). This would give you the chance to cancel the timer from within the callback (or you could just use a global variable to access the timer instance).
  3. Currently, if you create a timer object, set a callback, but don't save a reference to the returned timer instance (eg you don't assign to aa in your example above) then the GC could collect the timer object along with the callback, yet the OS still retains a pointer to this now-reclaimed heap memory. To fix it you can store a pointer to the created timer in a global variable that is scanned by the GC. This means you can only create 1 instance of the timer, but that makes sense because there's only 1 OS timer that the callback can be attached to.

What exactly is the symptom of your memory leak?

@mianos

This comment has been minimized.

Copy link
Author

commented Sep 12, 2015

All good points. I will redo it to suit.
Great point about just passing the timer like a self so I can cancel it in the handler.
I am not a fan of lambda passing but if it it what is done for the rest of the project, I'll change that too.
This module is the smallest custom one I have. I'm glad to have the feedback on this one if I am going to post the other ones. :)
The memory leaks when I simply run the code above. The timer calls the tcp request code. I am now learning about GC system so I'll update the timer code to take into account of these changes first and get back to you with a clear test case.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 12, 2015

I am not a fan of lambda passing but if it it what is done for the rest of the project, I'll change that too.

It's a Python thing (not just MicroPython). But usually you don't need to pass a lambda, you just pass the function or method directly and it just works.

The memory leaks when I simply run the code above.

Please be more specific: does memory usage increase on each timer call, or...?

Rob Fowler
@mianos

This comment has been minimized.

Copy link
Author

commented Sep 13, 2015

All changes as reviewed. lambda works much better than an arg.
Changed to esp.os_timer()

import esp
def doit(os_timer_instance, bb):
  print(os_timer_instance)
  print(bb)

kk = 456
xx = esp.os_timer(lambda ss: doit(ss, kk), 2000, repeat=False)
yy =  esp.os_timer(lambda ss: doit(ss, kk), 2000)
..
yy.cancel()
@mianos

This comment has been minimized.

Copy link
Author

commented Sep 13, 2015

In regards to the leaking. It's in the tcp interface handling of something. For example, this never leaks:

def doit(os_timer):
    print("Hello world")
    gc.collect()
    print(gc.mem_free())
aa = esp.os_timer(doit, 1000)

It just prints the same number over and over, yet this:

import esp, network, gc

network.connect('iot', 'noodlebrain')

def socket_printer(soc, data):
    print(data)

def on_connect(asoc):
    print("connected")
    asoc.send('GET / HTTP/1.0\r\n\r\n')

def on_disconnect(asoc):
    print("dsconnected")
    asoc.close()

def doit(os_timer):
    soc = esp.socket()
    soc.onconnect(on_connect)
    soc.ondisconnect(on_disconnect)
    soc.onrecv(socket_printer)
    soc.connect(('216.58.220.132', 80))

def wrapper(os_timer):
    doit(os_timer)
    gc.collect()
    print(gc.mem_free())

prints slowly decreasing values until the mcu crashes.

@@ -136,3 +140,10 @@ Q(localtime)
Q(mktime)
Q(sleep)
Q(time)

// Robs ESP timer

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 15, 2015

Contributor

You have these defined above. Comment is probably not suitable for mainline.

} else {
mp_obj_print_exception(&mp_plat_print, (mp_obj_t)nlr.ret_val);
}
gc_collect(); // done out of timed job and are going back to the operating command and into non determinate timing

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 15, 2015

Contributor

I don't think this belongs here.

This comment has been minimized.

Copy link
@mianos

mianos Sep 15, 2015

Author

Which bit? I need the callback. Do you mean the exception around the call to the python?

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 15, 2015

Contributor

github allows to add comments per-line, and so people add a comment to exact line in question. In this case, it belongs to gc_collect() line.

This comment has been minimized.

Copy link
@mianos

mianos Sep 15, 2015

Author

As this is in the toy operating system thread and the repl that is part of the console is never called. I can't see anywhere the GC will be called otherwise.

This comment has been minimized.

Copy link
@pfalcon

pfalcon Sep 15, 2015

Contributor

GC is called by memory manager when needed. GC should never be called explicitly (modulo exceptions, but such exceptions should not be in general-purpose code).

@pfalcon

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2015

Thanks for refactoring. Please squash changes together, and give at extra look on your side, to remove development comments, etc.

@pfalcon pfalcon changed the title Add timers using the ESP schedular Add timers using the ESP scheduler Sep 15, 2015
mp_printf(print, "timer(callback=%p, period=%u, repeat=%c)",
(unsigned int)&self->callback,
self->period,
self->repeat ? 'y' : 'n');

This comment has been minimized.

Copy link
@dpgeorge

dpgeorge Sep 15, 2015

Member

Repeat state should print True or False.

@dpgeorge

This comment has been minimized.

Copy link
Member

commented Sep 15, 2015

It just prints the same number over and over, yet this:
...
prints slowly decreasing values until the mcu crashes.

Sounds like a problem with esp.socket.

@mianos mianos closed this Sep 19, 2015
@mianos

This comment has been minimized.

Copy link
Author

commented Sep 20, 2015

Cancel this. I'll just maintain my own fork.

tannewt added a commit to tannewt/circuitpython that referenced this pull request Jan 12, 2019
Enable the display on pyportal
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.