Rules for registering and unregistering GPIO triggers

[TerryE]

This one is my bad. In tightening up the parameter checking on the gpio.trig() function and removing a memory leak, I have also changed the processing rules. I want to understand the consensus of what we should actually do here. gpio.trig() has one mandatory argument (the pin) and two optional arguments (the level string and the function).

Old processing rules

1. If the level is one of the strings "up", "down", "both", "low", "high", then set the interrupt type accordingly else treat the level as "none".
2. If the callback is specified then unreference any previous callback if it exists, and set the callback reference.
3. Set the platform interrupt for the pin to the specified interrupt type
4. When the interrupt is triggered if the trigger function is specified then call it at ISR level.

These rules had various problems:

• Any mistypes of the level (e.g."Down") would silently default to "none" resulting in a difficult-to-diagnose error.
• There was no documented way to unreference a function, so it and any associated upvals couldn't be GCed.
• It was quite possible to do gpio.trig(4,"up") without ever specifying a callback, which would catch the trigger in an ISR but then do nothing.

New processing rules

1. If the level is one of the strings "none", "up", "down", "both", "low", "high", then set the interrupt type accordingly else throw an error.
2. If the level is none then unreference any previous trigger CB so that is can be GCed, and register the specified trigger to be used for trigger processing.
3. Set the platform interrupt for the pin to the specified interrupt type
4. When the interrupt is triggered then post the trigger task at ISR level. The trigger task then runs at a user level after the current task completes. (No preemption but no random crashes either).

The two main difference are (a) whether we allow level mistypes to default to "none". My vote is -1 to that. (b) Do we continue to allow the use case where an interrupt type can be toggled from "up", say, to "down" without specifying the function, but allow a default to the previous function. Again to my mind this makes no sense. In the rare circumstances when an application does want to change the type, then what is wrong with specifying the callback? The consequence of allowing this is that we can't CG the callback.

I also think that the argument of "not breaking existing applications" doesn't really apply in this case as the old gpio.trig was really unreliable and would regularly crater, so these applications were already broken.

So what is the consensus? I just need the code and the documentation to reflect this.

Thanks to @UncleRedz and @pjsg for pointing this out.

[pjsg]

For this sort of API changes, I do a google search to see what published code does.

https://github.com/sza2/node_hcsr04/blob/master/src/hcsr04.lua
https://github.com/Nervengift/esplight.py/blob/master/init.lua
https://github.com/opendata-stuttgart/sensors-software/blob/master/esp8266/ppd42ns_mqtt/main.lua
https://bluecreation.com/wifi/scripts/latest.lua

These were all from the first page of the search https://www.google.com/search?q=%22gpio.trig%22&oq=%22gpio.trig%22&aqs=chrome..69i57j0l3.3195j0j7&sourceid=chrome&es_sm=91&ie=UTF-8#q=%22gpio.trig%22+filetype:lua

In three of the four cases, they assume that the callback persists (as far as I can tell). Whether their code actually works is unclear to me, but it does appear that they are using the functionality.

Given this, I'm inclined to say that the callback should persist until the pin is closed (or no longer an interrupt pin). My intuition tells me that nodemcu apps are single purpose, and once a callback is registered, it will stay in use for the rest of the time.

[TerryE]

The trigger does persist and did before. You don't need to recall the trig() to re-enable it. This is done by the task wrapper unless you change the mode or the trigger type in the callback. Looking at @sza2's this uses the fact that the CB persists across up/down which is a usecase that I hadn't considered. Nervengift's isn't a real app. @tiborb's is also a real app and seems to think that triggers need to be reissued each interrupt which is incorrect. The bluecreation stuff seems to be a differenct elua implementation and not on the ESP CPU, so I don't think that this is relevant.

Even, so the two sensible patterns would indicate that we should tolerate the optional function callback when the level isn't "none" and there is already an existing trigger callback set. I am still of the view that setting the trigger to none should clear down the callback in the registry. @jmattsson @nickandrew @devsaurus, have you guys got any views here?

[UncleRedz]

Hi,

On A) I prefer the new rules, which do not silently fail or cause unexpected behaviour when an incorrect trigger type is entered.

On B) I would say that it's convenient to be able to change trigger type without specifying the callback. However with my limited experience with Lua, I fail to see any use case where this would be a deal breaker.

As for a breaking change in the API, you could argue that there already is a breaking change due to the rewrite where the callback is posted as a task instead of being directly called. Ignoring the improved stability, this most likely have some impact on timing and possibly some edge cases in behaviour.

So I'm fine with a breaking change, since the behaviour is not guaranteed to be the same anyway. The important thing is that the breaking change results in a controlled error, so that the application developer knows where to look in their code. (Of cause the breaking change should be justifiable, such as fixing stability issues and memory leaks or a flawed design that is not sustainable to maintain.)

Regarding single purpose and registering a trigger and never unsubscribe. It may be perfectly valid to register and read the signals for a while and then unregister. If you put all of that into a lua module, you can unload it and preserve memory while it's not in use.

Best regards,
Hans (and formatting rescued by TerryE)

[UncleRedz]

I think my last reply got mangled, so in short.

Don't allow level mistypes.
None clears the callback, it's gone.
I'm also fine with not allowing none, as long as there is a way to unregister and it's explained in the docs for gpio.trig how to do it.

Allowing change of level without specifying the callback is a nice to have, but if it's causing issues it's not required. You could argue the rewrite is already a breaking change, so you might as well tidy up the API at the same time.

[TerryE]

@UncleRedz Hans, I unscrewed the formatting on your earlier post. Hint: the github mail parser sucks; it's always safer to use the web version)

Allowing the optional callback is easy. We would just need to document explicitly that the cb reference is only cleared down when a gpio.trig(n,"none") or a gpio.mode() is requested for other than gpio.INT.

[TerryE]

OK, I left this a few days to see if we had any other comments. In the absence of these I think that what we are proposing is:

New processing rules (Version 2)

1. If the level is one of the strings "none", "up", "down", "both", "low", "high", then set the interrupt type accordingly, else throw an error.
2. If a cb has been specified for type "none" and for other types if (no cb specified or no previous cb defined), then throw an error.
3. Clear existing cb if type "none" or new cb specified (which enables it to be GCed)
4. Set cb if new cb specified.
5. Set the platform interrupt for the pin to the specified interrupt type.
6. When the interrupt is triggered then post the trigger task at ISR level. The trigger task then runs at a user level after the current task completes. (No pree-mption but no random crashes either).

The essential difference to new rules (V1) is that specifying a new cb is optional to that constructs such as

    gpio.trig(pin, (level==low) and "high" or "low")

can be used to toggle level-based processing within a trigger without needed to respecify the cb. This will also mean that existing sensible coded application will not need changing.

I'll do this change later today or tomorrow and update the documentation accordingly if there are no further changes suggested.

[jmattsson]

Sounds sensible to me.

[pjsg]

This seems to accord with what people are doing -- which is always good. Also, it probably makes sense to update the docs to spell this out.

[TerryE]

Now reflected in PR #1136.