Fixes broken tmr.alarm

[vsky279]

Fixes #3261.

Make sure all boxes are checked (add x inside the brackets) when you submit your contribution, remove this sentence before doing so.

• This PR is for the dev branch rather than for master.
• This PR is compliant with the other contributing guidelines as well (if not, please describe why).
• I have thoroughly tested my contribution.
• The code changes are reflected in the documentation at docs/*.

Fixes broken tmr.alarm following the commit d72ea91.

The following code fragments now work correctly:

if not tmr.create():alarm(5000, tmr.ALARM_SINGLE, function()
  print("hey there")
end)
then
  print("whoopsie")
end

mytimer = tmr.create()
mytimer:register(5000, tmr.ALARM_SINGLE, function() print("hey there") end)
if not mytimer:start() then print("uh oh") end

[nwf]

Minor nits, but at a glance it LGTM.

[KT819GM]

Edit 3
With cbf2afa problems I've wrote are gone.

Edit 2
Code I've posted (SoftUart readout) is working OK with 87030a8

Edit
Example code fragments work OK without stopping, so I'm taking back my bug report as it's most likely something bad in my code.

---

I'm still having problems, and thinking that it's related to timer. Timer will stop after some iterations (it's not constant, could be 5, could be 10). s:on part stays alive, as I manualy can call s:write("1") and it will fire response. Previous dev was behaving correctly. Dirty code sample for this would be:

--read_serial
t = {0,0,0,0,0,0,0,0,0,0,0}
index = 1
s = softuart.setup(9600, 6,7)
s:on("data","\n", function(data) print('Data received, printing')
for token in string.gmatch(data, "%w+") do
    t[index] = token
    index = index + 1
    end
    print(t[2])
end)

tmr.create():alarm(2000, tmr.ALARM_AUTO, function()
print('Reading values')
index = 1
s:write("1")
    end)```

[vsky279]

Everything works fine under Lua 5.3. However under Lua 5.1 this one does not work (no timer is fired)

print(tmr.create():alarm(1000, tmr.ALARM_AUTO, function() print("hey there") end));

while

t=tmr.create();print(t:alarm(1000, tmr.ALARM_AUTO, function() print("hey there") end));

works correctly.

This seems like a woodoo to me.

EDIT:
Ok, it's clear. The luaL_ref needs to have UD on the top of the stack. So returning back lua_settop(L, 1); at line 135 fixes the issue.

[mk-pmb]

I observed heap usage flickering with this fix and Lua 5.3. It was not present in my previous firmware image with Lua 5.1, but I didn't keep a backup so I can't compare. I'm trying to find a somewhat recent dev commit to rebuild a Lua 5.1 firmware and compare. However this may take a bit because somehow a lot of the late dev commits won't build for me.

[vsky279]

I think this can be due to different GC algorithm in Lua 5.3 (there is a discussion about it in #2783 and #3152). Try to run collectgarbage() when your timer is running.

[mk-pmb]

Yes, thanks for explaining. Now when I collect garbage in each run, the heap use settles to a fixed number starting at the 3rd trigger.

[TerryE]

The 5.3 is GC strategy is lazier than 5.1. 5.1 effectively does a GC after every malloc. This gives a nice low heap, but slugs runtime by maybe 3× or thereabouts. 5.3 is less frequent and sweeps up more when it does -- hence the heap flicker. As Lukas says, you can always do a GC at the end of your timer function to force housekeeping before you sleep again.

[TerryE]

This is only needed because tmr_start() is a LUA_API function but is also called directly by another LUA_API function tmr_alarm(), which is a bit naughty. Since the alarm function is doing the naughties, then it should get the stack right, and this settop should be moved to the line immediately above the trm_start(L) call in trm_alarm().

According to the documentation:

• start is supposed to return nil if the timer is not registered and (true, mode) otherwise. yet in reality it only returns the boolean (was the timer started?)
• alarm' is supposed trueif the timer was started andfalse` on error.

So t:start(true) is supposed to force a restart, but what is the behaviour of t:alarm() if the timer is already running? Do you need the restart flag here as well.

Either way, the documentation should reflect the actual behaviour.

[vsky279]

@TerryE I already have lua_settop in tmr_alarm() at line 155-157.

This one lua_settop is there because we need the tmr.timer UD on the top of the stack before eventually calling

tmr->self_ref = luaL_ref(L, LUA_REGISTRYINDEX);

This gets rid off the optional argument.

Do you agree with this reasoning and approach?

[nwf]

Can we decouple tmr_start() and tmr_alarm() so that their LUA_API aspects are separate (but they might use common bits of C code encapsulated as functions)?

[vsky279]

Do you find this still too dirty?

I think it would not be very elegant as in tmr_register only one line is C code (os_timer_setfn(&tmr->os, alarm_timer_common, tmr);) and 16 lines relates to LUA_API.
It's very similar for tmr_start.

To me this implementation is not so much dirty and if documented well in the code it is readable and does the job well.

[TerryE]

Lukas, the LUA_API entries are really intended to be invoked through lua_call() If we are calling one directly then this should be clearly documented inline. Either the caller or the callee should properly balance the Lua stack inbound and outbound.

Since we document the behaviour of start with the optional flag, the tobj:alarm() documentation should explain what happens is a tobj is an existing timer has been started.

[vsky279]

the LUA_API entries are really intended to be invoked through lua_call() If we are calling one directly then this should be clearly documented inline. Either the caller or the callee should properly balance the Lua stack inbound and outbound.

This is clear. I think the proposed implementation does exactly that.

Since we document the behaviour of start with the optional flag, the tobj:alarm() documentation should explain what happens is a tobj is an existing timer has been started.

Do you think we should add optional parameter restart to tmr.alam()? That would be probably the way giving the highest degree of freedom to user. It should be easy to be implemented + documented inline as well as in documentation.
As tmr.register stops the timer if it's running (L120) the following call of tmr.start in tmr.alarm alway acts like if tmr.start is asked to restart the timer. I think this is the right behavior. It works as one would expect.
I will update the documentation to describe this behavior.

[marcelstoer]

I think this is the right behavior.

I went through the documentation and checked the obj methods again for consistency (primarily behavioral). It all makes sense to me.

[marcelstoer]

I tested all of the documented behavior on the timer obj: ✔️